[PATCH 1/8] sysctl: introduce sysctl SYSCTL_U8_MAX and SYSCTL_LONG_S32_MAX

[PATCH 1/8] sysctl: introduce sysctl SYSCTL_U8_MAX and SYSCTL_LONG_S32_MAX

[wenyang.linux]

From: Wen Yang <wenyang.linux@foxmail.com>

The boundary check of multiple modules uses these static variables (such as
two_five_five, n_65535, ue_int_max, etc), and they are also not changed.
Therefore, add them to the shared sysctl_vals and sysctl_long_vals to avoid
duplication.

Also rearranged sysctl_vals and sysctl_long_vals in numerical order.

Signed-off-by: Wen Yang <wenyang.linux@foxmail.com>
Cc: Luis Chamberlain <mcgrof@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Joel Granados <j.granados@samsung.com>
Cc: Christian Brauner <brauner@kernel.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: David Ahern <dsahern@kernel.org>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 include/linux/sysctl.h | 15 +++++++++------
 kernel/sysctl.c        |  4 ++--
 2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index ee7d33b89e9e..b7a13e4c411c 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -45,19 +45,22 @@ struct ctl_dir;
 #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_THREE_THOUSAND		((void *)&sysctl_vals[8])
-#define SYSCTL_INT_MAX			((void *)&sysctl_vals[9])
+#define SYSCTL_U8_MAX			((void *)&sysctl_vals[7])
+#define SYSCTL_ONE_THOUSAND		((void *)&sysctl_vals[8])
+#define SYSCTL_THREE_THOUSAND		((void *)&sysctl_vals[9])
+#define SYSCTL_U16_MAX			((void *)&sysctl_vals[10])
+#define SYSCTL_INT_MAX			((void *)&sysctl_vals[11])
+#define SYSCTL_NEG_ONE			((void *)&sysctl_vals[12])
 
 /* this is needed for the proc_dointvec_minmax for [fs_]overflow UID and GID */
-#define SYSCTL_MAXOLDUID		((void *)&sysctl_vals[10])
-#define SYSCTL_NEG_ONE			((void *)&sysctl_vals[11])
+#define SYSCTL_MAXOLDUID		SYSCTL_U16_MAX
 
 extern const int sysctl_vals[];
 
 #define SYSCTL_LONG_ZERO	((void *)&sysctl_long_vals[0])
 #define SYSCTL_LONG_ONE		((void *)&sysctl_long_vals[1])
-#define SYSCTL_LONG_MAX		((void *)&sysctl_long_vals[2])
+#define SYSCTL_LONG_S32_MAX	((void *)&sysctl_long_vals[2])
+#define SYSCTL_LONG_MAX		((void *)&sysctl_long_vals[3])
 
 extern const unsigned long sysctl_long_vals[];
 
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 157f7ce2942d..e1e00937cb29 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -82,10 +82,10 @@
 #endif
 
 /* shared constants to be used in various sysctls */
-const int sysctl_vals[] = { 0, 1, 2, 3, 4, 100, 200, 1000, 3000, INT_MAX, 65535, -1 };
+const int sysctl_vals[] = { 0, 1, 2, 3, 4, 100, 200, U8_MAX, 1000, 3000, 65535, INT_MAX, -1, };
 EXPORT_SYMBOL(sysctl_vals);
 
-const unsigned long sysctl_long_vals[] = { 0, 1, LONG_MAX };
+const unsigned long sysctl_long_vals[] = { 0, 1, S32_MAX, LONG_MAX, };
 EXPORT_SYMBOL_GPL(sysctl_long_vals);
 
 #if defined(CONFIG_SYSCTL)
-- 
2.25.1

[PATCH 2/8] rxrpc: delete these duplicate static variables n_65535 and four

[wenyang.linux]

From: Wen Yang <wenyang.linux@foxmail.com>

Since these static variables (n_65535 and four) are only used for
boundary checks and will not be changed, remove them and use the ones
in our shared const array.

Signed-off-by: Wen Yang <wenyang.linux@foxmail.com>
Cc: Luis Chamberlain <mcgrof@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Joel Granados <j.granados@samsung.com>
Cc: Christian Brauner <brauner@kernel.org>
Cc: David Howells <dhowells@redhat.com>
Cc: Marc Dionne <marc.dionne@auristor.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: linux-kernel@vger.kernel.org
---
 net/rxrpc/sysctl.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/net/rxrpc/sysctl.c b/net/rxrpc/sysctl.c
