diff mbox series

[v4,2/4] sysctl: Move some boundary constants form sysctl.c to sysctl_vals

Message ID 1589859071-25898-3-git-send-email-nixiaoming@huawei.com (mailing list archive)
State New, archived
Headers show
Series cleaning up the sysctls table (hung_task watchdog) | expand

Commit Message

Xiaoming Ni May 19, 2020, 3:31 a.m. UTC 
Some boundary (.extra1 .extra2) constants (E.g: neg_one two) in
sysctl.c are used in multiple features. Move these variables to
sysctl_vals to avoid adding duplicate variables when cleaning up
sysctls table.

Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
---
 fs/proc/proc_sysctl.c  |  2 +-
 include/linux/sysctl.h | 11 ++++++++---
 kernel/sysctl.c        | 39 +++++++++++++++++----------------------
 3 files changed, 26 insertions(+), 26 deletions(-)

Comments

Tetsuo Handa May 19, 2020, 4:44 a.m. UTC | #1 
On 2020/05/19 12:31, Xiaoming Ni wrote:
> Some boundary (.extra1 .extra2) constants (E.g: neg_one two) in
> sysctl.c are used in multiple features. Move these variables to
> sysctl_vals to avoid adding duplicate variables when cleaning up
> sysctls table.
> 
> Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
> Reviewed-by: Kees Cook <keescook@chromium.org>

I feel that it is use of

	void *extra1;
	void *extra2;

in "struct ctl_table" that requires constant values indirection.
Can't we get rid of sysctl_vals using some "union" like below?

struct ctl_table {
	const char *procname;           /* Text ID for /proc/sys, or zero */
	void *data;
	int maxlen;
	umode_t mode;
	struct ctl_table *child;        /* Deprecated */
	proc_handler *proc_handler;     /* Callback for text formatting */
	struct ctl_table_poll *poll;
	union {
		void *min_max_ptr[2];
		int min_max_int[2];
		long min_max_long[2];
	};
} __randomize_layout;
Xiaoming Ni May 20, 2020, 1:14 a.m. UTC | #2 
On 2020/5/19 12:44, Tetsuo Handa wrote:
> On 2020/05/19 12:31, Xiaoming Ni wrote:
>> Some boundary (.extra1 .extra2) constants (E.g: neg_one two) in
>> sysctl.c are used in multiple features. Move these variables to
>> sysctl_vals to avoid adding duplicate variables when cleaning up
>> sysctls table.
>>
>> Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
>> Reviewed-by: Kees Cook <keescook@chromium.org>
> 
> I feel that it is use of
> 
> 	void *extra1;
> 	void *extra2;
> 
> in "struct ctl_table" that requires constant values indirection.
> Can't we get rid of sysctl_vals using some "union" like below?
> 
> struct ctl_table {
> 	const char *procname;           /* Text ID for /proc/sys, or zero */
> 	void *data;
> 	int maxlen;
> 	umode_t mode;
> 	struct ctl_table *child;        /* Deprecated */
> 	proc_handler *proc_handler;     /* Callback for text formatting */
> 	struct ctl_table_poll *poll;
> 	union {
> 		void *min_max_ptr[2];
> 		int min_max_int[2];
> 		long min_max_long[2];
> 	};
> } __randomize_layout;
> 
> .
> 

net/decnet/dn_dev.c:
static void dn_dev_sysctl_register(struct net_device *dev, struct 
dn_dev_parms *parms)
{
	struct dn_dev_sysctl_table *t;
	int i;

	char path[sizeof("net/decnet/conf/") + IFNAMSIZ];

	t = kmemdup(&dn_dev_sysctl, sizeof(*t), GFP_KERNEL);
	if (t == NULL)
		return;

	for(i = 0; i < ARRAY_SIZE(t->dn_dev_vars) - 1; i++) {
		long offset = (long)t->dn_dev_vars[i].data;
		t->dn_dev_vars[i].data = ((char *)parms) + offset;
	}

	snprintf(path, sizeof(path), "net/decnet/conf/%s",
		dev? dev->name : parms->name);

	t->dn_dev_vars[0].extra1 = (void *)dev;

	t->sysctl_header = register_net_sysctl(&init_net, path, t->dn_dev_vars);
	if (t->sysctl_header == NULL)
		kfree(t);
	else
		parms->sysctl = t;
}

A small amount of code is not used as a boundary value when using 
extra1. This scenario may not be suitable for renaming to min_max_ptr.

Should we add const to extra1 extra2 ?

--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -124,8 +124,8 @@ 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;
+       const void *extra1;
+       const void *extra2;
  } __randomize_layout;


