Messages in this thread | | | Subject | Re: [PATCH v2 2/9] sysctl: Move some boundary constants from sysctl.c to sysctl_vals | From | Xiaoming Ni <> | Date | Wed, 24 Nov 2021 15:05:00 +0800 |
| |
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) >> { > . >
| |