| Message ID | 20250224095826.16458-3-nicolas.bouchinet@clip-os.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Fixes multiple sysctl bound checks | expand |
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.
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
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
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, }, };