On 8/28/24 17:28, Ajay Kaher wrote:
> The commit eca2040972b4 introduced a backward compatibility issue in
> the function ioprio_check_cap.
>
> Before the change, if ioprio contains a level greater than 0x7, it was
> treated as -EINVAL:
>
> data = ioprio & 0x1FFF
> if data >= 0x7, return -EINVAL
>
> Since the change, if ioprio contains a level greater than 0x7 say 0x8
> it is calculated as 0x0:
>
> level = ioprio & 0x7
>
> To maintain backward compatibility the kernel should return -EINVAL in
> the above case as well.
>
> Fixes: eca2040972b4 ("scsi: block: ioprio: Clean up interface definition")
> Signed-off-by: Ajay Kaher <ajay.kaher@xxxxxxxxxxxx>
> ---
> block/ioprio.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/block/ioprio.c b/block/ioprio.c
> index 73301a2..f08e76b 100644
> --- a/block/ioprio.c
> +++ b/block/ioprio.c
> @@ -30,6 +30,15 @@
> #include <linux/security.h>
> #include <linux/pid_namespace.h>
>
> +static inline int ioprio_check_level(int ioprio, int max_level)
> +{
> + int data = IOPRIO_PRIO_DATA(ioprio);
> +
> + if (IOPRIO_BAD_VALUE(data, max_level))
> + return -EINVAL;
No, this cannot possibly work correctly because the prio level part of the prio
data is only 3 bits, so 0 to 7. The remaining 10 bits of the prio data are used
for priority hints (IOPRIO_HINT_XXX).
Your change will thus return an error for cases where the prio data has a level
AND also a hint (e.g. for command duration limits). This change would break
command duration limits. So NACK.
The userspace header file has the ioprio_value() that a user should use to
construct an ioprio. Bad values are checked in that function and errors will be
returned if an invalid level is passed.
> + return 0;
> +}
> +
> int ioprio_check_cap(int ioprio)
> {
> int class = IOPRIO_PRIO_CLASS(ioprio);
> @@ -49,7 +58,7 @@ int ioprio_check_cap(int ioprio)
> fallthrough;
> /* rt has prio field too */
> case IOPRIO_CLASS_BE:
> - if (level >= IOPRIO_NR_LEVELS)
> + if (ioprio_check_level(ioprio, IOPRIO_NR_LEVELS))
> return -EINVAL;
> break;
> case IOPRIO_CLASS_IDLE:
--
Damien Le Moal
Western Digital Research
