Re: [PATCH v2 2/6] sysctl: Fixes nsm_local_state bounds

On 3/3/25 9:12 AM, Joel Granados wrote:
> On Mon, Feb 24, 2025 at 09:38:17AM -0500, Chuck Lever wrote:
>> On 2/24/25 4:58 AM, [email protected] wrote:
>>> From: Nicolas Bouchinet <[email protected]>
>>>
>>> Bound nsm_local_state sysctl writings between SYSCTL_ZERO
>>> and SYSCTL_INT_MAX.
>>>
>>> The proc_handler has thus been updated to proc_dointvec_minmax.
>>>
>>> Signed-off-by: Nicolas Bouchinet <[email protected]>
>>> ---
>>>  fs/lockd/svc.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
>>> index 2c8eedc6c2cc9..984ab233af8b6 100644
>>> --- a/fs/lockd/svc.c
>>> +++ b/fs/lockd/svc.c
>>> @@ -461,7 +461,9 @@ static const struct ctl_table nlm_sysctls[] = {
>>>  		.data		= &nsm_local_state,
>>>  		.maxlen		= sizeof(int),
>>>  		.mode		= 0644,
>>> -		.proc_handler	= proc_dointvec,
>>> +		.proc_handler	= proc_dointvec_minmax,
>>> +		.extra1		= SYSCTL_ZERO,
>>> +		.extra2		= SYSCTL_INT_MAX,
>>>  	},
>>>  };
>>>  
>>
>> Hi Nicolas -
>>
>> nsm_local_state is an unsigned 32-bit integer. The type of that value is
>> defined by spec, because this value is exchanged between peers on the
>> network.
>>
>> Perhaps this patch should replace proc_dointvec with proc_douintvec
>> instead.
> As Nicolas stated, that is completely up to how you used the variable.
> 
> Things to notice:
> 1. If you want the full range of a unsigned long, then you should stop
>    using proc_dointvec as it will upper limit the value to INT_MAX.
> 2. If you want to keep using nsm_local_state as unsigned int, then
>    please add SYSCTL_ZERO as a lower bound to avoid assigning negative
>    values
> 3. Having SYSCTL_INT_MAX is not necessary as it is already capped by
>    proc_dointvec{_minmax,}, but it is nice to have as it makes explicit
>    what is happening.
> 
> Let me know if you take this through your trees so I can remove it from
> sysctl.
> 
> Reviewed-by: Joel Granados <[email protected]>

The variable "nsm_local_state" is declared as "u32".

The range of the value of nsm_local_state is supposed to be 0 -
UINT_MAX (ie, an unsigned 32-bit integer), so I'm proposing:

 		.data		= &nsm_local_state,
 		.maxlen		= sizeof(int),
 		.mode		= 0644,
-		.proc_handler	= proc_dointvec,
+		.proc_handler	= proc_douintvec,
 	},
 };

But I suspect that the "maxlen" field should be changed to something
like "sizeof(nsm_local_state)".

Does that seem correct to you?

I can take this patch through the NFSD tree so it can get some NFS-
specific testing.

-- 
Chuck Lever