Messages in this thread Patch in this message | | | Date | Sat, 13 Dec 2014 21:51:02 +0100 | From | Manfred Spraul <> | Subject | Re: [PATCH] kernel: sysctl: use 'unsigned long' type for 'zero' variable |
| |
Hi,
On 12/04/2014 12:25 AM, Andrew Morton wrote: > On Wed, 03 Dec 2014 15:41:21 +0300 Andrey Ryabinin <a.ryabinin@samsung.com> wrote: > >> Use the 'unsigned long' type for 'zero' variable to fix this. >> Changing type to 'unsigned long' shouldn't affect any other users >> of this variable. >> >> Reported-by: Dmitry Vyukov <dvyukov@google.com> >> Fixes: ed4d4902ebdd ("mm, hugetlb: remove hugetlb_zero and hugetlb_infinity") >> Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com> >> --- >> kernel/sysctl.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/kernel/sysctl.c b/kernel/sysctl.c >> index 15f2511..45c45c9 100644 >> --- a/kernel/sysctl.c >> +++ b/kernel/sysctl.c >> @@ -120,7 +120,7 @@ static int sixty = 60; >> >> static int __maybe_unused neg_one = -1; >> >> -static int zero; >> +static unsigned long zero;
After some (useless) playing around (see the attached patch):
Using > .extra1=zero, for proc_doulongvec_minmax doesn't make any sense:
__do_proc_doulongvec_minmax() internally contains > if ((min && val < *min) || (max && val > *max)) > continue;
What about just deleting the offending .extra1=zero line? .extra1=NULL has the same effect as .extra1=&zero.
-- Manfred
From 194e5d4758bb30531bad0907f06f3518002cd8b4 Mon Sep 17 00:00:00 2001 From: Manfred Spraul <manfred@colorfullife.com> Date: Sat, 13 Dec 2014 21:25:27 +0100 Subject: [PATCH] kernel/sysctl.c: Type safe macros
struct ctl_table is used for creating entries in e.g. /proc/sys/kernel.
The structure contains three void * entries and a function pointer, thus there is the risk of incorrectly mixing types.
The patch create a macro that prevents accidential mixing, it enforces that the type expected by the function pointer and the three void * are all of the same type.
Notes: - From my first impression, most proc entries mix types, and it works, because (int)1 and (unsigned int)1 are identical. Thus I'm not sure if this is the right approach.
- the code doesn't compile, it contains an intentional incompatible type
What do you think?
-- Manfred --- kernel/sysctl.c | 113 ++++++++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 97 insertions(+), 16 deletions(-)
diff --git a/kernel/sysctl.c b/kernel/sysctl.c index 15f2511..bc446a6 100644 --- a/kernel/sysctl.c +++ b/kernel/sysctl.c @@ -276,6 +276,101 @@ static int min_extfrag_threshold; static int max_extfrag_threshold = 1000; #endif +/* + * Type safe macros for creating the sysctl table entries: + * + * TODO: + * - 1) It works. + * I.e. it complains when _dointvec is used to assign + * to an unsigned int. Unfortunately, this is very common: + * * _minmax is used, with min=0 and e.g. max=int_max. + * * the variable is actually used as a boolean, thus it doesn't matter + * if the user space interface returns -1 or 0xffffffff. + * * unsigned int zero is used as min + * * ... + * Thus most error messages would be false positives. + * + * - 2) The error message is difficult to understand + * error: initializer element is not constant + */ + +#define ASSIGN_TYPE_SAFE(param, type) \ + __builtin_choose_expr(__builtin_types_compatible_p(typeof(param), type), \ + param, \ + /* The void expression results in a compile-time error \ + when assigning the result to something. */ \ + ((void)0) \ + ) + +#define SYSCTL_BUILD_INT_ENTRY_MINMAX(name, entry, min_ptr, max_ptr) \ + { \ + .procname = (name), \ + .data = ASSIGN_TYPE_SAFE(&(entry), int *), \ + .maxlen = sizeof( (entry) ), \ + .mode = 0644, \ + .proc_handler = proc_dointvec_minmax, \ + .extra1 = ASSIGN_TYPE_SAFE( (min_ptr), int *), \ + .extra2 = ASSIGN_TYPE_SAFE( (max_ptr), int *) \ + } + +#define SYSCTL_BUILD_ULONG_ENTRY_MINMAX(name, entry, min_ptr, max_ptr) \ + { \ + .procname = (name), \ + .data = ASSIGN_TYPE_SAFE(&(entry), unsigned long *), \ + .maxlen = sizeof( (entry) ), \ + .mode = 0644, \ + .proc_handler = proc_doulongvec_minmax, \ + .extra1 = ASSIGN_TYPE_SAFE( (min_ptr), unsigned long *), \ + .extra2 = ASSIGN_TYPE_SAFE( (max_ptr), unsigned long *) \ + } + +#define SYSCTL_BUILD_ENTRY_MINMAX(name, entry, min_ptr, max_ptr) \ + { \ + .procname = (name), \ + .data = &(entry), \ + .maxlen = sizeof( (entry) ), \ + .mode = 0644, \ + .proc_handler = __builtin_choose_expr(__builtin_types_compatible_p(typeof( &(entry)), int *), \ + proc_dointvec_minmax, \ + __builtin_choose_expr(__builtin_types_compatible_p(typeof( &(entry)), unsigned long *), \ + proc_doulongvec_minmax, \ + ((void)0) ) ), \ + .extra1 = ASSIGN_TYPE_SAFE( (min_ptr), typeof( &(entry) )), \ + .extra2 = ASSIGN_TYPE_SAFE( (max_ptr), typeof( &(entry) )) \ + } + +#define SYSCTL_BUILD_INT_ENTRY(name, entry) \ + { \ + .procname = (name), \ + .data = ASSIGN_TYPE_SAFE( &(entry), int *), \ + .maxlen = sizeof( (entry) ), \ + .mode = 0644, \ + .proc_handler = proc_dointvec, \ + } + +#define SYSCTL_BUILD_ULONG_ENTRY(name, entry) \ + { \ + .procname = (name), \ + .data = ASSIGN_TYPE_SAFE( &(entry), unsigned long *), \ + .maxlen = sizeof( (entry) ), \ + .mode = 0644, \ + .proc_handler = proc_doulongvec, \ + } + +#define SYSCTL_BUILD_ENTRY(name, entry) \ + { \ + .procname = (name), \ + .data = &(entry), \ + .maxlen = sizeof( (entry) ), \ + .mode = 0644, \ + .proc_handler = __builtin_choose_expr(__builtin_types_compatible_p(typeof( &(entry)), int *), \ + proc_dointvec, \ + __builtin_choose_expr(__builtin_types_compatible_p(typeof( &(entry)), unsigned long *), \ + proc_doulongvec, \ + ((void)0) ) ) \ + } + + static struct ctl_table kern_table[] = { { .procname = "sched_child_runs_first", @@ -1344,22 +1439,8 @@ static struct ctl_table vm_table[] = { .mode = 0644, .proc_handler = proc_dointvec_jiffies, }, - { - .procname = "block_dump", - .data = &block_dump, - .maxlen = sizeof(block_dump), - .mode = 0644, - .proc_handler = proc_dointvec, - .extra1 = &zero, - }, - { - .procname = "vfs_cache_pressure", - .data = &sysctl_vfs_cache_pressure, - .maxlen = sizeof(sysctl_vfs_cache_pressure), - .mode = 0644, - .proc_handler = proc_dointvec, - .extra1 = &zero, - }, + SYSCTL_BUILD_ENTRY_MINMAX("block_dump", block_dump, &zero, (unsigned int *)NULL), + SYSCTL_BUILD_ENTRY_MINMAX("vfs_cache_pressure", sysctl_vfs_cache_pressure, &zero, (int *)NULL), #ifdef HAVE_ARCH_PICK_MMAP_LAYOUT { .procname = "legacy_va_layout", -- 2.1.0
| |