block: improve ioprio value validity checks

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(-)

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>