Re: [PATCH] block: improve ioprio value validity checks

[Damien Le Moal <dlemoal@kernel.org>]

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

-- 
Damien Le Moal
Western Digital Research