Thanks
Xiaoming Ni
Luis Chamberlain May 20, 2020, 1:39 a.m. UTC | #3 
On Wed, May 20, 2020 at 09:14:08AM +0800, Xiaoming Ni wrote:
> On 2020/5/19 12:44, Tetsuo Handa wrote:
> > On 2020/05/19 12:31, Xiaoming Ni wrote:
> > > Some boundary (.extra1 .extra2) constants (E.g: neg_one two) in
> > > sysctl.c are used in multiple features. Move these variables to
> > > sysctl_vals to avoid adding duplicate variables when cleaning up
> > > sysctls table.
> > > 
> > > Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
> > > Reviewed-by: Kees Cook <keescook@chromium.org>
> > 
> > I feel that it is use of
> > 
> > 	void *extra1;
> > 	void *extra2;
> > 
> > in "struct ctl_table" that requires constant values indirection.
> > Can't we get rid of sysctl_vals using some "union" like below?
> > 
> > struct ctl_table {
> > 	const char *procname;           /* Text ID for /proc/sys, or zero */
> > 	void *data;
> > 	int maxlen;
> > 	umode_t mode;
> > 	struct ctl_table *child;        /* Deprecated */
> > 	proc_handler *proc_handler;     /* Callback for text formatting */
> > 	struct ctl_table_poll *poll;
> > 	union {
> > 		void *min_max_ptr[2];
> > 		int min_max_int[2];
> > 		long min_max_long[2];
> > 	};
> > } __randomize_layout;
> > 
> > .
> > 
> 
> net/decnet/dn_dev.c:
> static void dn_dev_sysctl_register(struct net_device *dev, struct
> dn_dev_parms *parms)
> {
> 	struct dn_dev_sysctl_table *t;
> 	int i;
> 
> 	char path[sizeof("net/decnet/conf/") + IFNAMSIZ];
> 
> 	t = kmemdup(&dn_dev_sysctl, sizeof(*t), GFP_KERNEL);
> 	if (t == NULL)
> 		return;
> 
> 	for(i = 0; i < ARRAY_SIZE(t->dn_dev_vars) - 1; i++) {
> 		long offset = (long)t->dn_dev_vars[i].data;
> 		t->dn_dev_vars[i].data = ((char *)parms) + offset;
> 	}
> 
> 	snprintf(path, sizeof(path), "net/decnet/conf/%s",
> 		dev? dev->name : parms->name);
> 
> 	t->dn_dev_vars[0].extra1 = (void *)dev;
> 
> 	t->sysctl_header = register_net_sysctl(&init_net, path, t->dn_dev_vars);
> 	if (t->sysctl_header == NULL)
> 		kfree(t);
> 	else
> 		parms->sysctl = t;
> }
> 
> A small amount of code is not used as a boundary value when using extra1.
> This scenario may not be suitable for renaming to min_max_ptr.
> 
> Should we add const to extra1 extra2 ?
> 
> --- a/include/linux/sysctl.h
> +++ b/include/linux/sysctl.h
> @@ -124,8 +124,8 @@ 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;
> +       const void *extra1;
> +       const void *extra2;
>  } __randomize_layout;

Do that, compile an allyesconfig and it'll fail, but if you fix the
callers so that they use a const, then yes. That would cover only your
architecture. It is unclear if we ever used non-const for this on purpose.

  Luis
diff mbox series

Patch

diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 5b405f3..3d65e7d 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -24,7 +24,7 @@ 
 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, 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 857ba93..97586ee 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -38,9 +38,14 @@ 
 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_ONE_THOUSAND	((void *)&sysctl_vals[6])
+#define SYSCTL_INT_MAX	((void *)&sysctl_vals[7])
 
 extern const int sysctl_vals[];
 
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 8afd713..38469bf 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -111,14 +111,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 one_thousand = 1000;
 #ifdef CONFIG_PRINTK
 static int ten_thousand = 10000;
 #endif
@@ -1885,7 +1880,7 @@  int proc_do_static_key(struct ctl_table *table, int write,
 		.maxlen		= sizeof(int),
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_minmax,
-		.extra1		= &neg_one,
+		.extra1		= SYSCTL_NEG_ONE,
 		.extra2		= SYSCTL_ONE,
 	},
 #endif
