Re: [PATCH v1] block: ioprio: Fix ioprio_check_cap() validation logic

[Wei Gao]

On Fri, Nov 24, 2023 at 02:53:52PM +0900, Damien Le Moal wrote:
> On 11/24/23 12:05, Wei Gao wrote:
> > The current logic "if (level >= IOPRIO_NR_LEVELS)" can not be reached since
> > level value get from IOPRIO_PRIO_LEVEL ONLY extract lower 3-bits of ioprio.
> > (IOPRIO_NR_LEVELS=8)
> > 
> > So this trigger LTP test case ioprio_set03 failed, the test case expect
> > error when set IOPRIO_CLASS_BE prio 8, in current implementation level
> > value will be 0 and obviously can not return error.
> > 
> > Fixes: eca2040972b4 ("scsi: block: ioprio: Clean up interface definition")
> 
> No. Please see below.
> 
> > Signed-off-by: Wei Gao <wegao@xxxxxxxx>
> > ---
> >  block/ioprio.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/block/ioprio.c b/block/ioprio.c
> > index b5a942519a79..f83029208f2a 100644
> > --- a/block/ioprio.c
> > +++ b/block/ioprio.c
> > @@ -33,7 +33,7 @@
> >  int ioprio_check_cap(int ioprio)
> >  {
> >  	int class = IOPRIO_PRIO_CLASS(ioprio);
> > -	int level = IOPRIO_PRIO_LEVEL(ioprio);
> > +	int data = IOPRIO_PRIO_DATA(ioprio);
> >  
> >  	switch (class) {
> >  		case IOPRIO_CLASS_RT:
> > @@ -49,13 +49,13 @@ int ioprio_check_cap(int ioprio)
> >  			fallthrough;
> >  			/* rt has prio field too */
> >  		case IOPRIO_CLASS_BE:
> > -			if (level >= IOPRIO_NR_LEVELS)
> > +			if (data >= IOPRIO_NR_LEVELS || data < 0)
> 
> This is incorrect: data is the combination of level AND hints, so that value can
> be larger than or equal to 8 with the level still being valid. Hard NACK on this.
> 
> The issue with LTP test case has been fixed in LTP and by changing the ioprio.h
> header file. See commit 01584c1e2337 ("scsi: block: Improve ioprio value
> validity checks") which introduces IOPRIO_BAD_VALUE() macro for that.
> 
> And for ltp, the commits are:
> 6b7f448fe392 ("ioprio: Use IOPRIO_PRIO_NUM to check prio range")
> 7c84fa710f75 ("ioprio: use ioprio.h kernel header if it exists")
> 
> So please update your setup, including your install of kernel user API header files.
> 

Thanks a lot for your quick feedback and detail explaination, if i am guess correctly, 
my test kernel include eca2040972b4 ("scsi: block: ioprio: Clean up interface definition") but 
not include 01584c1e2337 ("scsi: block: Improve ioprio value validity checks") by coincidence.

> >  				return -EINVAL;
> >  			break;
> >  		case IOPRIO_CLASS_IDLE:
> >  			break;
> >  		case IOPRIO_CLASS_NONE:
> > -			if (level)
> > +			if (data)
> >  				return -EINVAL;
> >  			break;
> >  		case IOPRIO_CLASS_INVALID:
> 
> -- 
> Damien Le Moal
> Western Digital Research
>