lkml.org 
[lkml]   [2021]   [Jan]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH v2] proc_sysctl: fix oops caused by incorrect command parameters.
From
Date
On 2021/1/9 9:50, Andrew Morton wrote:
> On Fri, 8 Jan 2021 21:10:25 +0100 Michal Hocko <mhocko@suse.com> wrote:
>
>>>> Why would that matter? A missing value is clearly a error path and it
>>>> should be reported.
>>>
>>> This test is in the correct place. I think it's just a question of the
>>> return values.
>>
>> I was probably not clear. The test for val is at the right place. I
>> would just expect -EINVAL and have the generic code to report.
>
> It does seem a bit screwy that process_sysctl_arg() returns zero in all
> situations (parse_args() is set up to handle an error return from it).
> But this patch is consistent with all the other error handling in
> process_sysctl_arg().
> .
>


Set the kernel startup parameter to "nosmp nokaslr hung_task_panic"
and test the startup logs of different patches.

patch1:
+++ b/fs/proc/proc_sysctl.c
@@ -1757,6 +1757,11 @@ static int process_sysctl_arg(char *param, char
*val,
loff_t pos = 0;
ssize_t wret;

+ if (!val) {
+ pr_err("Missing param value! Expected
'%s=...value...'\n", param);
+ return 0;
+ }
+
if (strncmp(param, "sysctl", sizeof("sysctl") - 1) == 0) {
param += sizeof("sysctl") - 1;

sysctl log for patch1:
Missing param value! Expected 'nosmp=...value...'
Missing param value! Expected 'nokaslr=...value...'
Missing param value! Expected 'hung_task_panic=...value...'

patch2:
+++ b/fs/proc/proc_sysctl.c
@@ -1756,6 +1756,8 @@ static int process_sysctl_arg(char *param, char *val,
int err;
loff_t pos = 0;
ssize_t wret;
+ if (!val)
+ return -EINVAL;

if (strncmp(param, "sysctl", sizeof("sysctl") - 1) == 0) {
param += sizeof("sysctl") - 1;

sysctl log for patch2:
Setting sysctl args: `' invalid for parameter `nosmp'
Setting sysctl args: `' invalid for parameter `nokaslr'
Setting sysctl args: `' invalid for parameter `hung_task_panic'

patch3:
+++ b/fs/proc/proc_sysctl.c
@@ -1770,6 +1770,9 @@ static int process_sysctl_arg(char *param, char *val,
return 0;
}

+ if (!val)
+ return -EINVAL;
+
/*
* To set sysctl options, we use a temporary mount of proc, look up the
* respective sys/ file and write to it. To avoid mounting it when no

sysctl log for patch3:
Setting sysctl args: `' invalid for parameter `hung_task_panic'

patch4:
+++ b/fs/proc/proc_sysctl.c
@@ -1757,6 +1757,9 @@ static int process_sysctl_arg(char *param, char *val,
loff_t pos = 0;
ssize_t wret;

+ if (!val)
+ return 0;
+
if (strncmp(param, "sysctl", sizeof("sysctl") - 1) == 0) {
param += sizeof("sysctl") - 1;

sysctl log for patch3:
no log

When process_sysctl_arg() is called, the param parameter may not be the
sysctl parameter.

Patch3 or patch4, which is better?

Thanks
Xiaoming Ni
\
 
 \ /
  Last update: 2021-01-11 04:50    [W:0.094 / U:4.424 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site