index ecaeb4ecfb58..4e2230cf6ff6 100644
--- a/net/rxrpc/sysctl.c
+++ b/net/rxrpc/sysctl.c
@@ -11,9 +11,7 @@
 #include "ar-internal.h"
 
 static struct ctl_table_header *rxrpc_sysctl_reg_table;
-static const unsigned int four = 4;
 static const unsigned int max_backlog = RXRPC_BACKLOG_MAX - 1;
-static const unsigned int n_65535 = 65535;
 static const unsigned int n_max_acks = 255;
 static const unsigned long one_jiffy = 1;
 static const unsigned long max_jiffies = MAX_JIFFY_OFFSET;
@@ -86,8 +84,8 @@ static struct ctl_table rxrpc_sysctl_table[] = {
 		.maxlen		= sizeof(unsigned int),
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_minmax,
-		.extra1		= (void *)SYSCTL_ONE,
-		.extra2		= (void *)&n_65535,
+		.extra1		= SYSCTL_ONE,
+		.extra2		= SYSCTL_U16_MAX,
 	},
 	{
 		.procname	= "max_backlog",
@@ -95,7 +93,7 @@ static struct ctl_table rxrpc_sysctl_table[] = {
 		.maxlen		= sizeof(unsigned int),
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_minmax,
-		.extra1		= (void *)&four,
+		.extra1		= SYSCTL_FOUR,
 		.extra2		= (void *)&max_backlog,
 	},
 	{
@@ -113,8 +111,8 @@ static struct ctl_table rxrpc_sysctl_table[] = {
 		.maxlen		= sizeof(unsigned int),
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_minmax,
-		.extra1		= (void *)SYSCTL_ONE,
-		.extra2		= (void *)&n_65535,
+		.extra1		= SYSCTL_ONE,
+		.extra2		= SYSCTL_U16_MAX,
 	},
 	{
 		.procname	= "rx_jumbo_max",
@@ -122,8 +120,8 @@ static struct ctl_table rxrpc_sysctl_table[] = {
 		.maxlen		= sizeof(unsigned int),
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_minmax,
-		.extra1		= (void *)SYSCTL_ONE,
-		.extra2		= (void *)&four,
+		.extra1		= SYSCTL_ONE,
+		.extra2		= SYSCTL_FOUR,
 	},
 	{ }
 };
-- 
2.25.1

[PATCH 3/8] net: ipv6: delete these duplicate static variables two_five_five and minus_one

[wenyang.linux]

From: Wen Yang <wenyang.linux@foxmail.com>

Since these static variables (two_five_five and minus_one) are only used
for boundary checks and will not be changed, remove them and use the ones
in our shared const array.

Signed-off-by: Wen Yang <wenyang.linux@foxmail.com>
Cc: Luis Chamberlain <mcgrof@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Joel Granados <j.granados@samsung.com>
Cc: Christian Brauner <brauner@kernel.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: David Ahern <dsahern@kernel.org>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 net/ipv6/addrconf.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index d63f5d063f07..1d55df7d34ed 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -6660,8 +6660,6 @@ static int addrconf_sysctl_disable_policy(struct ctl_table *ctl, int write,
 	return ret;
 }
 
-static int minus_one = -1;
-static const int two_five_five = 255;
 static u32 ioam6_if_id_max = U16_MAX;
 
 static const struct ctl_table addrconf_sysctl[] = {
@@ -6678,8 +6676,8 @@ static const struct ctl_table addrconf_sysctl[] = {
 		.maxlen		= sizeof(int),
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_minmax,
-		.extra1		= (void *)SYSCTL_ONE,
-		.extra2		= (void *)&two_five_five,
+		.extra1		= SYSCTL_ONE,
+		.extra2		= SYSCTL_U8_MAX,
 	},
 	{
 		.procname	= "mtu",
@@ -6722,7 +6720,7 @@ static const struct ctl_table addrconf_sysctl[] = {
 		.maxlen		= sizeof(int),
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_minmax,
-		.extra1		= &minus_one,
+		.extra1		= SYSCTL_NEG_ONE,
 	},
 	{
 		.procname	= "router_solicitation_interval",
@@ -7061,8 +7059,8 @@ static const struct ctl_table addrconf_sysctl[] = {
 		.maxlen		= sizeof(int),
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_minmax,
-		.extra1		= (void *)SYSCTL_ZERO,
-		.extra2		= (void *)&two_five_five,
+		.extra1		= SYSCTL_ZERO,
+		.extra2		= SYSCTL_U8_MAX,
 	},
 	{
 		.procname	= "rpl_seg_enabled",
-- 
2.25.1

[PATCH 4/8] svcrdma: delete the duplicate static variables zero

[wenyang.linux]

From: Wen Yang <wenyang.linux@foxmail.com>

Since these static variable zero is only used for boundary checks and
will not be changed, remove it and use the ones in our shared const array.

Signed-off-by: Wen Yang <wenyang.linux@foxmail.com>
Cc: Luis Chamberlain <mcgrof@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Joel Granados <j.granados@samsung.com>
Cc: Christian Brauner <brauner@kernel.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: David Ahern <dsahern@kernel.org>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 net/sunrpc/xprtrdma/svc_rdma.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/net/sunrpc/xprtrdma/svc_rdma.c b/net/sunrpc/xprtrdma/svc_rdma.c
index f86970733eb0..5e4cd17eeb5a 100644
--- a/net/sunrpc/xprtrdma/svc_rdma.c
+++ b/net/sunrpc/xprtrdma/svc_rdma.c
@@ -63,7 +63,6 @@ unsigned int svcrdma_max_req_size = RPCRDMA_DEF_INLINE_THRESH;
 static unsigned int min_max_inline = RPCRDMA_DEF_INLINE_THRESH;
 static unsigned int max_max_inline = RPCRDMA_MAX_INLINE_THRESH;
 static unsigned int svcrdma_stat_unused;
-static unsigned int zero;
 
 struct percpu_counter svcrdma_stat_read;
 struct percpu_counter svcrdma_stat_recv;
@@ -170,8 +169,8 @@ static struct ctl_table svcrdma_parm_table[] = {
 		.maxlen		= sizeof(unsigned int),
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_minmax,
-		.extra1		= &zero,
-		.extra2		= &zero,
+		.extra1		= SYSCTL_ZERO,
+		.extra2		= SYSCTL_ZERO,
 	},
 	{
 		.procname	= "rdma_stat_rq_poll",
@@ -179,8 +178,8 @@ static struct ctl_table svcrdma_parm_table[] = {
 		.maxlen		= sizeof(unsigned int),
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_minmax,
-		.extra1		= &zero,
-		.extra2		= &zero,
+		.extra1		= SYSCTL_ZERO,
+		.extra2		= SYSCTL_ZERO,
 	},
 	{
 		.procname	= "rdma_stat_rq_prod",
@@ -188,8 +187,8 @@ static struct ctl_table svcrdma_parm_table[] = {
 		.maxlen		= sizeof(unsigned int),
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_minmax,
-		.extra1		= &zero,
-		.extra2		= &zero,
+		.extra1		= SYSCTL_ZERO,
+		.extra2		= SYSCTL_ZERO,
 	},
 	{
 		.procname	= "rdma_stat_sq_poll",
@@ -197,8 +196,8 @@ static struct ctl_table svcrdma_parm_table[] = {
 		.maxlen		= sizeof(unsigned int),
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_minmax,
-		.extra1		= &zero,
-		.extra2		= &zero,
+		.extra1		= SYSCTL_ZERO,
+		.extra2		= SYSCTL_ZERO,
 	},
 	{
 		.procname	= "rdma_stat_sq_prod",
@@ -206,8 +205,8 @@ static struct ctl_table svcrdma_parm_table[] = {
 		.maxlen		= sizeof(unsigned int),
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_minmax,
-		.extra1		= &zero,
-		.extra2		= &zero,
+		.extra1		= SYSCTL_ZERO,
+		.extra2		= SYSCTL_ZERO,
 	},
 	{ },
 };
-- 
2.25.1

[PATCH 5/8] sysctl: delete these duplicate static variables i_zero and i_one_hundred

[wenyang.linux]

From: Wen Yang <wenyang.linux@foxmail.com>

Since these static variables (i_zero and i_one_hundred) are only used for
boundary checks and will not be changed, remove it and use the ones in
our shared const array.

Signed-off-by: Wen Yang <wenyang.linux@foxmail.com>
Cc: Luis Chamberlain <mcgrof@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Joel Granados <j.granados@samsung.com>
Cc: Christian Brauner <brauner@kernel.org>
Cc: linux-kernel@vger.kernel.org
---
 lib/test_sysctl.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/lib/test_sysctl.c b/lib/test_sysctl.c
index 9321d850931f..ee69dd277a1e 100644
--- a/lib/test_sysctl.c
+++ b/lib/test_sysctl.c
@@ -26,8 +26,6 @@
 #include <linux/delay.h>
 #include <linux/vmalloc.h>
 
-static int i_zero;
-static int i_one_hundred = 100;
 static int match_int_ok = 1;
 
 
@@ -78,8 +76,8 @@ static struct ctl_table test_table[] = {
 		.maxlen		= sizeof(int),
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_minmax,
-		.extra1		= &i_zero,
-		.extra2         = &i_one_hundred,
+		.extra1		= SYSCTL_ZERO,
+		.extra2         = SYSCTL_ONE_HUNDRED,
 	},
 	{
 		.procname	= "int_0002",
-- 
2.25.1

[PATCH 6/8] epoll: delete these duplicate static variables long_zero and long_max

[wenyang.linux]

From: Wen Yang <wenyang.linux@foxmail.com>

Since these static variables (long_zero and long_max) are only used for
boundary checks and will not be changed, remove it and use the ones in
our shared const array.

Signed-off-by: Wen Yang <wenyang.linux@foxmail.com>
Cc: Luis Chamberlain <mcgrof@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Joel Granados <j.granados@samsung.com>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Jan Kara <jack@suse.cz>
Cc: "Darrick J. Wong" <djwong@kernel.org>
Cc: linux-kernel@vger.kernel.org
---
 fs/eventpoll.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index 3534d36a1474..22864d6de04f 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -309,9 +309,6 @@ static void unlist_file(struct epitems_head *head)
 
 #include <linux/sysctl.h>
 
-static long long_zero;
-static long long_max = LONG_MAX;
-
 static struct ctl_table epoll_table[] = {
 	{
 		.procname	= "max_user_watches",
@@ -319,8 +316,8 @@ static struct ctl_table epoll_table[] = {
 		.maxlen		= sizeof(max_user_watches),
 		.mode		= 0644,
 		.proc_handler	= proc_doulongvec_minmax,
-		.extra1		= &long_zero,
-		.extra2		= &long_max,
+		.extra1		= SYSCTL_LONG_ZERO,
+		.extra2		= SYSCTL_LONG_MAX,
 	},
 };
 
-- 
2.25.1

[PATCH 7/8] fs: inotify: delete these duplicate static variables it_zero and it_int_max

[wenyang.linux]

From: Wen Yang <wenyang.linux@foxmail.com>

Since these static variables (it_zero and it_int_max) are only used for
boundary checks and will not be changed, remove it and use the ones in
our shared const array.

Signed-off-by: Wen Yang <wenyang.linux@foxmail.com>
Cc: Luis Chamberlain <mcgrof@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Joel Granados <j.granados@samsung.com>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Jan Kara <jack@suse.cz>
Cc: "Darrick J. Wong" <djwong@kernel.org>
Cc: linux-kernel@vger.kernel.org
---
 fs/notify/inotify/inotify_user.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index 85d8fdd55329..fd5c00ed8559 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -55,9 +55,6 @@ struct kmem_cache *inotify_inode_mark_cachep __ro_after_init;
 
 #include <linux/sysctl.h>
 
-static long it_zero = 0;
-static long it_int_max = INT_MAX;
-
 static struct ctl_table inotify_table[] = {
 	{
 		.procname	= "max_user_instances",
@@ -65,8 +62,8 @@ static struct ctl_table inotify_table[] = {
 		.maxlen		= sizeof(long),
 		.mode		= 0644,
 		.proc_handler	= proc_doulongvec_minmax,
-		.extra1		= &it_zero,
-		.extra2		= &it_int_max,
+		.extra1		= SYSCTL_LONG_ZERO,
+		.extra2		= SYSCTL_LONG_S32_MAX,
 	},
 	{
 		.procname	= "max_user_watches",
@@ -74,8 +71,8 @@ static struct ctl_table inotify_table[] = {
 		.maxlen		= sizeof(long),
 		.mode		= 0644,
 		.proc_handler	= proc_doulongvec_minmax,
-		.extra1		= &it_zero,
-		.extra2		= &it_int_max,
+		.extra1		= SYSCTL_LONG_ZERO,
+		.extra2		= SYSCTL_LONG_S32_MAX,
 	},
 	{
 		.procname	= "max_queued_events",
-- 
2.25.1

[PATCH 8/8] ucounts: delete these duplicate static variables ue_zero and ue_int_max

[wenyang.linux]

From: Wen Yang <wenyang.linux@foxmail.com>

Since these static variables (ue_zero and ue_int_max) are only used for
boundary checks and will not be changed, remove it and use the ones in
our shared const array.

Signed-off-by: Wen Yang <wenyang.linux@foxmail.com>
Cc: Luis Chamberlain <mcgrof@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Joel Granados <j.granados@samsung.com>
Cc: Christian Brauner <brauner@kernel.org>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Shuah Khan <skhan@linuxfoundation.org>
Cc: linux-kernel@vger.kernel.org
---
 kernel/ucount.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/kernel/ucount.c b/kernel/ucount.c
index 4aa6166cb856..05bbba02ae4f 100644
--- a/kernel/ucount.c
+++ b/kernel/ucount.c
@@ -58,17 +58,14 @@ static struct ctl_table_root set_root = {
 	.permissions = set_permissions,
 };
 
-static long ue_zero = 0;
-static long ue_int_max = INT_MAX;
-
 #define UCOUNT_ENTRY(name)					\
 	{							\
 		.procname	= name,				\
 		.maxlen		= sizeof(long),			\
 		.mode		= 0644,				\
 		.proc_handler	= proc_doulongvec_minmax,	\
-		.extra1		= &ue_zero,			\
-		.extra2		= &ue_int_max,			\
+		.extra1		= SYSCTL_LONG_ZERO,		\
+		.extra2		= SYSCTL_LONG_S32_MAX,		\
 	}
 static struct ctl_table user_table[] = {
 	UCOUNT_ENTRY("max_user_namespaces"),
-- 
2.25.1

Re: [PATCH 8/8] ucounts: delete these duplicate static variables ue_zero and ue_int_max

[Eric W. Biederman]

wenyang.linux@foxmail.com writes:

> From: Wen Yang <wenyang.linux@foxmail.com>
>
> Since these static variables (ue_zero and ue_int_max) are only used for
> boundary checks and will not be changed, remove it and use the ones in
> our shared const array.

What happened to the plans to kill the shared const array?

You can still save a lot more by turning .extra1 and .extra2
into longs instead of keeping them as pointers and needing
constants to be pointed at somewhere.

As I recall the last version of this actually broke the code,
(but not on little endian).

This one if the constants are properly named looks better
than that, but I don't see any reason why you want shared
constants for such a handful of things.  Especially when
it has proven to be error prone in the past.

The only people I can see who find a significant benefit by
consolidating all of the constants into one place are people who know
how to stomp kernel memory.

Eric


>
> Signed-off-by: Wen Yang <wenyang.linux@foxmail.com>
> Cc: Luis Chamberlain <mcgrof@kernel.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Joel Granados <j.granados@samsung.com>
> Cc: Christian Brauner <brauner@kernel.org>
> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> Cc: Shuah Khan <skhan@linuxfoundation.org>
> Cc: linux-kernel@vger.kernel.org
> ---
>  kernel/ucount.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/ucount.c b/kernel/ucount.c
> index 4aa6166cb856..05bbba02ae4f 100644
> --- a/kernel/ucount.c
> +++ b/kernel/ucount.c
> @@ -58,17 +58,14 @@ static struct ctl_table_root set_root = {
>  	.permissions = set_permissions,
>  };
>  
> -static long ue_zero = 0;
> -static long ue_int_max = INT_MAX;
> -
>  #define UCOUNT_ENTRY(name)					\
>  	{							\
>  		.procname	= name,				\
>  		.maxlen		= sizeof(long),			\
>  		.mode		= 0644,				\
>  		.proc_handler	= proc_doulongvec_minmax,	\
> -		.extra1		= &ue_zero,			\
> -		.extra2		= &ue_int_max,			\
> +		.extra1		= SYSCTL_LONG_ZERO,		\
> +		.extra2		= SYSCTL_LONG_S32_MAX,		\
>  	}
>  static struct ctl_table user_table[] = {
>  	UCOUNT_ENTRY("max_user_namespaces"),

Re: [PATCH 8/8] ucounts: delete these duplicate static variables ue_zero and ue_int_max

[Wen Yang]

On 2024/2/25 20:29, Eric W. Biederman wrote:
> wenyang.linux@foxmail.com writes:
> 
>> From: Wen Yang <wenyang.linux@foxmail.com>
>>
>> Since these static variables (ue_zero and ue_int_max) are only used for
>> boundary checks and will not be changed, remove it and use the ones in
>> our shared const array.
> 
> What happened to the plans to kill the shared const array?
> 
> You can still save a lot more by turning .extra1 and .extra2
> into longs instead of keeping them as pointers and needing
> constants to be pointed at somewhere.
> 
> As I recall the last version of this actually broke the code,
> (but not on little endian).
>
Thank you. While developing a driver recently, we accidentally 
discovered some redundant code related to extra1/extra2, so we tried to 
optimize it a bit.


Thank you for your comments. This plan (kill the shared const array) 
seems meaningful and should require some work. We are very willing to 
participate.

I am glad to receive your feedback. This plan (kill the shared const 
array) seems meaningful and should require a lot of work. We are very 
willing to participate.


> This one if the constants are properly named looks better
> than that, but I don't see any reason why you want shared
> constants for such a handful of things.  Especially when
> it has proven to be error prone in the past.
>


This patch series replaces multiple static variables (such as zero, 
two_five_five, n_65535, ue_int_max, etc) with some unified macros (such 
as SYSCTL_U8_ZERO, SYSCTL_U8_MAX, SYSCTL_U16_MAX, etc.).

Although according to the current implementation of sysctl, these macros 
are currently defined as pointers to the elements of this shared array, 
and they can also be easily switched from pointers to appropriate 
numbers when the shared array of sysctl is removed according to the 
above plan.

So the current patch series is also beneficial for subsequent 
optimization, that is, deleting this shared const array.


> The only people I can see who find a significant benefit by
> consolidating all of the constants into one place are people who know
> how to stomp kernel memory.
> 

As above, thanks.


--
Best wishes,
Wen

> 
> 
>>
>> Signed-off-by: Wen Yang <wenyang.linux@foxmail.com>
>> Cc: Luis Chamberlain <mcgrof@kernel.org>
>> Cc: Kees Cook <keescook@chromium.org>
>> Cc: Joel Granados <j.granados@samsung.com>
>> Cc: Christian Brauner <brauner@kernel.org>
>> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
>> Cc: Shuah Khan <skhan@linuxfoundation.org>
>> Cc: linux-kernel@vger.kernel.org
>> ---
>>   kernel/ucount.c | 7 ++-----
>>   1 file changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/kernel/ucount.c b/kernel/ucount.c
>> index 4aa6166cb856..05bbba02ae4f 100644
>> --- a/kernel/ucount.c
>> +++ b/kernel/ucount.c
>> @@ -58,17 +58,14 @@ static struct ctl_table_root set_root = {
>>   	.permissions = set_permissions,
>>   };
>>   
>> -static long ue_zero = 0;
>> -static long ue_int_max = INT_MAX;
>> -
>>   #define UCOUNT_ENTRY(name)					\
>>   	{							\
>>   		.procname	= name,				\
>>   		.maxlen		= sizeof(long),			\
>>   		.mode		= 0644,				\
>>   		.proc_handler	= proc_doulongvec_minmax,	\
>> -		.extra1		= &ue_zero,			\
>> -		.extra2		= &ue_int_max,			\
>> +		.extra1		= SYSCTL_LONG_ZERO,		\
>> +		.extra2		= SYSCTL_LONG_S32_MAX,		\
>>   	}
>>   static struct ctl_table user_table[] = {
>>   	UCOUNT_ENTRY("max_user_namespaces"),

Re: [PATCH 1/8] sysctl: introduce sysctl SYSCTL_U8_MAX and SYSCTL_LONG_S32_MAX

[Joel Granados]

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

On Sun, Feb 25, 2024 at 12:05:31PM +0800, wenyang.linux@foxmail.com wrote:
> From: Wen Yang <wenyang.linux@foxmail.com>
> 
> The boundary check of multiple modules uses these static variables (such as
> two_five_five, n_65535, ue_int_max, etc), and they are also not changed.
> Therefore, add them to the shared sysctl_vals and sysctl_long_vals to avoid
> duplication.
> 
> Also rearranged sysctl_vals and sysctl_long_vals in numerical order.
> 
> Signed-off-by: Wen Yang <wenyang.linux@foxmail.com>
> Cc: Luis Chamberlain <mcgrof@kernel.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Joel Granados <j.granados@samsung.com>
> Cc: Christian Brauner <brauner@kernel.org>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: David Ahern <dsahern@kernel.org>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: Paolo Abeni <pabeni@redhat.com>
> Cc: netdev@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  include/linux/sysctl.h | 15 +++++++++------
>  kernel/sysctl.c        |  4 ++--
>  2 files changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
> index ee7d33b89e9e..b7a13e4c411c 100644
> --- a/include/linux/sysctl.h
> +++ b/include/linux/sysctl.h
> @@ -45,19 +45,22 @@ struct ctl_dir;
>  #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_THREE_THOUSAND		((void *)&sysctl_vals[8])
> -#define SYSCTL_INT_MAX			((void *)&sysctl_vals[9])
> +#define SYSCTL_U8_MAX			((void *)&sysctl_vals[7])
> +#define SYSCTL_ONE_THOUSAND		((void *)&sysctl_vals[8])
> +#define SYSCTL_THREE_THOUSAND		((void *)&sysctl_vals[9])
> +#define SYSCTL_U16_MAX			((void *)&sysctl_vals[10])
> +#define SYSCTL_INT_MAX			((void *)&sysctl_vals[11])
> +#define SYSCTL_NEG_ONE			((void *)&sysctl_vals[12])
>  
>  /* this is needed for the proc_dointvec_minmax for [fs_]overflow UID and GID */
> -#define SYSCTL_MAXOLDUID		((void *)&sysctl_vals[10])
> -#define SYSCTL_NEG_ONE			((void *)&sysctl_vals[11])
> +#define SYSCTL_MAXOLDUID		SYSCTL_U16_MAX
>  
>  extern const int sysctl_vals[];
>  
>  #define SYSCTL_LONG_ZERO	((void *)&sysctl_long_vals[0])
>  #define SYSCTL_LONG_ONE		((void *)&sysctl_long_vals[1])
> -#define SYSCTL_LONG_MAX		((void *)&sysctl_long_vals[2])
> +#define SYSCTL_LONG_S32_MAX	((void *)&sysctl_long_vals[2])
> +#define SYSCTL_LONG_MAX		((void *)&sysctl_long_vals[3])
>  
>  extern const unsigned long sysctl_long_vals[];
>  
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 157f7ce2942d..e1e00937cb29 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -82,10 +82,10 @@
>  #endif
>  
>  /* shared constants to be used in various sysctls */
> -const int sysctl_vals[] = { 0, 1, 2, 3, 4, 100, 200, 1000, 3000, INT_MAX, 65535, -1 };
> +const int sysctl_vals[] = { 0, 1, 2, 3, 4, 100, 200, U8_MAX, 1000, 3000, 65535, INT_MAX, -1, };
Why do you insert the new values instead of just appending?
I figure that the patch would be much smaller if you appended.

>  EXPORT_SYMBOL(sysctl_vals);
>  
> -const unsigned long sysctl_long_vals[] = { 0, 1, LONG_MAX };
> +const unsigned long sysctl_long_vals[] = { 0, 1, S32_MAX, LONG_MAX, };
Same here. Why insert instead of append?
>  EXPORT_SYMBOL_GPL(sysctl_long_vals);
>  
>  #if defined(CONFIG_SYSCTL)
> -- 
> 2.25.1
> 

-- 

Joel Granados

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 1/8] sysctl: introduce sysctl SYSCTL_U8_MAX and SYSCTL_LONG_S32_MAX

[Wen Yang]

On 2024/2/26 04:16, Joel Granados wrote:
> On Sun, Feb 25, 2024 at 12:05:31PM +0800, wenyang.linux@foxmail.com wrote:
>> From: Wen Yang <wenyang.linux@foxmail.com>
>>
>> The boundary check of multiple modules uses these static variables (such as
>> two_five_five, n_65535, ue_int_max, etc), and they are also not changed.
>> Therefore, add them to the shared sysctl_vals and sysctl_long_vals to avoid
>> duplication.
>>
>> Also rearranged sysctl_vals and sysctl_long_vals in numerical order.
>>
>> Signed-off-by: Wen Yang <wenyang.linux@foxmail.com>
>> Cc: Luis Chamberlain <mcgrof@kernel.org>
>> Cc: Kees Cook <keescook@chromium.org>
>> Cc: Joel Granados <j.granados@samsung.com>
>> Cc: Christian Brauner <brauner@kernel.org>
>> Cc: "David S. Miller" <davem@davemloft.net>
>> Cc: David Ahern <dsahern@kernel.org>
>> Cc: Eric Dumazet <edumazet@google.com>
>> Cc: Jakub Kicinski <kuba@kernel.org>
>> Cc: Paolo Abeni <pabeni@redhat.com>
>> Cc: netdev@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> ---
>>   include/linux/sysctl.h | 15 +++++++++------
>>   kernel/sysctl.c        |  4 ++--
>>   2 files changed, 11 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
>> index ee7d33b89e9e..b7a13e4c411c 100644
>> --- a/include/linux/sysctl.h
>> +++ b/include/linux/sysctl.h
>> @@ -45,19 +45,22 @@ struct ctl_dir;
>>   #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_THREE_THOUSAND		((void *)&sysctl_vals[8])
>> -#define SYSCTL_INT_MAX			((void *)&sysctl_vals[9])
>> +#define SYSCTL_U8_MAX			((void *)&sysctl_vals[7])
>> +#define SYSCTL_ONE_THOUSAND		((void *)&sysctl_vals[8])
>> +#define SYSCTL_THREE_THOUSAND		((void *)&sysctl_vals[9])
>> +#define SYSCTL_U16_MAX			((void *)&sysctl_vals[10])
>> +#define SYSCTL_INT_MAX			((void *)&sysctl_vals[11])
>> +#define SYSCTL_NEG_ONE			((void *)&sysctl_vals[12])
>>   
>>   /* this is needed for the proc_dointvec_minmax for [fs_]overflow UID and GID */
>> -#define SYSCTL_MAXOLDUID		((void *)&sysctl_vals[10])
>> -#define SYSCTL_NEG_ONE			((void *)&sysctl_vals[11])
>> +#define SYSCTL_MAXOLDUID		SYSCTL_U16_MAX
>>   
>>   extern const int sysctl_vals[];
>>   
>>   #define SYSCTL_LONG_ZERO	((void *)&sysctl_long_vals[0])
>>   #define SYSCTL_LONG_ONE		((void *)&sysctl_long_vals[1])
>> -#define SYSCTL_LONG_MAX		((void *)&sysctl_long_vals[2])
>> +#define SYSCTL_LONG_S32_MAX	((void *)&sysctl_long_vals[2])
>> +#define SYSCTL_LONG_MAX		((void *)&sysctl_long_vals[3])
>>   
>>   extern const unsigned long sysctl_long_vals[];
>>   
>> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
>> index 157f7ce2942d..e1e00937cb29 100644
>> --- a/kernel/sysctl.c
>> +++ b/kernel/sysctl.c
>> @@ -82,10 +82,10 @@
>>   #endif
>>   
>>   /* shared constants to be used in various sysctls */
>> -const int sysctl_vals[] = { 0, 1, 2, 3, 4, 100, 200, 1000, 3000, INT_MAX, 65535, -1 };
>> +const int sysctl_vals[] = { 0, 1, 2, 3, 4, 100, 200, U8_MAX, 1000, 3000, 65535, INT_MAX, -1, };
> Why do you insert the new values instead of just appending?
> I figure that the patch would be much smaller if you appended.
> 


Thanks.
We originally intended to rearrange this array in numerical order, but 
now it seems unnecessary.
We will follow your suggestions and send v2 later.

--
Best wishes,
Wen


>>   EXPORT_SYMBOL(sysctl_vals);
>>   
>> -const unsigned long sysctl_long_vals[] = { 0, 1, LONG_MAX };
>> +const unsigned long sysctl_long_vals[] = { 0, 1, S32_MAX, LONG_MAX, };
> Same here. Why insert instead of append?
>>   EXPORT_SYMBOL_GPL(sysctl_long_vals);
>>   
>>   #if defined(CONFIG_SYSCTL)
>> -- 
>> 2.25.1
>>
> 

Re: [PATCH 8/8] ucounts: delete these duplicate static variables ue_zero and ue_int_max

[Joel Granados]

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

On Sun, Feb 25, 2024 at 06:29:33AM -0600, Eric W. Biederman wrote:
> wenyang.linux@foxmail.com writes:
> 
> > From: Wen Yang <wenyang.linux@foxmail.com>
> >
> > Since these static variables (ue_zero and ue_int_max) are only used for
> > boundary checks and will not be changed, remove it and use the ones in
> > our shared const array.
> 
> What happened to the plans to kill the shared const array?
I agree with removing sysctl_vals. I wanted to get more info but was not
able to find much in lore.kernel.org. I was wondering if the push
towards removing it had been discussed in the lists? Or was it more like
a conference hallway discussion?

Best
> 
> You can still save a lot more by turning .extra1 and .extra2
> into longs instead of keeping them as pointers and needing
> constants to be pointed at somewhere.
> 
> As I recall the last version of this actually broke the code,
> (but not on little endian).
> 
> This one if the constants are properly named looks better
> than that, but I don't see any reason why you want shared
> constants for such a handful of things.  Especially when
> it has proven to be error prone in the past.
> 
> The only people I can see who find a significant benefit by
> consolidating all of the constants into one place are people who know
> how to stomp kernel memory.
> 
> Eric
> 

-- 

Joel Granados

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]