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

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

On 2025/3/3 17:26, 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:
>>>
>>>> On Thu, Jan 23, 2025 at 12:30:25PM -0600, Eric W. Biederman wrote:
>>>>> "Eric W. Biederman" <ebiederm@xmission.com> writes:
>>>>>
>>>>>> Joel Granados <joel.granados@kernel.org> writes:
>>>>>>
>>>>>>> On Sun, Jan 19, 2025 at 10:59:21PM +0800, Wen Yang wrote:
> ...
>>>> struct ctl_table {
>>>> 	const char *procname;		/* Text ID for /proc/sys */
>>>> 	void *data;
>>>> 	int maxlen;
>>>> 	umode_t mode;
>>>> 	proc_handler *proc_handler;	/* Callback for text formatting */
>>>> 	struct ctl_table_poll *poll;
>>>>         unsigned long min;
>>>>         unsigned long max;
>>>> } __randomize_layout;
>>>
>>> That is just replace extra1 and extra2 with min and max members.  The
>>> members don't have any reason to be pointers.  Without being pointers
>>> the min/max functions can just use long values to cap either ints or
>>> longs, and there is no room for error.  The integer promotion rules
>>> will ensure that even negative values can be stored in unsigned long
>>> min and max values successfully.  Plus it is all bog standard C
>>> so there is nothing special to learn.
>>>
>>> There are a bunch of fiddly little details to transition from where we
>>> are today.  The most straightforward way I can see of making the
>>> transition is to add the min and max members.  Come up with replacements
>>> for proc_doulongvec_minmax and proc_dointvec_minmax that read the new
>>> min and max members.  Update all of the users.  Update the few users
>>> 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;
>> ...
>> }
> Quick question: Do you have a systemic way of identifying these? Do you
> have a grep or awk scripts somewhere? I'm actually very interested in
> finding out what is the impact of this.
> 

Thanks, we may use the following simple scripts:

- the extra {1,2} as pointers to some objects:
$ grep "\.extra1\|\.extra2" * -R | grep -v "SYSCTL_" | grep -v "\&"

- the extra {1,2} as pointers to elements in the shared constant array:
$ grep "\.extra1\|\.extra2" * -R | grep "SYSCTL_"

- the extra {1,2} as pointers to additional constant variables:
$ grep "\.extra1\|\.extra2" * -R | grep "\&"


--
Best wishes,
Wen


> 
> 
>>
>>
>> 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;
>>
>>
>> --
>> Best wishes,
>> Wen
>>
>