Re: [PATCH v5] sysctl: simplify the min/max boundary check

[Wen Yang <wen.yang@linux.dev>]

On 12/19/25 22:44, Joel Granados wrote:
> Hey Wen
> 
> I know it has been a very long time, but I think I have a plan to make
> this happen. And by "this" I mean changing the extra{1,2} to another
> type. I'm writing to you to see if you are still up for pushing it
> forward.
> 
> My proposal would go in two big steps. Each one might take more than one
> release.
> 
> First step
> ==========
>    Consolidate the way ctl_table entries are created into a SYSCTL_ENTRY
>    macro. There would be no functional change on this first step. It is
>    just a preparation step for the second stage. The main objective is to
>    put all the logic of creating the ctl_table entry in one place (mainly
>    into sysctl.h). An example of what I mean for step one in [1]
> 
> Second step
> ===========
>    Actually change the extern{1,2} from *void to ulong. Having the logic
>    in one place makes this easier. Here I'm guessing that we would still
>    have to handle outliers (like net), but they would be much less.
> 
> In my opinion, if we are going to go through the pain of doing a
> treewide change, we might as well make it easier for ourselves to make
> systemic changes to sysctl in the future; we do this by having the
> creation macro within sysctl.{c,h}
> 
> Tell me if you are still interested and have cycles for interested
> 

Thank you very much.
It is my honor to participate in this program, and I am willing to join 
as soon as possible.

--
Best wishes,
Wen

