| Message ID | 20231124030525.31426-1-wegao@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] block: ioprio: Fix ioprio_check_cap() validation logic | expand |
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:
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 >
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:
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(-)