@@ -2227,7 +2222,7 @@  int proc_do_static_key(struct ctl_table *table, int write,
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_minmax_sysadmin,
 		.extra1		= SYSCTL_ZERO,
-		.extra2		= &two,
+		.extra2		= SYSCTL_TWO,
 	},
 #endif
 	{
@@ -2487,7 +2482,7 @@  int proc_do_static_key(struct ctl_table *table, int write,
 		.maxlen		= sizeof(int),
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_minmax,
-		.extra1		= &neg_one,
+		.extra1		= SYSCTL_NEG_ONE,
 	},
 #endif
 #ifdef CONFIG_RT_MUTEXES
@@ -2549,7 +2544,7 @@  int proc_do_static_key(struct ctl_table *table, int write,
 		.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",
@@ -2567,7 +2562,7 @@  int proc_do_static_key(struct ctl_table *table, int write,
 		.mode		= 0644,
 		.proc_handler	= perf_event_max_stack_handler,
 		.extra1		= SYSCTL_ZERO,
-		.extra2		= &one_thousand,
+		.extra2		= SYSCTL_ONE_THOUSAND,
 	},
 #endif
 	{
@@ -2642,7 +2637,7 @@  int proc_do_static_key(struct ctl_table *table, int write,
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_minmax,
 		.extra1		= SYSCTL_ZERO,
-		.extra2		= &two,
+		.extra2		= SYSCTL_TWO,
 	},
 	{
 		.procname	= "panic_on_oom",
@@ -2651,7 +2646,7 @@  int proc_do_static_key(struct ctl_table *table, int write,
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_minmax,
 		.extra1		= SYSCTL_ZERO,
-		.extra2		= &two,
+		.extra2		= SYSCTL_TWO,
 	},
 	{
 		.procname	= "oom_kill_allocating_task",
@@ -2696,7 +2691,7 @@  int proc_do_static_key(struct ctl_table *table, int write,
 		.mode		= 0644,
 		.proc_handler	= dirty_background_ratio_handler,
 		.extra1		= SYSCTL_ZERO,
-		.extra2		= &one_hundred,
+		.extra2		= SYSCTL_ONE_HUNDRED,
 	},
 	{
 		.procname	= "dirty_background_bytes",
@@ -2713,7 +2708,7 @@  int proc_do_static_key(struct ctl_table *table, int write,
 		.mode		= 0644,
 		.proc_handler	= dirty_ratio_handler,
 		.extra1		= SYSCTL_ZERO,
-		.extra2		= &one_hundred,
+		.extra2		= SYSCTL_ONE_HUNDRED,
 	},
 	{
 		.procname	= "dirty_bytes",
@@ -2753,7 +2748,7 @@  int proc_do_static_key(struct ctl_table *table, int write,
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_minmax,
 		.extra1		= SYSCTL_ZERO,
-		.extra2		= &one_hundred,
+		.extra2		= SYSCTL_ONE_HUNDRED,
 	},
 #ifdef CONFIG_HUGETLB_PAGE
 	{
@@ -2810,7 +2805,7 @@  int proc_do_static_key(struct ctl_table *table, int write,
 		.mode		= 0200,
 		.proc_handler	= drop_caches_sysctl_handler,
 		.extra1		= SYSCTL_ONE,
-		.extra2		= &four,
+		.extra2		= SYSCTL_FOUR,
 	},
 #ifdef CONFIG_COMPACTION
 	{
@@ -2863,7 +2858,7 @@  int proc_do_static_key(struct ctl_table *table, int write,
 		.mode		= 0644,
 		.proc_handler	= watermark_scale_factor_sysctl_handler,
 		.extra1		= SYSCTL_ONE,
-		.extra2		= &one_thousand,
+		.extra2		= SYSCTL_ONE_THOUSAND,
 	},
 	{
 		.procname	= "percpu_pagelist_fraction",
@@ -2942,7 +2937,7 @@  int proc_do_static_key(struct ctl_table *table, int write,
 		.mode		= 0644,
 		.proc_handler	= sysctl_min_unmapped_ratio_sysctl_handler,
 		.extra1		= SYSCTL_ZERO,
-		.extra2		= &one_hundred,
+		.extra2		= SYSCTL_ONE_HUNDRED,
 	},
 	{
 		.procname	= "min_slab_ratio",
@@ -2951,7 +2946,7 @@  int proc_do_static_key(struct ctl_table *table, int write,
 		.mode		= 0644,
 		.proc_handler	= sysctl_min_slab_ratio_sysctl_handler,
 		.extra1		= SYSCTL_ZERO,
-		.extra2		= &one_hundred,
+		.extra2		= SYSCTL_ONE_HUNDRED,
 	},
 #endif
 #ifdef CONFIG_SMP
@@ -3234,7 +3229,7 @@  int proc_do_static_key(struct ctl_table *table, int write,
 		.mode		= 0600,
 		.proc_handler	= proc_dointvec_minmax,
 		.extra1		= SYSCTL_ZERO,
-		.extra2		= &two,
+		.extra2		= SYSCTL_TWO,
 	},
 	{
 		.procname	= "protected_regular",
@@ -3243,7 +3238,7 @@  int proc_do_static_key(struct ctl_table *table, int write,
 		.mode		= 0600,
 		.proc_handler	= proc_dointvec_minmax,
 		.extra1		= SYSCTL_ZERO,
-		.extra2		= &two,
+		.extra2		= SYSCTL_TWO,
 	},
 	{
 		.procname	= "suid_dumpable",
@@ -3252,7 +3247,7 @@  int proc_do_static_key(struct ctl_table *table, int write,
 		.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)
 	{