On 2021/1/9 9:50, Andrew Morton wrote:
On Fri, 8 Jan 2021 21:10:25 +0100 Michal Hocko <mhocko@xxxxxxxx> 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
