From: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
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@ssi.gouv.fr>
---
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,
},
};
--
2.48.1From: Chuck Lever <chuck.lever@oracle.com>
On Mon, 24 Feb 2025 10:58:17 +0100, nicolas.bouchinet@clip-os.org wrote:
> Bound nsm_local_state sysctl writings between SYSCTL_ZERO
> and SYSCTL_INT_MAX.
>
> The proc_handler has thus been updated to proc_dointvec_minmax.
>
>
Applied to nfsd-testing with modifications, thanks!
[2/6] sysctl: Fixes nsm_local_state bounds
commit: 8e6d33ea0159b39d670b7986324bd6135ee9d5f7
--
Chuck Lever
On Mon, Mar 03, 2025 at 04:42:17PM -0500, cel@kernel.org wrote: > From: Chuck Lever <chuck.lever@oracle.com> > > On Mon, 24 Feb 2025 10:58:17 +0100, nicolas.bouchinet@clip-os.org wrote: > > Bound nsm_local_state sysctl writings between SYSCTL_ZERO > > and SYSCTL_INT_MAX. > > > > The proc_handler has thus been updated to proc_dointvec_minmax. > > > > > > Applied to nfsd-testing with modifications, thanks! > > [2/6] sysctl: Fixes nsm_local_state bounds > commit: 8e6d33ea0159b39d670b7986324bd6135ee9d5f7 > > -- > Chuck Lever Thx! @nicolas: remove this one from your next Version Best -- Joel Granados
On 2/24/25 4:58 AM, nicolas.bouchinet@clip-os.org wrote:
> From: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
>
> 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@ssi.gouv.fr>
> ---
> 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.
--
Chuck Lever
On Mon, Feb 24, 2025 at 09:38:17AM -0500, Chuck Lever wrote:
> On 2/24/25 4:58 AM, nicolas.bouchinet@clip-os.org wrote:
> > From: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
> >
> > 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@ssi.gouv.fr>
> > ---
> > 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@kernel.org>
Best
>
>
> --
> Chuck Lever
--
Joel Granados
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@clip-os.org wrote:
>>> From: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
>>>
>>> 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@ssi.gouv.fr>
>>> ---
>>> 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@kernel.org>
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
On 2/24/25 15:38, Chuck Lever wrote:
> On 2/24/25 4:58 AM, nicolas.bouchinet@clip-os.org wrote:
>> From: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
>>
>> 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@ssi.gouv.fr>
>> ---
>> 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.
>
>
Hi Chuck,
Thank's for your review.
If `nsm_local_state` should be set to the
full range of an uint32_t by a user writing in the sysctl, then yes it
should
use `proc_douintvec` instead of limiting it to SYSCTL_INT_MAX value
(INT_MAX).
I've used `proc_dointvec_minmax` since it already used `proc_dointvec`
and thus
was already capped at INT_MAX.
Best regards,
Nicolas
© 2016 - 2026 Red Hat, Inc.