| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <2f8363b3-781e-b065-82f4-f84e6e787fad@huawei.com>
Date: Thu, 14 May 2020 14:05:48 +0800
From: Xiaoming Ni <nixiaoming@...wei.com>
To: Luis Chamberlain <mcgrof@...nel.org>
CC: "Eric W. Biederman" <ebiederm@...ssion.com>,
Al Viro <viro@...IV.linux.org.uk>,
Kees Cook <keescook@...omium.org>,
"Stephen Rothwell" <sfr@...b.auug.org.au>,
Helge Deller <deller@....de>,
Parisc List <linux-parisc@...r.kernel.org>,
<yzaikin@...gle.com>, <linux-fsdevel@...r.kernel.org>,
Linux Next Mailing List <linux-next@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Christoph Hellwig <hch@....de>, <gregkh@...uxfoundation.org>
Subject: Re: linux-next: manual merge of the vfs tree with the parisc-hd tree
On 2020/5/13 20:50, Luis Chamberlain wrote:
> On Wed, May 13, 2020 at 12:04:02PM +0800, Xiaoming Ni wrote:
>> On 2020/5/13 6:03, Luis Chamberlain wrote:
>>> On Tue, May 12, 2020 at 12:40:55PM -0500, Eric W. Biederman wrote:
>>>> Luis Chamberlain <mcgrof@...nel.org> writes:
>>>>
>>>>> On Tue, May 12, 2020 at 06:52:35AM -0500, Eric W. Biederman wrote:
>>>>>> Luis Chamberlain <mcgrof@...nel.org> writes:
>>>>>>
>>>>>>> +static struct ctl_table fs_base_table[] = {
>>>>>>> + {
>>>>>>> + .procname = "fs",
>>>>>>> + .mode = 0555,
>>>>>>> + .child = fs_table,
>>>>>>> + },
>>>>>>> + { }
>>>>>>> +};
>>>>>> ^^^^^^^^^^^^^^^^^^^^^^^^ You don't need this at all.
>>>>>>>> +static int __init fs_procsys_init(void)
>>>>>>> +{
>>>>>>> + struct ctl_table_header *hdr;
>>>>>>> +
>>>>>>> + hdr = register_sysctl_table(fs_base_table);
>>>>>> ^^^^^^^^^^^^^^^^^^^^^ Please use register_sysctl instead.
>>>>>> AKA
>>>>>> hdr = register_sysctl("fs", fs_table);
>>>>>
>>>>> Ah, much cleaner thanks!
>>>>
>>>> It is my hope you we can get rid of register_sysctl_table one of these
>>>> days. It was the original interface but today it is just a
>>>> compatibility wrapper.
>>>>
>>>> I unfortunately ran out of steam last time before I finished converting
>>>> everything over.
>>>
>>> Let's give it one more go. I'll start with the fs stuff.
>>>
>>> Luis
>>>
>>> .
>>>
>>
>> If we register each feature in its own feature code file using register() to
>> register the sysctl interface. To avoid merge conflicts when different
>> features modify sysctl.c at the same time.
>> that is, try to Avoid mixing code with multiple features in the same code
>> file.
>>
>> For example, the multiple file interfaces defined in sysctl.c by the
>> hung_task feature can be moved to hung_task.c.
>>
>> Perhaps later, without centralized sysctl.c ?
>> Is this better?
>>
>> Thanks
>> Xiaoming Ni
>>
>> ---
>> include/linux/sched/sysctl.h | 8 +----
>> kernel/hung_task.c | 78
>> +++++++++++++++++++++++++++++++++++++++++++-
>> kernel/sysctl.c | 50 ----------------------------
>> 3 files changed, 78 insertions(+), 58 deletions(-)
>>
>> diff --git a/include/linux/sched/sysctl.h b/include/linux/sched/sysctl.h
>> index d4f6215..bb4e0d3 100644
>> --- a/include/linux/sched/sysctl.h
>> +++ b/include/linux/sched/sysctl.h
>> @@ -7,14 +7,8 @@
>> struct ctl_table;
>>
>> #ifdef CONFIG_DETECT_HUNG_TASK
>> -extern int sysctl_hung_task_check_count;
>> -extern unsigned int sysctl_hung_task_panic;
>> +/* used for block/ */
>> extern unsigned long sysctl_hung_task_timeout_secs;
>> -extern unsigned long sysctl_hung_task_check_interval_secs;
>> -extern int sysctl_hung_task_warnings;
>> -extern int proc_dohung_task_timeout_secs(struct ctl_table *table, int
>> write,
>> - void __user *buffer,
>> - size_t *lenp, loff_t *ppos);
>> #else
>> /* Avoid need for ifdefs elsewhere in the code */
>> enum { sysctl_hung_task_timeout_secs = 0 };
>> diff --git a/kernel/hung_task.c b/kernel/hung_task.c
>> index 14a625c..53589f2 100644
>> --- a/kernel/hung_task.c
>> +++ b/kernel/hung_task.c
>> @@ -20,10 +20,10 @@
>> #include <linux/utsname.h>
>> #include <linux/sched/signal.h>
>> #include <linux/sched/debug.h>
>> +#include <linux/kmemleak.h>
>> #include <linux/sched/sysctl.h>
>>
>> #include <trace/events/sched.h>
>> -
>> /*
>> * The number of tasks checked:
>> */
>> @@ -296,8 +296,84 @@ static int watchdog(void *dummy)
>> return 0;
>> }
>>
>> +/*
>> + * This is needed for proc_doulongvec_minmax of
>> sysctl_hung_task_timeout_secs
>> + * and hung_task_check_interval_secs
>> + */
>> +static unsigned long hung_task_timeout_max = (LONG_MAX / HZ);
>
> This is not generic so it can stay in this file.
>
>> +static int __maybe_unused neg_one = -1;
>
> This is generic so we can share it, I suggest we just rename this
> for now to sysctl_neg_one, export it to a symbol namespace,
> EXPORT_SYMBOL_NS_GPL(sysctl_neg_one, SYSCTL) and then import it with
> MODULE_IMPORT_NS(SYSCTL)
>
>
>> +static struct ctl_table hung_task_sysctls[] = {
>
> We want to wrap this around with CONFIG_SYSCTL, so a cleaner solution
> is something like this:
>
> diff --git a/kernel/Makefile b/kernel/Makefile
> index a42ac3a58994..689718351754 100644
> --- a/kernel/Makefile
> +++ b/kernel/Makefile
> @@ -88,7 +88,9 @@ obj-$(CONFIG_KCOV) += kcov.o
> obj-$(CONFIG_KPROBES) += kprobes.o
> obj-$(CONFIG_FAIL_FUNCTION) += fail_function.o
> obj-$(CONFIG_KGDB) += debug/
> -obj-$(CONFIG_DETECT_HUNG_TASK) += hung_task.o
> +obj-$(CONFIG_DETECT_HUNG_TASK) += hung_tasks.o
> +hung_tasks-y := hung_task.o
> +hung_tasks-$(CONFIG_SYSCTL) += hung_task_sysctl.o
> obj-$(CONFIG_LOCKUP_DETECTOR) += watchdog.o
> obj-$(CONFIG_HARDLOCKUP_DETECTOR_PERF) += watchdog_hld.o
> obj-$(CONFIG_SECCOMP) += seccomp.o
>
>> +/* get /proc/sys/kernel root */
>> +static struct ctl_table sysctls_root[] = {
>> + {
>> + .procname = "kernel",
>> + .mode = 0555,
>> + .child = hung_task_sysctls,
>> + },
>> + {}
>> +};
>> +
>
> And as per Eric, this is not needed, we can simplify this more, as noted
> below.
>
>> +static int __init hung_task_sysctl_init(void)
>> +{
>> + struct ctl_table_header *srt = register_sysctl_table(sysctls_root);
>
> You want instead something like::
>
> struct ctl_table_header *srt;
>
> srt = register_sysctl("kernel", hung_task_sysctls);
>> +
>> + if (!srt)
>> + return -ENOMEM;
>> + kmemleak_not_leak(srt);
>> + return 0;
>> +}
>> +
>
>> static int __init hung_task_init(void)
>> {
>> + int ret = hung_task_sysctl_init();
>> +
>> + if (ret != 0)
>> + return ret;
>> +
>
> And just #ifdef this around CONFIG_SYSCTL.
>
> Luis
>
> .
>
Thank you for your guidance, I will send the patch later
Xiaoming Ni
Powered by blists - more mailing lists