| Message ID | 20230608095556.124001-1-dlemoal@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [RESEND] block: improve ioprio value validity checks | expand |
On Thu, Jun 08, 2023 at 06:55:56PM +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 > I already gave my R-b tag on the original message, but since my tag is not here, I'm giving it again: Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>
On Thu, Jun 8, 2023 at 11:55 AM Damien Le Moal <dlemoal@kernel.org> 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> This makes it easier to get things right. Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On 6/9/23 16:46, Linus Walleij wrote: > On Thu, Jun 8, 2023 at 11:55 AM Damien Le Moal <dlemoal@kernel.org> 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> > > This makes it easier to get things right. > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Thanks Linus ! Martin, can you queue this please ? > > Yours, > Linus Walleij
Damien, > 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]. Applied to 6.5/scsi-staging, thanks!
On Thu, 08 Jun 2023 18:55:56 +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]. > > [...] Applied to 6.5/scsi-queue, thanks! [1/1] block: improve ioprio value validity checks https://git.kernel.org/mkp/scsi/c/01584c1e2337
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(-)