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@xxxxxxxxxx>
> ---
> 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
