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

[Manfred Spraul <manfred@colorfullife.com>]

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

Hi Andrew,

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;
>>   static int __maybe_unused one = 1;
>>   static int __maybe_unused two = 2;
>>   static int __maybe_unused four = 4;
> Yeah, this is ghastly.
>
> Look at
>
> 	{
> 		.procname	= "numa_balancing",
> 		.data		= NULL, /* filled in by handler */
> 		.maxlen		= sizeof(unsigned int),
> 		.mode		= 0644,
> 		.proc_handler	= sysctl_numa_balancing,
> 		.extra1		= &zero,
> 		.extra2		= &one,
> 	},
>
> Now extra1 points at a long and extra2 points at an int.
> sysctl_numa_balancing() calls proc_dointvec_minmax() and I think your
> patch just broke big-endian 64-bit machines.  "sched_autogroup_enabled"
> breaks as well.
What about getting rid of "extra1" and "extra2" as well and replace it 
with "min" and "max"?
I've attached an idea

> and change proc_dointvec_minmax() and a million other functions to take
> `union sysctl_payload *' arguments.  But I haven't thought about it much.
Another idea: why do we pass "int *" instead of "int"?

With "int", we could use
     .int_min = 0;
     .int_max = 1;


--
     Manfred

[-- Attachment #2: 0001-PATCH-Allow-type-safe-documented-sysctl.patch --]
[-- Type: text/x-patch, Size: 4368 bytes --]

>From 7a210bec3d9dc3382ef0d6809a7742856373bbee Mon Sep 17 00:00:00 2001
From: Manfred Spraul <manfred@colorfullife.com>
Date: Thu, 4 Dec 2014 07:03:39 +0100
Subject: [PATCH] Allow type safe & documented sysctl

Idea from Andrew:
- add a union into struct ctl_table instead of the void *
- further idea: replace "extra1" and "extra2" with min/max
- use it for ipc

Notes:
- not tested
- not coding style reviewed
- open FIXME in ipc_sysctl.c

Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
---
 include/linux/sysctl.h | 16 ++++++++++++++--
 ipc/ipc_sysctl.c       | 34 ++++++++++++++++++----------------
 2 files changed, 32 insertions(+), 18 deletions(-)

diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index b7361f8..acc7581 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -111,8 +111,20 @@ struct ctl_table
 	struct ctl_table *child;	/* Deprecated */
 	proc_handler *proc_handler;	/* Callback for text formatting */
 	struct ctl_table_poll *poll;
-	void *extra1;
-	void *extra2;
+	union {
+		void *extra1;
+		int *int_min;
+		long *long_min;
+		unsigned int *uint_min;
+		unsigned long *ulong_min;
+	};
+	union {
+		void *extra2;
+		int *int_max;
+		long *long_max;
+		unsigned int *uint_max;
+		unsigned long *ulong_max;
+	};
 };
 
 struct ctl_node {
diff --git a/ipc/ipc_sysctl.c b/ipc/ipc_sysctl.c
index c3f0326..50a6e1c 100644
--- a/ipc/ipc_sysctl.c
+++ b/ipc/ipc_sysctl.c
@@ -167,6 +167,7 @@ static struct ctl_table ipc_kern_table[] = {
 		.maxlen		= sizeof(init_ipc_ns.shm_ctlmax),
 		.mode		= 0644,
 		.proc_handler	= proc_ipc_doulongvec_minmax,
+		/* FIXME: Why no ulong_min & ulong_max ?? */
 	},
 	{
 		.procname	= "shmall",
@@ -174,6 +175,7 @@ static struct ctl_table ipc_kern_table[] = {
 		.maxlen		= sizeof(init_ipc_ns.shm_ctlall),
 		.mode		= 0644,
 		.proc_handler	= proc_ipc_doulongvec_minmax,
+		/* FIXME: Why no ulong_min & ulong_max ?? */
 	},
 	{
 		.procname	= "shmmni",
@@ -188,8 +190,8 @@ static struct ctl_table ipc_kern_table[] = {
 		.maxlen		= sizeof(init_ipc_ns.shm_rmid_forced),
 		.mode		= 0644,
 		.proc_handler	= proc_ipc_dointvec_minmax_orphans,
-		.extra1		= &zero,
-		.extra2		= &one,
+		.int_min	= &zero,
+		.int_max	= &one,
 	},
 	{
 		.procname	= "msgmax",
@@ -197,8 +199,8 @@ static struct ctl_table ipc_kern_table[] = {
 		.maxlen		= sizeof(init_ipc_ns.msg_ctlmax),
 		.mode		= 0644,
 		.proc_handler	= proc_ipc_dointvec_minmax,
-		.extra1		= &zero,
-		.extra2		= &int_max,
+		.int_min	= &zero,
+		.int_max	= &int_max,
 	},
 	{
 		.procname	= "msgmni",
@@ -206,8 +208,8 @@ static struct ctl_table ipc_kern_table[] = {
 		.maxlen		= sizeof(init_ipc_ns.msg_ctlmni),
 		.mode		= 0644,
 		.proc_handler	= proc_ipc_callback_dointvec_minmax,
-		.extra1		= &zero,
-		.extra2		= &int_max,
+		.int_min	= &zero,
+		.int_max	= &int_max,
 	},
 	{
 		.procname	=  "msgmnb",
@@ -215,8 +217,8 @@ static struct ctl_table ipc_kern_table[] = {
 		.maxlen		= sizeof(init_ipc_ns.msg_ctlmnb),
 		.mode		= 0644,
 		.proc_handler	= proc_ipc_dointvec_minmax,
-		.extra1		= &zero,
-		.extra2		= &int_max,
+		.int_min	= &zero,
+		.int_max	= &int_max,
 	},
 	{
 		.procname	= "sem",
@@ -231,8 +233,8 @@ static struct ctl_table ipc_kern_table[] = {
 		.maxlen		= sizeof(int),
 		.mode		= 0644,
 		.proc_handler	= proc_ipcauto_dointvec_minmax,
-		.extra1		= &zero,
-		.extra2		= &one,
+		.int_min	= &zero,
+		.int_max	= &one,
 	},
 #ifdef CONFIG_CHECKPOINT_RESTORE
 	{
@@ -241,8 +243,8 @@ static struct ctl_table ipc_kern_table[] = {
 		.maxlen		= sizeof(init_ipc_ns.ids[IPC_SEM_IDS].next_id),
 		.mode		= 0644,
 		.proc_handler	= proc_ipc_dointvec_minmax,
-		.extra1		= &zero,
-		.extra2		= &int_max,
+		.int_min	= &zero,
+		.int_max	= &int_max,
 	},
 	{
 		.procname	= "msg_next_id",
@@ -250,8 +252,8 @@ static struct ctl_table ipc_kern_table[] = {
 		.maxlen		= sizeof(init_ipc_ns.ids[IPC_MSG_IDS].next_id),
 		.mode		= 0644,
 		.proc_handler	= proc_ipc_dointvec_minmax,
-		.extra1		= &zero,
-		.extra2		= &int_max,
+		.int_min	= &zero,
+		.int_max	= &int_max,
 	},
 	{
 		.procname	= "shm_next_id",
@@ -259,8 +261,8 @@ static struct ctl_table ipc_kern_table[] = {
 		.maxlen		= sizeof(init_ipc_ns.ids[IPC_SHM_IDS].next_id),
 		.mode		= 0644,
 		.proc_handler	= proc_ipc_dointvec_minmax,
-		.extra1		= &zero,
-		.extra2		= &int_max,
+		.int_min	= &zero,
+		.int_max	= &int_max,
 	},
 #endif
 	{}
-- 
1.9.3