linux-kernel - Re: [External] Re: [PATCH] block: remove test of io priority level

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <65ed2ba5-3f29-4cfc-8181-29d45edf935a@kernel.org>
Date: Tue, 29 Apr 2025 21:24:48 +0900
From: Damien Le Moal <dlemoal@...nel.org>
To: Aaron Lu <ziqianlu@...edance.com>
Cc: Jens Axboe <axboe@...nel.dk>, Kexin Wei <ys.weikexin@....com>,
 linux-block@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [External] Re: [PATCH] block: remove test of io priority level

On 4/29/25 20:44, Aaron Lu wrote:
> On Tue, Apr 29, 2025 at 07:50:11PM +0900, Damien Le Moal wrote:
>> On 4/29/25 17:29, Aaron Lu wrote:
>>> Ever since commit eca2040972b4("scsi: block: ioprio: Clean up interface
>>> definition"), the io priority level is masked and can no longer be larger
>>> than IOPRIO_NR_LEVELS so remove this now useless test.
>>>
>>> The actual test of io prio level is done in ioprio_value() where any
>>> invalid input of class/level/hint will result in an invalid class being
>>> passed to the syscall, this is introduced in commit 01584c1e2337("scsi: 
>>> block: Improve ioprio value validity checks").
>>>
>>> Reported-by: Kexin Wei <ys.weikexin@....com>
>>> Cc: Damien Le Moal <dlemoal@...nel.org>
>>> Signed-off-by: Aaron Lu <ziqianlu@...edance.com>
>>> ---
>>> Kexin reported a LTP/ioprio_set03 case failure, where the test would
>>> pass IOPRIO_CLASS_BE with priority level 8 and see if kernel would
>>> return error. Turned out she is using an old kernel header where the
>>> change introduced in commit 01584c1e2337("scsi: block: Improve ioprio
>>> value validity checks") isn't available. During troubleshooting, I find
>>> this priority level test confusing and misleading so I think it should
>>> be removed.
>>
>> What is confusing and misleading about the fact that we support only 8 priority
>> levels (0 to 7) and should check for it ?
> 
> I meant when I'm troubleshooting this LTP issue, I looked at this level
> test and had no idea why it didn't work.

OK. I understand the "confusing" now :)

>> With that said, the test is indeed redundant for the BE and RT class because we
>> have:
>>
>> int ioprio_check_cap(int ioprio)
>> {
>> 	int class = IOPRIO_PRIO_CLASS(ioprio);
>> 	int level = IOPRIO_PRIO_LEVEL(ioprio);
>>
>> And the macro IOPRIO_PRIO_LEVEL() will mask the level value to something between
>> 0 and 7, always. So necessarily, level will always be lower than
>> IOPRIO_NR_LEVELS. So please reword your commit message to explain that rather
>> than describe what a user may or may not use when setting an ioprio field.
> 
> No problem. Does something below look OK to you?
> 
> "
> Ever since commit eca2040972b4("scsi: block: ioprio: Clean up interface
> definition"), the macro IOPRIO_PRIO_LEVEL() will mask the level value to
> something between 0 and 7 so necessarily, level will always be lower than
> IOPRIO_NR_LEVELS.
> 
> Remove this obsolete check.
> "

Yes, looks much better !


-- 
Damien Le Moal
Western Digital Research