| Message ID | 20230530061307.525644-1-dlemoal@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: improve ioprio value validity checks | expand |
On Tue, May 30, 2023 at 03:13:07PM +0900, Damien Le Moal wrote: > The introduction of the macro IOPRIO_PRIO_LEVEL() in commit > eca2040972b4 ("scsi: block: ioprio: Clean up interface definition") > results in an iopriority level to always be masked using the macro > IOPRIO_LEVEL_MASK, and thus to the kernel always seeing an acceptable > value for an I/O priority level when checked in ioprio_check_cap(). > Before this patch, this function would return an error for some (but not > all) invalid values for a level valid range of [0..7]. > > Restore and improve the detection of invalid priority levels by > introducing the inline function ioprio_value() to check an ioprio class, > level and hint value before combining these fields into a single value > to be used with ioprio_set() or AIOs. If an invalid value for the class, > level or hint of an ioprio is detected, ioprio_value() returns an ioprio > using the class IOPRIO_CLASS_INVALID, indicating an invalid value and > causing ioprio_check_cap() to return -EINVAL. > > Fixes: 6c913257226a ("scsi: block: Introduce ioprio hints") > Fixes: eca2040972b4 ("scsi: block: ioprio: Clean up interface definition") > Signed-off-by: Damien Le Moal <dlemoal@kernel.org> > --- > block/ioprio.c | 1 + > include/uapi/linux/ioprio.h | 47 +++++++++++++++++++++++-------------- > 2 files changed, 31 insertions(+), 17 deletions(-) > > diff --git a/block/ioprio.c b/block/ioprio.c > index f0d9e818abc5..b5a942519a79 100644 > --- a/block/ioprio.c > +++ b/block/ioprio.c > @@ -58,6 +58,7 @@ int ioprio_check_cap(int ioprio) > if (level) > return -EINVAL; > break; > + case IOPRIO_CLASS_INVALID: > default: > return -EINVAL; > } > diff --git a/include/uapi/linux/ioprio.h b/include/uapi/linux/ioprio.h > index 607c7617b9d2..7310449c0178 100644 > --- a/include/uapi/linux/ioprio.h > +++ b/include/uapi/linux/ioprio.h > @@ -6,15 +6,13 @@ > * Gives us 8 prio classes with 13-bits of data for each class > */ > #define IOPRIO_CLASS_SHIFT 13 > -#define IOPRIO_CLASS_MASK 0x07 > +#define IOPRIO_NR_CLASSES 8 > +#define IOPRIO_CLASS_MASK (IOPRIO_NR_CLASSES - 1) > #define IOPRIO_PRIO_MASK ((1UL << IOPRIO_CLASS_SHIFT) - 1) > > #define IOPRIO_PRIO_CLASS(ioprio) \ > (((ioprio) >> IOPRIO_CLASS_SHIFT) & IOPRIO_CLASS_MASK) > #define IOPRIO_PRIO_DATA(ioprio) ((ioprio) & IOPRIO_PRIO_MASK) > -#define IOPRIO_PRIO_VALUE(class, data) \ > - ((((class) & IOPRIO_CLASS_MASK) << IOPRIO_CLASS_SHIFT) | \ > - ((data) & IOPRIO_PRIO_MASK)) > > /* > * These are the io priority classes as implemented by the BFQ and mq-deadline > @@ -25,10 +23,13 @@ > * served when no one else is using the disk. > */ > enum { > - IOPRIO_CLASS_NONE, > - IOPRIO_CLASS_RT, > - IOPRIO_CLASS_BE, > - IOPRIO_CLASS_IDLE, > + IOPRIO_CLASS_NONE = 0, > + IOPRIO_CLASS_RT = 1, > + IOPRIO_CLASS_BE = 2, > + IOPRIO_CLASS_IDLE = 3, > + > + /* Special class to indicate an invalid ioprio value */ > + IOPRIO_CLASS_INVALID = 7, > }; > > /* > @@ -73,15 +74,6 @@ enum { > #define IOPRIO_PRIO_HINT(ioprio) \ > (((ioprio) >> IOPRIO_HINT_SHIFT) & IOPRIO_HINT_MASK) > > -/* > - * Alternate macro for IOPRIO_PRIO_VALUE() to define an IO priority with > - * a class, level and hint. > - */ > -#define IOPRIO_PRIO_VALUE_HINT(class, level, hint) \ > - ((((class) & IOPRIO_CLASS_MASK) << IOPRIO_CLASS_SHIFT) | \ > - (((hint) & IOPRIO_HINT_MASK) << IOPRIO_HINT_SHIFT) | \ > - ((level) & IOPRIO_LEVEL_MASK)) > - > /* > * IO hints. > */ > @@ -107,4 +99,25 @@ enum { > IOPRIO_HINT_DEV_DURATION_LIMIT_7 = 7, > }; > > +#define IOPRIO_BAD_VALUE(val, max) ((val) < 0 || (val) >= (max)) > + > +/* > + * Return an I/O priority value based on a class, a level and a hint. > + */ > +static __always_inline __u16 ioprio_value(int class, int level, int hint) > +{ > + if (IOPRIO_BAD_VALUE(class, IOPRIO_NR_CLASSES) || > + IOPRIO_BAD_VALUE(level, IOPRIO_NR_LEVELS) || > + IOPRIO_BAD_VALUE(hint, IOPRIO_NR_HINTS)) > + return IOPRIO_CLASS_INVALID << IOPRIO_CLASS_SHIFT; > + > + return (class << IOPRIO_CLASS_SHIFT) | > + (hint << IOPRIO_HINT_SHIFT) | level; > +} > + > +#define IOPRIO_PRIO_VALUE(class, level) \ > + ioprio_value(class, level, IOPRIO_HINT_NONE) > +#define IOPRIO_PRIO_VALUE_HINT(class, level, hint) \ > + ioprio_value(class, level, hint) > + > #endif /* _UAPI_LINUX_IOPRIO_H */ > -- > 2.40.1 > Some additional context: We noticed that the LTP test case: https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/ioprio/ioprio_set03.c Started failing since this commit in linux-next: eca2040972b4 ("scsi: block: ioprio: Clean up interface definition") The test case expects that a syscall that sets ioprio with a class of 8 should fail. Before this commit in linux next, the 16 bit ioprio was defined like this: 3 bits class | 13 bits level However, ioprio_check_cap() rejected any priority levels in the range 8-8191, which meant that the only bits that could actually be used to store an ioprio were: 3 bits class | 10 bits unused | 3 bits level The 10 unused bits were defined to store an ioprio hint in commit: 6c913257226a ("scsi: block: Introduce ioprio hints"), so it is now: 3 bits class | 10 bits hint | 3 bits level This meant that the LTP test trying to set a ioprio level of 8, will no longer fail. It will now set a level of 0, and a hint of value 1. The fix that Damien suggested, which adds multiple boundary checks in the IOPRIO_PRIO_VALUE() macro will fix any user space program that uses the uapi header. However, some applications, like the LTP test case, do not use the uapi header, but defines the macros inside their own header. Note that even before commit: eca2040972b4 ("scsi: block: ioprio: Clean up interface definition") The exact same problem existed, ioprio_check_cap() would not give an error if a user space program sent in a level that was higher than what could be represented by the bits used to define the level, e.g. a user space program using IOPRIO_PRIO_VALUE(IOPRIO_CLASS_RT, 8192) would not have the syscall return error, even though the level was higher than 7. (And the effective level would be 0.) The question is if we want to dedicate a priority class to be able to detect, in runtime, a user trying to set a level higher than 7. (It will however only work if the user space program uses the uapi header.) One option could be to do nothing. The previous check wasn't able to detect invalid levels higher than what can be represented by the level field, so this behavior is kept, it is just that the level field itself has shrunk. The LTP test case needs to be updated anyway, since it copies the ioprio macros instead of including the uapi header. Kind regards, Niklas
On Tue, May 30, 2023 at 11:09 AM Niklas Cassel <Niklas.Cassel@wdc.com> wrote: > We noticed that the LTP test case: > https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/ioprio/ioprio_set03.c > > Started failing since this commit in linux-next: > eca2040972b4 ("scsi: block: ioprio: Clean up interface definition") > > The test case expects that a syscall that sets ioprio with a class of 8 > should fail. > > Before this commit in linux next, the 16 bit ioprio was defined like this: > 3 bits class | 13 bits level > > However, ioprio_check_cap() rejected any priority levels in the range > 8-8191, which meant that the only bits that could actually be used to > store an ioprio were: > 3 bits class | 10 bits unused | 3 bits level > > The 10 unused bits were defined to store an ioprio hint in commit: > 6c913257226a ("scsi: block: Introduce ioprio hints"), so it is now: > 3 bits class | 10 bits hint | 3 bits level > > This meant that the LTP test trying to set a ioprio level of 8, > will no longer fail. It will now set a level of 0, and a hint of value 1. Wow good digging! I knew the test would be good for something. Like for maintaining the test. > The fix that Damien suggested, which adds multiple boundary checks in the > IOPRIO_PRIO_VALUE() macro will fix any user space program that uses the uapi > header. Fixing things in the UAPI headers make it easier to do things right going forward with classes and all. > However, some applications, like the LTP test case, do not use the > uapi header, but defines the macros inside their own header. IIRC that was because there were no UAPI headers when the test was created, I don't think I was just randomly lazy... Well maybe I was. The numbers are ABI anyway, not the header files. > Note that even before commit: > eca2040972b4 ("scsi: block: ioprio: Clean up interface definition") > > The exact same problem existed, ioprio_check_cap() would not give an > error if a user space program sent in a level that was higher than > what could be represented by the bits used to define the level, > e.g. a user space program using IOPRIO_PRIO_VALUE(IOPRIO_CLASS_RT, 8192) > would not have the syscall return error, even though the level was higher > than 7. (And the effective level would be 0.) > > The LTP test case needs to be updated anyway, since it copies the ioprio > macros instead of including the uapi header. Yeah one of the reasons the kernel fails is in order to be able to slot in new behaviour in response to these ioctls, so the test should probably just be updated to also test the new scheduling classes and all that, using the UAPI headers. Will you send a patch? Yours, Linus Walleij
On 5/30/23 20:30, Linus Walleij wrote: > On Tue, May 30, 2023 at 11:09 AM Niklas Cassel <Niklas.Cassel@wdc.com> wrote: > >> We noticed that the LTP test case: >> https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/ioprio/ioprio_set03.c >> >> Started failing since this commit in linux-next: >> eca2040972b4 ("scsi: block: ioprio: Clean up interface definition") >> >> The test case expects that a syscall that sets ioprio with a class of 8 >> should fail. >> >> Before this commit in linux next, the 16 bit ioprio was defined like this: >> 3 bits class | 13 bits level >> >> However, ioprio_check_cap() rejected any priority levels in the range >> 8-8191, which meant that the only bits that could actually be used to >> store an ioprio were: >> 3 bits class | 10 bits unused | 3 bits level >> >> The 10 unused bits were defined to store an ioprio hint in commit: >> 6c913257226a ("scsi: block: Introduce ioprio hints"), so it is now: >> 3 bits class | 10 bits hint | 3 bits level >> >> This meant that the LTP test trying to set a ioprio level of 8, >> will no longer fail. It will now set a level of 0, and a hint of value 1. > > Wow good digging! I knew the test would be good for something. > Like for maintaining the test. > >> The fix that Damien suggested, which adds multiple boundary checks in the >> IOPRIO_PRIO_VALUE() macro will fix any user space program that uses the uapi >> header. > > Fixing things in the UAPI headers make it easier to do things right > going forward with classes and all. > >> However, some applications, like the LTP test case, do not use the >> uapi header, but defines the macros inside their own header. > > IIRC that was because there were no UAPI headers when the test > was created, I don't think I was just randomly lazy... Well maybe I > was. The numbers are ABI anyway, not the header files. > >> Note that even before commit: >> eca2040972b4 ("scsi: block: ioprio: Clean up interface definition") >> >> The exact same problem existed, ioprio_check_cap() would not give an >> error if a user space program sent in a level that was higher than >> what could be represented by the bits used to define the level, >> e.g. a user space program using IOPRIO_PRIO_VALUE(IOPRIO_CLASS_RT, 8192) >> would not have the syscall return error, even though the level was higher >> than 7. (And the effective level would be 0.) >> >> The LTP test case needs to be updated anyway, since it copies the ioprio >> macros instead of including the uapi header. > > Yeah one of the reasons the kernel fails is in order to be > able to slot in new behaviour in response to these ioctls, > so the test should probably just be updated to also test > the new scheduling classes and all that, using the UAPI > headers. > > Will you send a patch? Yep, we can work on something. Which kernel versions are these tests run against ? I mean, do we need to patch assuming that /usr/include/linux/ioprio.h may not exist ? I did a quick hack replacing the internal definitions in ltp ioprio.h with an include of the patched header file, and all is good. But that needs to work with older kernels as well and I wonder how far back we need to go. > > Yours, > Linus Walleij
On 5/30/23 20:30, Linus Walleij wrote: > On Tue, May 30, 2023 at 11:09 AM Niklas Cassel <Niklas.Cassel@wdc.com> wrote: > >> We noticed that the LTP test case: >> https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/ioprio/ioprio_set03.c >> >> Started failing since this commit in linux-next: >> eca2040972b4 ("scsi: block: ioprio: Clean up interface definition") >> >> The test case expects that a syscall that sets ioprio with a class of 8 >> should fail. >> >> Before this commit in linux next, the 16 bit ioprio was defined like this: >> 3 bits class | 13 bits level >> >> However, ioprio_check_cap() rejected any priority levels in the range >> 8-8191, which meant that the only bits that could actually be used to >> store an ioprio were: >> 3 bits class | 10 bits unused | 3 bits level >> >> The 10 unused bits were defined to store an ioprio hint in commit: >> 6c913257226a ("scsi: block: Introduce ioprio hints"), so it is now: >> 3 bits class | 10 bits hint | 3 bits level >> >> This meant that the LTP test trying to set a ioprio level of 8, >> will no longer fail. It will now set a level of 0, and a hint of value 1. > > Wow good digging! I knew the test would be good for something. > Like for maintaining the test. > >> The fix that Damien suggested, which adds multiple boundary checks in the >> IOPRIO_PRIO_VALUE() macro will fix any user space program that uses the uapi >> header. > > Fixing things in the UAPI headers make it easier to do things right > going forward with classes and all. > >> However, some applications, like the LTP test case, do not use the >> uapi header, but defines the macros inside their own header. > > IIRC that was because there were no UAPI headers when the test > was created, I don't think I was just randomly lazy... Well maybe I > was. The numbers are ABI anyway, not the header files. > >> Note that even before commit: >> eca2040972b4 ("scsi: block: ioprio: Clean up interface definition") >> >> The exact same problem existed, ioprio_check_cap() would not give an >> error if a user space program sent in a level that was higher than >> what could be represented by the bits used to define the level, >> e.g. a user space program using IOPRIO_PRIO_VALUE(IOPRIO_CLASS_RT, 8192) >> would not have the syscall return error, even though the level was higher >> than 7. (And the effective level would be 0.) >> >> The LTP test case needs to be updated anyway, since it copies the ioprio >> macros instead of including the uapi header. > > Yeah one of the reasons the kernel fails is in order to be > able to slot in new behaviour in response to these ioctls, > so the test should probably just be updated to also test > the new scheduling classes and all that, using the UAPI > headers. > > Will you send a patch? Linus, I sent a couple of patches for ltp to fix this. As I am not subscribed to the ltp list, I got the usual "Your mail to 'ltp' with the subject ... Is being held until the list moderator can review it for approval.". > > Yours, > Linus Walleij
On Tue, May 30, 2023 at 03:13:07PM +0900, Damien Le Moal wrote: > The introduction of the macro IOPRIO_PRIO_LEVEL() in commit > eca2040972b4 ("scsi: block: ioprio: Clean up interface definition") > results in an iopriority level to always be masked using the macro > IOPRIO_LEVEL_MASK, and thus to the kernel always seeing an acceptable > value for an I/O priority level when checked in ioprio_check_cap(). > Before this patch, this function would return an error for some (but not > all) invalid values for a level valid range of [0..7]. > > Restore and improve the detection of invalid priority levels by > introducing the inline function ioprio_value() to check an ioprio class, > level and hint value before combining these fields into a single value > to be used with ioprio_set() or AIOs. If an invalid value for the class, > level or hint of an ioprio is detected, ioprio_value() returns an ioprio > using the class IOPRIO_CLASS_INVALID, indicating an invalid value and > causing ioprio_check_cap() to return -EINVAL. > > Fixes: 6c913257226a ("scsi: block: Introduce ioprio hints") > Fixes: eca2040972b4 ("scsi: block: ioprio: Clean up interface definition") > Signed-off-by: Damien Le Moal <dlemoal@kernel.org> > --- > block/ioprio.c | 1 + > include/uapi/linux/ioprio.h | 47 +++++++++++++++++++++++-------------- > 2 files changed, 31 insertions(+), 17 deletions(-) > > diff --git a/block/ioprio.c b/block/ioprio.c > index f0d9e818abc5..b5a942519a79 100644 > --- a/block/ioprio.c > +++ b/block/ioprio.c > @@ -58,6 +58,7 @@ int ioprio_check_cap(int ioprio) > if (level) > return -EINVAL; > break; > + case IOPRIO_CLASS_INVALID: > default: > return -EINVAL; > } > diff --git a/include/uapi/linux/ioprio.h b/include/uapi/linux/ioprio.h > index 607c7617b9d2..7310449c0178 100644 > --- a/include/uapi/linux/ioprio.h > +++ b/include/uapi/linux/ioprio.h > @@ -6,15 +6,13 @@ > * Gives us 8 prio classes with 13-bits of data for each class > */ > #define IOPRIO_CLASS_SHIFT 13 > -#define IOPRIO_CLASS_MASK 0x07 > +#define IOPRIO_NR_CLASSES 8 > +#define IOPRIO_CLASS_MASK (IOPRIO_NR_CLASSES - 1) > #define IOPRIO_PRIO_MASK ((1UL << IOPRIO_CLASS_SHIFT) - 1) > > #define IOPRIO_PRIO_CLASS(ioprio) \ > (((ioprio) >> IOPRIO_CLASS_SHIFT) & IOPRIO_CLASS_MASK) > #define IOPRIO_PRIO_DATA(ioprio) ((ioprio) & IOPRIO_PRIO_MASK) > -#define IOPRIO_PRIO_VALUE(class, data) \ > - ((((class) & IOPRIO_CLASS_MASK) << IOPRIO_CLASS_SHIFT) | \ > - ((data) & IOPRIO_PRIO_MASK)) > > /* > * These are the io priority classes as implemented by the BFQ and mq-deadline > @@ -25,10 +23,13 @@ > * served when no one else is using the disk. > */ > enum { > - IOPRIO_CLASS_NONE, > - IOPRIO_CLASS_RT, > - IOPRIO_CLASS_BE, > - IOPRIO_CLASS_IDLE, > + IOPRIO_CLASS_NONE = 0, > + IOPRIO_CLASS_RT = 1, > + IOPRIO_CLASS_BE = 2, > + IOPRIO_CLASS_IDLE = 3, > + > + /* Special class to indicate an invalid ioprio value */ > + IOPRIO_CLASS_INVALID = 7, > }; > > /* > @@ -73,15 +74,6 @@ enum { > #define IOPRIO_PRIO_HINT(ioprio) \ > (((ioprio) >> IOPRIO_HINT_SHIFT) & IOPRIO_HINT_MASK) > > -/* > - * Alternate macro for IOPRIO_PRIO_VALUE() to define an IO priority with > - * a class, level and hint. > - */ > -#define IOPRIO_PRIO_VALUE_HINT(class, level, hint) \ > - ((((class) & IOPRIO_CLASS_MASK) << IOPRIO_CLASS_SHIFT) | \ > - (((hint) & IOPRIO_HINT_MASK) << IOPRIO_HINT_SHIFT) | \ > - ((level) & IOPRIO_LEVEL_MASK)) > - > /* > * IO hints. > */ > @@ -107,4 +99,25 @@ enum { > IOPRIO_HINT_DEV_DURATION_LIMIT_7 = 7, > }; > > +#define IOPRIO_BAD_VALUE(val, max) ((val) < 0 || (val) >= (max)) > + > +/* > + * Return an I/O priority value based on a class, a level and a hint. > + */ > +static __always_inline __u16 ioprio_value(int class, int level, int hint) > +{ > + if (IOPRIO_BAD_VALUE(class, IOPRIO_NR_CLASSES) || > + IOPRIO_BAD_VALUE(level, IOPRIO_NR_LEVELS) || > + IOPRIO_BAD_VALUE(hint, IOPRIO_NR_HINTS)) > + return IOPRIO_CLASS_INVALID << IOPRIO_CLASS_SHIFT; > + > + return (class << IOPRIO_CLASS_SHIFT) | > + (hint << IOPRIO_HINT_SHIFT) | level; > +} > + > +#define IOPRIO_PRIO_VALUE(class, level) \ > + ioprio_value(class, level, IOPRIO_HINT_NONE) > +#define IOPRIO_PRIO_VALUE_HINT(class, level, hint) \ > + ioprio_value(class, level, hint) > + > #endif /* _UAPI_LINUX_IOPRIO_H */ > -- > 2.40.1 > It seems a bit unfortunate that we need to "allocate" a priority class just in order to detect values out of range, but considering that this enables out of range checking for class, level, and hint, I think that this it is worth it. (Especially since there wasn't any _reliable_ out of range checking done before.) Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>
On Tue, May 30, 2023 at 01:30:18PM +0200, Linus Walleij wrote: > On Tue, May 30, 2023 at 11:09 AM Niklas Cassel <Niklas.Cassel@wdc.com> wrote: > > > The fix that Damien suggested, which adds multiple boundary checks in the > > IOPRIO_PRIO_VALUE() macro will fix any user space program that uses the uapi > > header. > > Fixing things in the UAPI headers make it easier to do things right > going forward with classes and all. Hello Linus, Thank you for providing your input! Considering that you seem to be in favor of fixing things in the UAPI headers (i.e. the patch in $subject). Could you possibly consider sending a proper A-b or R-b tag? Kind regards, Niklas
diff --git a/block/ioprio.c b/block/ioprio.c index f0d9e818abc5..b5a942519a79 100644 --- a/block/ioprio.c +++ b/block/ioprio.c @@ -58,6 +58,7 @@ int ioprio_check_cap(int ioprio) if (level) return -EINVAL; break; + case IOPRIO_CLASS_INVALID: default: return -EINVAL; } diff --git a/include/uapi/linux/ioprio.h b/include/uapi/linux/ioprio.h index 607c7617b9d2..7310449c0178 100644 --- a/include/uapi/linux/ioprio.h +++ b/include/uapi/linux/ioprio.h @@ -6,15 +6,13 @@ * Gives us 8 prio classes with 13-bits of data for each class */ #define IOPRIO_CLASS_SHIFT 13 -#define IOPRIO_CLASS_MASK 0x07 +#define IOPRIO_NR_CLASSES 8 +#define IOPRIO_CLASS_MASK (IOPRIO_NR_CLASSES - 1) #define IOPRIO_PRIO_MASK ((1UL << IOPRIO_CLASS_SHIFT) - 1) #define IOPRIO_PRIO_CLASS(ioprio) \ (((ioprio) >> IOPRIO_CLASS_SHIFT) & IOPRIO_CLASS_MASK) #define IOPRIO_PRIO_DATA(ioprio) ((ioprio) & IOPRIO_PRIO_MASK) -#define IOPRIO_PRIO_VALUE(class, data) \ - ((((class) & IOPRIO_CLASS_MASK) << IOPRIO_CLASS_SHIFT) | \ - ((data) & IOPRIO_PRIO_MASK)) /* * These are the io priority classes as implemented by the BFQ and mq-deadline @@ -25,10 +23,13 @@ * served when no one else is using the disk. */ enum { - IOPRIO_CLASS_NONE, - IOPRIO_CLASS_RT, - IOPRIO_CLASS_BE, - IOPRIO_CLASS_IDLE, + IOPRIO_CLASS_NONE = 0, + IOPRIO_CLASS_RT = 1, + IOPRIO_CLASS_BE = 2, + IOPRIO_CLASS_IDLE = 3, + + /* Special class to indicate an invalid ioprio value */ + IOPRIO_CLASS_INVALID = 7, }; /* @@ -73,15 +74,6 @@ enum { #define IOPRIO_PRIO_HINT(ioprio) \ (((ioprio) >> IOPRIO_HINT_SHIFT) & IOPRIO_HINT_MASK) -/* - * Alternate macro for IOPRIO_PRIO_VALUE() to define an IO priority with - * a class, level and hint. - */ -#define IOPRIO_PRIO_VALUE_HINT(class, level, hint) \ - ((((class) & IOPRIO_CLASS_MASK) << IOPRIO_CLASS_SHIFT) | \ - (((hint) & IOPRIO_HINT_MASK) << IOPRIO_HINT_SHIFT) | \ - ((level) & IOPRIO_LEVEL_MASK)) - /* * IO hints. */ @@ -107,4 +99,25 @@ enum { IOPRIO_HINT_DEV_DURATION_LIMIT_7 = 7, }; +#define IOPRIO_BAD_VALUE(val, max) ((val) < 0 || (val) >= (max)) + +/* + * Return an I/O priority value based on a class, a level and a hint. + */ +static __always_inline __u16 ioprio_value(int class, int level, int hint) +{ + if (IOPRIO_BAD_VALUE(class, IOPRIO_NR_CLASSES) || + IOPRIO_BAD_VALUE(level, IOPRIO_NR_LEVELS) || + IOPRIO_BAD_VALUE(hint, IOPRIO_NR_HINTS)) + return IOPRIO_CLASS_INVALID << IOPRIO_CLASS_SHIFT; + + return (class << IOPRIO_CLASS_SHIFT) | + (hint << IOPRIO_HINT_SHIFT) | level; +} + +#define IOPRIO_PRIO_VALUE(class, level) \ + ioprio_value(class, level, IOPRIO_HINT_NONE) +#define IOPRIO_PRIO_VALUE_HINT(class, level, hint) \ + ioprio_value(class, level, hint) + #endif /* _UAPI_LINUX_IOPRIO_H */
The introduction of the macro IOPRIO_PRIO_LEVEL() in commit eca2040972b4 ("scsi: block: ioprio: Clean up interface definition") results in an iopriority level to always be masked using the macro IOPRIO_LEVEL_MASK, and thus to the kernel always seeing an acceptable value for an I/O priority level when checked in ioprio_check_cap(). Before this patch, this function would return an error for some (but not all) invalid values for a level valid range of [0..7]. Restore and improve the detection of invalid priority levels by introducing the inline function ioprio_value() to check an ioprio class, level and hint value before combining these fields into a single value to be used with ioprio_set() or AIOs. If an invalid value for the class, level or hint of an ioprio is detected, ioprio_value() returns an ioprio using the class IOPRIO_CLASS_INVALID, indicating an invalid value and causing ioprio_check_cap() to return -EINVAL. Fixes: 6c913257226a ("scsi: block: Introduce ioprio hints") Fixes: eca2040972b4 ("scsi: block: ioprio: Clean up interface definition") Signed-off-by: Damien Le Moal <dlemoal@kernel.org> --- block/ioprio.c | 1 + include/uapi/linux/ioprio.h | 47 +++++++++++++++++++++++-------------- 2 files changed, 31 insertions(+), 17 deletions(-)