From: Xiaoming Ni <nixiaoming@huawei.com>
To: "Eric W. Biederman" <ebiederm@xmission.com>,
Luis Chamberlain <mcgrof@kernel.org>
Cc: <akpm@linux-foundation.org>, <keescook@chromium.org>,
<yzaikin@google.com>, <peterz@infradead.org>,
<gregkh@linuxfoundation.org>, <pjt@google.com>,
<liu.hailong6@zte.com.cn>, <andriy.shevchenko@linux.intel.com>,
<sre@kernel.org>, <penguin-kernel@i-love.sakura.ne.jp>,
<pmladek@suse.com>, <senozhatsky@chromium.org>,
<wangqing@vivo.com>, <bcrl@kvack.org>, <viro@zeniv.linux.org.uk>,
<jack@suse.cz>, <amir73il@gmail.com>,
<linux-fsdevel@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 2/9] sysctl: Move some boundary constants from sysctl.c to sysctl_vals
Date: Wed, 24 Nov 2021 15:05:00 +0800 [thread overview]
Message-ID: <a2d657e4-617a-ff4b-1334-928560701589@huawei.com> (raw)
In-Reply-To: <87k0gygnq4.fsf@email.froward.int.ebiederm.org>
On 2021/11/24 12:51, Eric W. Biederman wrote:
> Luis Chamberlain <mcgrof@kernel.org> writes:
>
>> From: Xiaoming Ni <nixiaoming@huawei.com>
>>
>> sysctl has helpers which let us specify boundary values for a min or
>> max int value. Since these are used for a boundary check only they don't
>> change, so move these variables to sysctl_vals to avoid adding duplicate
>> variables. This will help with our cleanup of kernel/sysctl.c.
>
> Ouch.
>
> I kind of, sort of, have to protest.
>
> Where the macros that use sysctl_vals don't have a type they have caused
> mysterious code breakage because people did not realize they can not be
> used with sysctls that take a long instead of an int.
>
> This came up with when the internal storage for ucounts see
> kernel/ucount.c changed from an int to a long. We were quite a while
> tracking what was going on until we realized that the code could not use
> SYSCTL_ZERO and SYSCTL_INT_MAX and that we had to defined our own thatSYSCTL_ZERO and SYSCTL_ZERO involve dozens of files and are used in hundreds of places.
> were long.
>
static unsigned long zero_ul;
static unsigned long one_ul = 1;
static unsigned long long_max = LONG_MAX;
EXPORT_SYMBOL(proc_doulongvec_minmax);
Yes, min/max of type unsigned long is used in multiple sysctl
interfaces. It is necessary to add an unsigned long sysctl_val array to
avoid repeated definitions in different .c files.
> So before we extend something like this can we please change the
> macro naming convention so that it includes the size of the type
> we want.
>
The int type is the most widely used type. By default, numeric constants
are also of the int type. SYSCTL_ZERO and SYSCTL_ZERO involve dozens of
files and are used in hundreds of places. Whether only non-int macros
need to be named with their type size?
>
> I am also not a fan of sysctl_vals living in proc_sysctl. They
> have nothing to do with the code in that file. They would do much
> better in kernel/sysctl.c close to the helpers that use them.
>
yes
Thanks
Xiaoming Ni
> Eric
>
>
>> Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
>> Reviewed-by: Kees Cook <keescook@chromium.org>
>> [mcgrof: major rebase]
>> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
>> ---
>> fs/proc/proc_sysctl.c | 2 +-
>> include/linux/sysctl.h | 12 +++++++++---
>> kernel/sysctl.c | 44 ++++++++++++++++++------------------------
>> 3 files changed, 29 insertions(+), 29 deletions(-)
>>
>> diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
>> index b4950843d90a..6d462644bb00 100644
>> --- a/fs/proc/proc_sysctl.c
>> +++ b/fs/proc/proc_sysctl.c
>> @@ -26,7 +26,7 @@ static const struct file_operations proc_sys_dir_file_operations;
>> static const struct inode_operations proc_sys_dir_operations;
>>
>> /* shared constants to be used in various sysctls */
>> -const int sysctl_vals[] = { 0, 1, INT_MAX };
>> +const int sysctl_vals[] = { -1, 0, 1, 2, 4, 100, 200, 1000, INT_MAX };
>> EXPORT_SYMBOL(sysctl_vals);
>>
>> /* Support for permanently empty directories */
>> diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
>> index d3ab7969b6b5..718492057c70 100644
>> --- a/include/linux/sysctl.h
>> +++ b/include/linux/sysctl.h
>> @@ -38,9 +38,15 @@ struct ctl_table_header;
>> struct ctl_dir;
>>
>> /* Keep the same order as in fs/proc/proc_sysctl.c */
>> -#define SYSCTL_ZERO ((void *)&sysctl_vals[0])
>> -#define SYSCTL_ONE ((void *)&sysctl_vals[1])
>> -#define SYSCTL_INT_MAX ((void *)&sysctl_vals[2])
>> +#define SYSCTL_NEG_ONE ((void *)&sysctl_vals[0])
>> +#define SYSCTL_ZERO ((void *)&sysctl_vals[1])
>> +#define SYSCTL_ONE ((void *)&sysctl_vals[2])
>> +#define SYSCTL_TWO ((void *)&sysctl_vals[3])
>> +#define SYSCTL_FOUR ((void *)&sysctl_vals[4])
>> +#define SYSCTL_ONE_HUNDRED ((void *)&sysctl_vals[5])
>> +#define SYSCTL_TWO_HUNDRED ((void *)&sysctl_vals[6])
>> +#define SYSCTL_ONE_THOUSAND ((void *)&sysctl_vals[7])
>> +#define SYSCTL_INT_MAX ((void *)&sysctl_vals[8])
>>
>> extern const int sysctl_vals[];
>>
>> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
>> index 857c1ccad9e8..3097f0286504 100644
>> --- a/kernel/sysctl.c
>> +++ b/kernel/sysctl.c
>> @@ -113,15 +113,9 @@
>> static int sixty = 60;
>> #endif
>>
>> -static int __maybe_unused neg_one = -1;
>> -static int __maybe_unused two = 2;
>> -static int __maybe_unused four = 4;
>> static unsigned long zero_ul;
>> static unsigned long one_ul = 1;
>> static unsigned long long_max = LONG_MAX;
>> -static int one_hundred = 100;
>> -static int two_hundred = 200;
>> -static int one_thousand = 1000;
>> #ifdef CONFIG_PRINTK
>> static int ten_thousand = 10000;
>> #endif
>> @@ -1962,7 +1956,7 @@ static struct ctl_table kern_table[] = {
>> .maxlen = sizeof(int),
>> .mode = 0644,
>> .proc_handler = proc_dointvec_minmax,
>> - .extra1 = &neg_one,
>> + .extra1 = SYSCTL_NEG_ONE,
>> .extra2 = SYSCTL_ONE,
>> },
>> #endif
>> @@ -2304,7 +2298,7 @@ static struct ctl_table kern_table[] = {
>> .mode = 0644,
>> .proc_handler = proc_dointvec_minmax_sysadmin,
>> .extra1 = SYSCTL_ZERO,
>> - .extra2 = &two,
>> + .extra2 = SYSCTL_TWO,
>> },
>> #endif
>> {
>> @@ -2564,7 +2558,7 @@ static struct ctl_table kern_table[] = {
>> .maxlen = sizeof(int),
>> .mode = 0644,
>> .proc_handler = proc_dointvec_minmax,
>> - .extra1 = &neg_one,
>> + .extra1 = SYSCTL_NEG_ONE,
>> },
>> #endif
>> #ifdef CONFIG_RT_MUTEXES
>> @@ -2626,7 +2620,7 @@ static struct ctl_table kern_table[] = {
>> .mode = 0644,
>> .proc_handler = perf_cpu_time_max_percent_handler,
>> .extra1 = SYSCTL_ZERO,
>> - .extra2 = &one_hundred,
>> + .extra2 = SYSCTL_ONE_HUNDRED,
>> },
>> {
>> .procname = "perf_event_max_stack",
>> @@ -2644,7 +2638,7 @@ static struct ctl_table kern_table[] = {
>> .mode = 0644,
>> .proc_handler = perf_event_max_stack_handler,
>> .extra1 = SYSCTL_ZERO,
>> - .extra2 = &one_thousand,
>> + .extra2 = SYSCTL_ONE_THOUSAND,
>> },
>> #endif
>> {
>> @@ -2675,7 +2669,7 @@ static struct ctl_table kern_table[] = {
>> .mode = 0644,
>> .proc_handler = bpf_unpriv_handler,
>> .extra1 = SYSCTL_ZERO,
>> - .extra2 = &two,
>> + .extra2 = SYSCTL_TWO,
>> },
>> {
>> .procname = "bpf_stats_enabled",
>> @@ -2729,7 +2723,7 @@ static struct ctl_table vm_table[] = {
>> .mode = 0644,
>> .proc_handler = overcommit_policy_handler,
>> .extra1 = SYSCTL_ZERO,
>> - .extra2 = &two,
>> + .extra2 = SYSCTL_TWO,
>> },
>> {
>> .procname = "panic_on_oom",
>> @@ -2738,7 +2732,7 @@ static struct ctl_table vm_table[] = {
>> .mode = 0644,
>> .proc_handler = proc_dointvec_minmax,
>> .extra1 = SYSCTL_ZERO,
>> - .extra2 = &two,
>> + .extra2 = SYSCTL_TWO,
>> },
>> {
>> .procname = "oom_kill_allocating_task",
>> @@ -2783,7 +2777,7 @@ static struct ctl_table vm_table[] = {
>> .mode = 0644,
>> .proc_handler = dirty_background_ratio_handler,
>> .extra1 = SYSCTL_ZERO,
>> - .extra2 = &one_hundred,
>> + .extra2 = SYSCTL_ONE_HUNDRED,
>> },
>> {
>> .procname = "dirty_background_bytes",
>> @@ -2800,7 +2794,7 @@ static struct ctl_table vm_table[] = {
>> .mode = 0644,
>> .proc_handler = dirty_ratio_handler,
>> .extra1 = SYSCTL_ZERO,
>> - .extra2 = &one_hundred,
>> + .extra2 = SYSCTL_ONE_HUNDRED,
>> },
>> {
>> .procname = "dirty_bytes",
>> @@ -2840,7 +2834,7 @@ static struct ctl_table vm_table[] = {
>> .mode = 0644,
>> .proc_handler = proc_dointvec_minmax,
>> .extra1 = SYSCTL_ZERO,
>> - .extra2 = &two_hundred,
>> + .extra2 = SYSCTL_TWO_HUNDRED,
>> },
>> #ifdef CONFIG_HUGETLB_PAGE
>> {
>> @@ -2897,7 +2891,7 @@ static struct ctl_table vm_table[] = {
>> .mode = 0200,
>> .proc_handler = drop_caches_sysctl_handler,
>> .extra1 = SYSCTL_ONE,
>> - .extra2 = &four,
>> + .extra2 = SYSCTL_FOUR,
>> },
>> #ifdef CONFIG_COMPACTION
>> {
>> @@ -2914,7 +2908,7 @@ static struct ctl_table vm_table[] = {
>> .mode = 0644,
>> .proc_handler = compaction_proactiveness_sysctl_handler,
>> .extra1 = SYSCTL_ZERO,
>> - .extra2 = &one_hundred,
>> + .extra2 = SYSCTL_ONE_HUNDRED,
>> },
>> {
>> .procname = "extfrag_threshold",
>> @@ -2959,7 +2953,7 @@ static struct ctl_table vm_table[] = {
>> .mode = 0644,
>> .proc_handler = watermark_scale_factor_sysctl_handler,
>> .extra1 = SYSCTL_ONE,
>> - .extra2 = &one_thousand,
>> + .extra2 = SYSCTL_ONE_THOUSAND,
>> },
>> {
>> .procname = "percpu_pagelist_high_fraction",
>> @@ -3038,7 +3032,7 @@ static struct ctl_table vm_table[] = {
>> .mode = 0644,
>> .proc_handler = sysctl_min_unmapped_ratio_sysctl_handler,
>> .extra1 = SYSCTL_ZERO,
>> - .extra2 = &one_hundred,
>> + .extra2 = SYSCTL_ONE_HUNDRED,
>> },
>> {
>> .procname = "min_slab_ratio",
>> @@ -3047,7 +3041,7 @@ static struct ctl_table vm_table[] = {
>> .mode = 0644,
>> .proc_handler = sysctl_min_slab_ratio_sysctl_handler,
>> .extra1 = SYSCTL_ZERO,
>> - .extra2 = &one_hundred,
>> + .extra2 = SYSCTL_ONE_HUNDRED,
>> },
>> #endif
>> #ifdef CONFIG_SMP
>> @@ -3337,7 +3331,7 @@ static struct ctl_table fs_table[] = {
>> .mode = 0644,
>> .proc_handler = proc_dointvec_minmax,
>> .extra1 = SYSCTL_ZERO,
>> - .extra2 = &two,
>> + .extra2 = SYSCTL_TWO,
>> },
>> {
>> .procname = "protected_regular",
>> @@ -3346,7 +3340,7 @@ static struct ctl_table fs_table[] = {
>> .mode = 0644,
>> .proc_handler = proc_dointvec_minmax,
>> .extra1 = SYSCTL_ZERO,
>> - .extra2 = &two,
>> + .extra2 = SYSCTL_TWO,
>> },
>> {
>> .procname = "suid_dumpable",
>> @@ -3355,7 +3349,7 @@ static struct ctl_table fs_table[] = {
>> .mode = 0644,
>> .proc_handler = proc_dointvec_minmax_coredump,
>> .extra1 = SYSCTL_ZERO,
>> - .extra2 = &two,
>> + .extra2 = SYSCTL_TWO,
>> },
>> #if defined(CONFIG_BINFMT_MISC) || defined(CONFIG_BINFMT_MISC_MODULE)
>> {
> .
>
next prev parent reply other threads:[~2021-11-24 7:05 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-23 20:23 [PATCH v2 0/9] sysctl: first set of kernel/sysctl cleanups Luis Chamberlain
2021-11-23 20:23 ` [PATCH v2 1/9] sysctl: add a new register_sysctl_init() interface Luis Chamberlain
2021-11-25 16:14 ` Petr Mladek
2021-11-29 21:01 ` Luis Chamberlain
2021-11-23 20:23 ` [PATCH v2 2/9] sysctl: Move some boundary constants from sysctl.c to sysctl_vals Luis Chamberlain
2021-11-24 4:51 ` Eric W. Biederman
2021-11-24 7:05 ` Xiaoming Ni [this message]
2021-11-24 17:38 ` Eric W. Biederman
2021-11-24 23:12 ` Luis Chamberlain
2021-11-23 20:23 ` [PATCH v2 3/9] hung_task: Move hung_task sysctl interface to hung_task.c Luis Chamberlain
2021-11-26 9:46 ` Petr Mladek
2021-11-23 20:23 ` [PATCH v2 4/9] watchdog: move watchdog sysctl interface to watchdog.c Luis Chamberlain
2021-11-26 9:40 ` Petr Mladek
2021-11-23 20:23 ` [PATCH v2 5/9] sysctl: make ngroups_max const Luis Chamberlain
2021-11-23 20:23 ` [PATCH v2 6/9] sysctl: use const for typically used max/min proc sysctls Luis Chamberlain
2021-11-23 20:23 ` [PATCH v2 7/9] sysctl: use SYSCTL_ZERO to replace some static int zero uses Luis Chamberlain
2021-11-23 20:23 ` [PATCH v2 8/9] aio: move aio sysctl to aio.c Luis Chamberlain
2021-11-24 9:32 ` Jan Kara
2021-11-23 20:23 ` [PATCH v2 9/9] dnotify: move dnotify sysctl to dnotify.c Luis Chamberlain
2021-11-24 9:31 ` Jan Kara
2021-11-24 0:14 ` [PATCH v2 0/9] sysctl: first set of kernel/sysctl cleanups Andrew Morton
2021-11-24 0:27 ` Luis Chamberlain
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a2d657e4-617a-ff4b-1334-928560701589@huawei.com \
--to=nixiaoming@huawei.com \
--cc=akpm@linux-foundation.org \
--cc=amir73il@gmail.com \
--cc=andriy.shevchenko@linux.intel.com \
--cc=bcrl@kvack.org \
--cc=ebiederm@xmission.com \
--cc=gregkh@linuxfoundation.org \
--cc=jack@suse.cz \
--cc=keescook@chromium.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=liu.hailong6@zte.com.cn \
--cc=mcgrof@kernel.org \
--cc=penguin-kernel@i-love.sakura.ne.jp \
--cc=peterz@infradead.org \
--cc=pjt@google.com \
--cc=pmladek@suse.com \
--cc=senozhatsky@chromium.org \
--cc=sre@kernel.org \
--cc=viro@zeniv.linux.org.uk \
--cc=wangqing@vivo.com \
--cc=yzaikin@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®