| Message ID | 20200330115535.3215-4-vbabka@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | support setting sysctl parameters from kernel command line | expand |
On Mon, Mar 30, 2020 at 01:55:35PM +0200, Vlastimil Babka wrote: > We can now handle sysctl parameters on kernel command line and have > infrastructure to convert legacy command line options that duplicate sysctl > to become a sysctl alias. > > This patch converts the hung_task_panic parameter. Note that the sysctl handler > is more strict and allows only 0 and 1, while the legacy parameter allowed > any non-zero value. But there is little reason anyone would not be using 1. > > Signed-off-by: Vlastimil Babka <vbabka@suse.cz> I'll let others double-check, but I think this change should be okay. If not, we can adjust the sysctl handler to accept an arbitrary int. Reviewed-by: Kees Cook <keescook@chromium.org> -Kees > --- > Documentation/admin-guide/kernel-parameters.txt | 2 +- > fs/proc/proc_sysctl.c | 1 + > kernel/hung_task.c | 10 ---------- > 3 files changed, 2 insertions(+), 11 deletions(-) > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > index 81ff626fc700..e0b8840404a1 100644 > --- a/Documentation/admin-guide/kernel-parameters.txt > +++ b/Documentation/admin-guide/kernel-parameters.txt > @@ -1457,7 +1457,7 @@ > [KNL] Should the hung task detector generate panics. > Format: <integer> > > - A nonzero value instructs the kernel to panic when a > + A value of 1 instructs the kernel to panic when a > hung task is detected. The default value is controlled > by the CONFIG_BOOTPARAM_HUNG_TASK_PANIC build-time > option. The value selected by this boot parameter can > diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c > index 97eb0b552bf8..77b1b844b02b 100644 > --- a/fs/proc/proc_sysctl.c > +++ b/fs/proc/proc_sysctl.c > @@ -1743,6 +1743,7 @@ struct sysctl_alias { > */ > static const struct sysctl_alias sysctl_aliases[] = { > {"numa_zonelist_order", "vm.numa_zonelist_order" }, > + {"hung_task_panic", "kernel.hung_task_panic" }, > { } > }; > > diff --git a/kernel/hung_task.c b/kernel/hung_task.c > index 14a625c16cb3..b22b5eeab3cb 100644 > --- a/kernel/hung_task.c > +++ b/kernel/hung_task.c > @@ -63,16 +63,6 @@ static struct task_struct *watchdog_task; > unsigned int __read_mostly sysctl_hung_task_panic = > CONFIG_BOOTPARAM_HUNG_TASK_PANIC_VALUE; > > -static int __init hung_task_panic_setup(char *str) > -{ > - int rc = kstrtouint(str, 0, &sysctl_hung_task_panic); > - > - if (rc) > - return rc; > - return 1; > -} > -__setup("hung_task_panic=", hung_task_panic_setup); > - > static int > hung_task_panic(struct notifier_block *this, unsigned long event, void *ptr) > { > -- > 2.25.1 >
On 3/30/20 10:43 AM, Kees Cook wrote: ... >> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt >> index 81ff626fc700..e0b8840404a1 100644 >> --- a/Documentation/admin-guide/kernel-parameters.txt >> +++ b/Documentation/admin-guide/kernel-parameters.txt >> @@ -1457,7 +1457,7 @@ >> [KNL] Should the hung task detector generate panics. >> Format: <integer> >> >> - A nonzero value instructs the kernel to panic when a >> + A value of 1 instructs the kernel to panic when a >> hung task is detected. The default value is controlled >> by the CONFIG_BOOTPARAM_HUNG_TASK_PANIC build-time >> option. The value selected by this boot parameter can >> diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c >> index 97eb0b552bf8..77b1b844b02b 100644 >> --- a/fs/proc/proc_sysctl.c >> +++ b/fs/proc/proc_sysctl.c >> @@ -1743,6 +1743,7 @@ struct sysctl_alias { >> */ >> static const struct sysctl_alias sysctl_aliases[] = { >> {"numa_zonelist_order", "vm.numa_zonelist_order" }, Hi Vlastimil, Maybe best to delete the above line? Because: a) it was added as an example, and now that you have a real use case in this patch, the example is no longer required, and b) numa_zonelist_order is deprecated, as a boot param. Adding support to it in this brand-new mechanism seems to be going a bit in the opposite direction of deprecation. And, I don't think you really want all the sysctls to be enabled as boot params, right? Your comment right above sysctl_aliases[] (shown in patch 2) sort of indicates that only some items are meant to be both sysctl's and boot params. And that makes sense. In fact, the sysctl_aliases[] is (or could be) effectively the whitelist that Luis Chamberlain was requesting in another thread. A whitelist makes good sense, for the reasons Luis listed. As such, keeping it limited to items that we want, seems like the way to go, IMHO. thanks,
On 3/31/20 2:34 AM, John Hubbard wrote: > On 3/30/20 10:43 AM, Kees Cook wrote: > ... >>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt >>> index 81ff626fc700..e0b8840404a1 100644 >>> --- a/Documentation/admin-guide/kernel-parameters.txt >>> +++ b/Documentation/admin-guide/kernel-parameters.txt >>> @@ -1457,7 +1457,7 @@ >>> [KNL] Should the hung task detector generate panics. >>> Format: <integer> >>> >>> - A nonzero value instructs the kernel to panic when a >>> + A value of 1 instructs the kernel to panic when a >>> hung task is detected. The default value is controlled >>> by the CONFIG_BOOTPARAM_HUNG_TASK_PANIC build-time >>> option. The value selected by this boot parameter can >>> diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c >>> index 97eb0b552bf8..77b1b844b02b 100644 >>> --- a/fs/proc/proc_sysctl.c >>> +++ b/fs/proc/proc_sysctl.c >>> @@ -1743,6 +1743,7 @@ struct sysctl_alias { >>> */ >>> static const struct sysctl_alias sysctl_aliases[] = { >>> {"numa_zonelist_order", "vm.numa_zonelist_order" }, > > > Hi Vlastimil, > > Maybe best to delete the above line? Because: > > a) it was added as an example, and now that you have a real use case in this patch, > the example is no longer required, and > > b) numa_zonelist_order is deprecated, as a boot param. Adding support to it in this > brand-new mechanism seems to be going a bit in the opposite direction of deprecation. Well, this aliases table is not the brand new mechanism, it's just for handling sysctls that also have a legacy boot param. numa_zonelist_order is such legacy boot param, so we can handle it here instead of its special handler. If we decide to remove it later, we can do that, but there is no user-visible effect on its deprecation by this series. > And, I don't think you really want all the sysctls to be enabled as boot params, right? Your The point of Patch 1 is very much so that all sysctls can be set using a boot param in the form of sysctl.foo.bar=baz > comment right above sysctl_aliases[] (shown in patch 2) sort of indicates that only some items > are meant to be both sysctl's and boot params. And that makes sense. Patches 2+3 are only about handling the legacy boot params that have a sysctl counterpart. > In fact, the sysctl_aliases[] is (or could be) effectively the whitelist that Luis Chamberlain > was requesting in another thread. A whitelist makes good sense, for the reasons Luis listed. > As such, keeping it limited to items that we want, seems like the way to go, IMHO. See my reply there once I send it :) > thanks, >
On 3/31/20 12:27 AM, Vlastimil Babka wrote: ... > Patches 2+3 are only about handling the legacy boot params that have a sysctl > counterpart. > OK, I misread what those were for. >> In fact, the sysctl_aliases[] is (or could be) effectively the whitelist that Luis Chamberlain >> was requesting in another thread. A whitelist makes good sense, for the reasons Luis listed. >> As such, keeping it limited to items that we want, seems like the way to go, IMHO. > > See my reply there once I send it :) > Saw that, and it all sounds good now. thanks,
On 2020/03/30 20:55, Vlastimil Babka wrote: > @@ -63,16 +63,6 @@ static struct task_struct *watchdog_task; > unsigned int __read_mostly sysctl_hung_task_panic = > CONFIG_BOOTPARAM_HUNG_TASK_PANIC_VALUE; > > -static int __init hung_task_panic_setup(char *str) > -{ > - int rc = kstrtouint(str, 0, &sysctl_hung_task_panic); > - > - if (rc) > - return rc; > - return 1; > -} > -__setup("hung_task_panic=", hung_task_panic_setup); Can we defer removal of this handler for "one release cycle" (and instead emit a line saying that "this parameter will be replaced by ..." during that cycle) ? I welcome PATCH 1/3, but kernel testing projects (e.g. syzbot) needs to update their settings between PATCH 1/3 was merged into linux.git and PATCH 3/3 is merged into linux.git . https://lkml.kernel.org/r/CACT4Y+YE-j5ncjTGN6UhngfCNRgVo-QDZ3VCBGACdbs9-v+axQ@mail.gmail.com says "Announcing unmerged changes is too early (as this patch showed). And once it's in linux-next it's already too late.." > - > static int > hung_task_panic(struct notifier_block *this, unsigned long event, void *ptr) > { >
On 4/1/20 1:12 AM, Tetsuo Handa wrote: > On 2020/03/30 20:55, Vlastimil Babka wrote: >> @@ -63,16 +63,6 @@ static struct task_struct *watchdog_task; >> unsigned int __read_mostly sysctl_hung_task_panic = >> CONFIG_BOOTPARAM_HUNG_TASK_PANIC_VALUE; >> >> -static int __init hung_task_panic_setup(char *str) >> -{ >> - int rc = kstrtouint(str, 0, &sysctl_hung_task_panic); >> - >> - if (rc) >> - return rc; >> - return 1; >> -} >> -__setup("hung_task_panic=", hung_task_panic_setup); > > Can we defer removal of this handler for "one release cycle" (and instead emit a line > saying that "this parameter will be replaced by ..." during that cycle) ? I welcome The old parameter is not removed, it's just handled differently, see patch 2. Both old name and new sysctl.name will work. > PATCH 1/3, but kernel testing projects (e.g. syzbot) needs to update their settings > between PATCH 1/3 was merged into linux.git and PATCH 3/3 is merged into linux.git . > > https://lkml.kernel.org/r/CACT4Y+YE-j5ncjTGN6UhngfCNRgVo-QDZ3VCBGACdbs9-v+axQ@mail.gmail.com says > "Announcing unmerged changes is too early (as this patch showed). And once it's in linux-next it's already too late.." > >> - >> static int >> hung_task_panic(struct notifier_block *this, unsigned long event, void *ptr) >> { >> >
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt index 81ff626fc700..e0b8840404a1 100644 --- a/Documentation/admin-guide/kernel-parameters.txt +++ b/Documentation/admin-guide/kernel-parameters.txt @@ -1457,7 +1457,7 @@ [KNL] Should the hung task detector generate panics. Format: <integer> - A nonzero value instructs the kernel to panic when a + A value of 1 instructs the kernel to panic when a hung task is detected. The default value is controlled by the CONFIG_BOOTPARAM_HUNG_TASK_PANIC build-time option. The value selected by this boot parameter can diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c index 97eb0b552bf8..77b1b844b02b 100644 --- a/fs/proc/proc_sysctl.c +++ b/fs/proc/proc_sysctl.c @@ -1743,6 +1743,7 @@ struct sysctl_alias { */ static const struct sysctl_alias sysctl_aliases[] = { {"numa_zonelist_order", "vm.numa_zonelist_order" }, + {"hung_task_panic", "kernel.hung_task_panic" }, { } }; diff --git a/kernel/hung_task.c b/kernel/hung_task.c index 14a625c16cb3..b22b5eeab3cb 100644 --- a/kernel/hung_task.c +++ b/kernel/hung_task.c @@ -63,16 +63,6 @@ static struct task_struct *watchdog_task; unsigned int __read_mostly sysctl_hung_task_panic = CONFIG_BOOTPARAM_HUNG_TASK_PANIC_VALUE; -static int __init hung_task_panic_setup(char *str) -{ - int rc = kstrtouint(str, 0, &sysctl_hung_task_panic); - - if (rc) - return rc; - return 1; -} -__setup("hung_task_panic=", hung_task_panic_setup); - static int hung_task_panic(struct notifier_block *this, unsigned long event, void *ptr) {
We can now handle sysctl parameters on kernel command line and have infrastructure to convert legacy command line options that duplicate sysctl to become a sysctl alias. This patch converts the hung_task_panic parameter. Note that the sysctl handler is more strict and allows only 0 and 1, while the legacy parameter allowed any non-zero value. But there is little reason anyone would not be using 1. Signed-off-by: Vlastimil Babka <vbabka@suse.cz> --- Documentation/admin-guide/kernel-parameters.txt | 2 +- fs/proc/proc_sysctl.c | 1 + kernel/hung_task.c | 10 ---------- 3 files changed, 2 insertions(+), 11 deletions(-)