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, nicolas.bouchinet@xxxxxxxxxxx wrote:
>>> From: Nicolas Bouchinet <nicolas.bouchinet@xxxxxxxxxxx>
>>>
>>> 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 <nicolas.bouchinet@xxxxxxxxxxx>
>>> ---
>>> 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 <joel.granados@xxxxxxxxxx>
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
