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?

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

-- 
Damien Le Moal
Western Digital Research