Re: [PATCH] kernel: sysctl: use 'unsigned long' type for 'zero' variable

[Manfred Spraul <manfred@colorfullife.com>]

[-- Attachment #1: Type: text/plain, Size: 1303 bytes --]

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




[-- Attachment #2: 0001-kernel-sysctl.c-Type-safe-macros.patch --]
[-- Type: text/x-patch, Size: 5277 bytes --]

>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