> Best
> 
> [1]
> 
> diff --git i/include/linux/sysctl.h w/include/linux/sysctl.h
> index 39cf1bf9703f..716ad7e41a01 100644
> --- i/include/linux/sysctl.h
> +++ w/include/linux/sysctl.h
> @@ -58,6 +58,7 @@ 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])
> +extern const void *SYSCTL_NULL;
>   
>   /**
>    *
> @@ -230,6 +231,23 @@ struct ctl_table {
>   	void *extra2;
>   } __randomize_layout;
>   
> +#define __SYSCTL_ENTRY(NAME, DATA, TYPE, MODE, HANDLER, SMIN, SMAX)\
> +	{							\
> +		.procname	= NAME,				\
> +		.data		= (void*) &DATA,		\
> +		.maxlen		= sizeof(TYPE),			\
> +		.mode		= MODE,				\
> +		.proc_handler	= HANDLER,			\
> +		.extra1		= SMIN,				\
> +		.extra2		= SMAX,				\
> +	}
> +
> +#define SYSCTL_ENTRY(NAME, DATA, TYPE, MODE)			\
> +	__SYSCTL_ENTRY(NAME, DATA, TYPE, MODE, proc_do##TYPE##vec, NULL, NULL)
> +
> +#define SYSCTL_RANGE_ENTRY(NAME, DATA, TYPE, MODE, SMIN, SMAX)	\
> +	__SYSCTL_ENTRY(NAME, DATA, TYPE, MODE, proc_do##TYPE##vec_minmax, SMIN, SMAX)
> +
>   struct ctl_node {
>   	struct rb_node node;
>   	struct ctl_table_header *header;
> diff --git i/kernel/exit.c w/kernel/exit.c
> index 8a87021211ae..04e531e45912 100644
> --- i/kernel/exit.c
> +++ w/kernel/exit.c
> @@ -88,13 +88,7 @@ static unsigned int oops_limit = 10000;
>   
>   #ifdef CONFIG_SYSCTL
>   static const struct ctl_table kern_exit_table[] = {
> -	{
> -		.procname       = "oops_limit",
> -		.data           = &oops_limit,
> -		.maxlen         = sizeof(oops_limit),
> -		.mode           = 0644,
> -		.proc_handler   = proc_douintvec,
> -	},
> +	SYSCTL_ENTRY("oops_limit", oops_limit, uint, 0644),
>   };
>   
>   static __init int kernel_exit_sysctls_init(void)
> diff --git i/kernel/sysctl.c w/kernel/sysctl.c
> index 0965ea1212b4..fa228b89862a 100644
> --- i/kernel/sysctl.c
> +++ w/kernel/sysctl.c
> @@ -22,6 +22,7 @@
>   #include <linux/uaccess.h>
>   #include <asm/processor.h>
>   
> +const void *SYSCTL_NULL = NULL;
>   /* 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 };
>   EXPORT_SYMBOL(sysctl_vals);
> @@ -1328,47 +1329,16 @@ int proc_do_static_key(const struct ctl_table *table, int dir,
>   
>   static const struct ctl_table sysctl_subsys_table[] = {
>   #ifdef CONFIG_PROC_SYSCTL
> -	{
> -		.procname	= "sysctl_writes_strict",
> -		.data		= &sysctl_writes_strict,
> -		.maxlen		= sizeof(int),
> -		.mode		= 0644,
> -		.proc_handler	= proc_dointvec_minmax,
> -		.extra1		= SYSCTL_NEG_ONE,
> -		.extra2		= SYSCTL_ONE,
> -	},
> +	SYSCTL_RANGE_ENTRY("sysctl_writes_strict", sysctl_writes_strict, int, \
> +			   0644, SYSCTL_NEG_ONE, SYSCTL_ONE),
>   #endif
> -	{
> -		.procname	= "ngroups_max",
> -		.data		= (void *)&ngroups_max,
> -		.maxlen		= sizeof (int),
> -		.mode		= 0444,
> -		.proc_handler	= proc_dointvec,
> -	},
> -	{
> -		.procname	= "cap_last_cap",
> -		.data		= (void *)&cap_last_cap,
> -		.maxlen		= sizeof(int),
> -		.mode		= 0444,
> -		.proc_handler	= proc_dointvec,
> -	},
> +	SYSCTL_ENTRY("ngroups_max", ngroups_max, int, 0444),
> +	SYSCTL_ENTRY("cap_last_cap", cap_last_cap, int, 0444),
>   #ifdef CONFIG_SYSCTL_ARCH_UNALIGN_ALLOW
> -	{
> -		.procname	= "unaligned-trap",
> -		.data		= &unaligned_enabled,
> -		.maxlen		= sizeof(int),
> -		.mode		= 0644,
> -		.proc_handler	= proc_dointvec,
> -	},
> +	SYSCTL_ENTRY("unaligned-trap", unaligned_enabled, int, 0644),
>   #endif
>   #ifdef CONFIG_SYSCTL_ARCH_UNALIGN_NO_WARN
> -	{
> -		.procname	= "ignore-unaligned-usertrap",
> -		.data		= &no_unaligned_warning,
> -		.maxlen		= sizeof (int),
> -		.mode		= 0644,
> -		.proc_handler	= proc_dointvec,
> -	},
> +	SYSCTL_ENTRY("ignore-unaligned-usertrap", no_unaligned_warning, int, 0644),
>   #endif
>   };
>   
> diff --git i/kernel/ucount.c w/kernel/ucount.c
> index 586af49fc03e..b9f4fec7638a 100644
> --- i/kernel/ucount.c
> +++ w/kernel/ucount.c
> @@ -63,15 +63,9 @@ static struct ctl_table_root set_root = {
>   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,			\
> -	}
> +#define UCOUNT_ENTRY(name)	\
> +	SYSCTL_RANGE_ENTRY(name, SYSCTL_NULL, ulong, 0644, &ue_zero, &ue_int_max)
> +
>   static const struct ctl_table user_table[] = {
>   	UCOUNT_ENTRY("max_user_namespaces"),
>   	UCOUNT_ENTRY("max_pid_namespaces"),
> 
> On Thu, Feb 27, 2025 at 03:09:12PM +0100, Joel Granados wrote:
>> On Thu, Jan 30, 2025 at 10:32:14PM +0800, Wen Yang wrote:
>>>
>>>
>>> On 2025/1/28 01:51, Eric W. Biederman wrote:
>>>> Joel Granados <joel.granados@kernel.org> writes:
>>>>
>> ...
>>>> that use extra1 or extra2 for something besides min and max.  Then
>>>> remove extra1 and extra2.  At the end it is simpler and requires the
>>>> same or a little less space.
>>>>
>>>> That was and remains my suggestion.
>>>>
>>>
>>> Thanks for your valuable suggestions. We will continue to move forward along
>>> it and need your more guidance.
>>>
>>> But there are also a few codes that do take the extra{1, 2} as pointers, for
>>> example:
>>>
>>> int neigh_sysctl_register(struct net_device *dev, struct neigh_parms *p,
>>>                            proc_handler *handler)
>>> {
>>> ...
>>>          for (i = 0; i < NEIGH_VAR_GC_INTERVAL; i++) {
>>>                  t->neigh_vars[i].data += (long) p;
>>>                  t->neigh_vars[i].extra1 = dev;
>>>                  t->neigh_vars[i].extra2 = p;
>>>          }
>>> ...
>>> }
>>>
>>> static void neigh_proc_update(const struct ctl_table *ctl, int write)
>>> {
>>>          struct net_device *dev = ctl->extra1;
>>>          struct neigh_parms *p = ctl->extra2;
>>>          struct net *net = neigh_parms_net(p);
>>>          int index = (int *) ctl->data - p->data;
>>> ...
>>> }
>>>
>>>
>>> So could we modify it in this way to make it compatible with these two
>>> situations:
>>>
>>> @@ -137,8 +137,16 @@ struct ctl_table {
>>>          umode_t mode;
>>>          proc_handler *proc_handler;     /* Callback for text formatting */
>>>          struct ctl_table_poll *poll;
>>> -       void *extra1;
>>> -       void *extra2;
>>> +       union {
>>> +               struct {
>>> +                       void *extra1;
>>> +                       void *extra2;
>>> +               };
>>> +               struct {
>>> +                       unsigned long min;
>>> +                       unsigned long max;
>>> +               };
>>> +       };
>>>   } __randomize_layout;
>>
>> I'm still not convinced that a union is the best way out of this. I have
>> postponed reviewing this for several weeks, but I'm slowly coming back
>> to it.
>>
>> Thx for your suggestion
>>
>> Best
>>
>>
>> -- 
>>
>> Joel Granados
>