Re: [PATCH v2 4/6] sysctl: Fixes scsi_logging_level bounds

On Tue, Feb 25, 2025 at 11:47:42AM +0100, Nicolas Bouchinet wrote:
> 
> On 2/25/25 02:20, Martin K. Petersen wrote:
> > Hi Nicolas!
> > 
> > > --- a/drivers/scsi/scsi_sysctl.c
> > > +++ b/drivers/scsi/scsi_sysctl.c
> > > @@ -17,7 +17,9 @@ static const struct ctl_table scsi_table[] = {
> > >   	  .data		= &scsi_logging_level,
> > >   	  .maxlen	= sizeof(scsi_logging_level),
> > >   	  .mode		= 0644,
> > > -	  .proc_handler	= proc_dointvec },
> > > +	  .proc_handler	= proc_dointvec_minmax,
> > > +	  .extra1	= SYSCTL_ZERO,
> > > +	  .extra2	= SYSCTL_INT_MAX },
> > scsi_logging_level is a bitmask and should be unsigned.
> > 
> 
> Hi Martin,
> 
> Thank's for your review.
> 
> Does `scsi_logging_level` needs the full range of a unsigned 32-bit integer
> ?
> As it was using `proc_dointvec`, it was capped to an INT_MAX.
> 
> If it effectively need the full range of an unsigned 32-bit integer, the
> `proc_handler` could be changed to `proc_douintvec` as suggested by Chuck.
And as mentioned in another patch in this series:
1. Having the upper bound be SYSCTL_INT_MAX is not necessary (as it is
   silently capped by proc_dointvec_minmax, but it is good to have as it
   adds to the understanding on what the range of the values are.

2. Having the lower bound capped by SYSCTL_ZERO is needed as it will
   prevent ppl trying to assigning negative values to the unsigned integer

Let me know if you take this through the scsi subsystem so I know to
drop it from sysctl 

Best

Reviewed-by: Joel Granados <[email protected]>


> 
> Best regards,
> 
> Nicolas
> 

-- 

Joel Granados