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

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

[Wei Gao]

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")
Signed-off-by: Wei Gao <wegao@suse.com>
---
 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)
 				return -EINVAL;
 			break;
 		case IOPRIO_CLASS_IDLE:
 			break;
 		case IOPRIO_CLASS_NONE:
-			if (level)
+			if (data)
 				return -EINVAL;
 			break;
 		case IOPRIO_CLASS_INVALID:
-- 
2.34.1

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

[Damien Le Moal]

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@suse.com>
> ---
>  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.

>  				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

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@suse.com>
> > ---
> >  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
>