ioprio_set can take 8 as the PROCESS CLASS_BE ioprio value

ioprio_set can take 8 as the PROCESS CLASS_BE ioprio value

[Murphy Zhou]

Hi Damien,

Since these commits:

  scsi: block: Introduce ioprio hints
  scsi: block: ioprio: Clean up interface definition

go into linux-next tree, ioprio_set can take the value of 8
as the PROCESS CLASS_BE ioprio parameter, returning
success but actually it is setting to 0 due to the mask roundup.

The LTP test case ioprio_set03[1] covers this boundary value
testing, which starts to fail since then.

This does not look as expected. Could you help to take a look?

Thanks,
Murphy

[1] https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/ioprio/ioprio_set03.c

Re: ioprio_set can take 8 as the PROCESS CLASS_BE ioprio value

[Damien Le Moal]

On 5/26/23 16:27, Murphy Zhou wrote:
> Hi Damien,
> 
> Since these commits:
> 
>   scsi: block: Introduce ioprio hints
>   scsi: block: ioprio: Clean up interface definition
> 
> go into linux-next tree, ioprio_set can take the value of 8
> as the PROCESS CLASS_BE ioprio parameter, returning
> success but actually it is setting to 0 due to the mask roundup.
> 
> The LTP test case ioprio_set03[1] covers this boundary value
> testing, which starts to fail since then.
> 
> This does not look as expected. Could you help to take a look?

Before the patches, the ioprio level of 8 could indeed be set, but that was
actually totally meaningless since the kernel components that use the priority
level all are limited to the range [0..7]. And why the level value 8 could be
seen, the effective level would have been 0. So at least, with the changes, we
are not lying to the user...

I am not sure what this ioprio_set03 test is trying to check.

> 
> Thanks,
> Murphy
> 
> [1] https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/ioprio/ioprio_set03.c

-- 
Damien Le Moal
Western Digital Research

Re: ioprio_set can take 8 as the PROCESS CLASS_BE ioprio value

[Murphy Zhou]

On Fri, May 26, 2023 at 3:42 PM Damien Le Moal <dlemoal@kernel.org> wrote:
>
> On 5/26/23 16:27, Murphy Zhou wrote:
> > Hi Damien,
> >
> > Since these commits:
> >
> >   scsi: block: Introduce ioprio hints
> >   scsi: block: ioprio: Clean up interface definition
> >
> > go into linux-next tree, ioprio_set can take the value of 8
> > as the PROCESS CLASS_BE ioprio parameter, returning
> > success but actually it is setting to 0 due to the mask roundup.
> >
> > The LTP test case ioprio_set03[1] covers this boundary value
> > testing, which starts to fail since then.
> >
> > This does not look as expected. Could you help to take a look?
>
> Before the patches, the ioprio level of 8 could indeed be set, but that was

Before the patches, it can't be set to 8 because the check in
ioprio_check_cap refused it.
   >= IOPRIO_NR_LEVELS
Before the patches, the value can be greater than 8, so it takes effect.
After the patches, the value is limited to [0..7], this check always passes.

> actually totally meaningless since the kernel components that use the priority
> level all are limited to the range [0..7]. And why the level value 8 could be
> seen, the effective level would have been 0. So at least, with the changes, we
> are not lying to the user...
>
> I am not sure what this ioprio_set03 test is trying to check.

I guess it is trying to make sure boundary values do not cause uncertaining.
The test case can be updated according to intended kernel changes. So does
other user space applications that may depend on this, or there is none of
them to worry about.

Thanks,
Murphy
>
> >
> > Thanks,
> > Murphy
> >
> > [1] https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/ioprio/ioprio_set03.c
>
> --
> Damien Le Moal
> Western Digital Research
>

Re: ioprio_set can take 8 as the PROCESS CLASS_BE ioprio value

[Damien Le Moal]

+Jens

and also +Jan as he did some work on ioprio previously.

On 5/27/23 09:02, Murphy Zhou wrote:
> On Fri, May 26, 2023 at 3:42 PM Damien Le Moal <dlemoal@kernel.org> wrote:
>>
>> On 5/26/23 16:27, Murphy Zhou wrote:
>>> Hi Damien,
>>>
>>> Since these commits:
>>>
>>>   scsi: block: Introduce ioprio hints
>>>   scsi: block: ioprio: Clean up interface definition
>>>
>>> go into linux-next tree, ioprio_set can take the value of 8
>>> as the PROCESS CLASS_BE ioprio parameter, returning
>>> success but actually it is setting to 0 due to the mask roundup.
>>>
>>> The LTP test case ioprio_set03[1] covers this boundary value
>>> testing, which starts to fail since then.
>>>
>>> This does not look as expected. Could you help to take a look?
>>
>> Before the patches, the ioprio level of 8 could indeed be set, but that was
> 
> Before the patches, it can't be set to 8 because the check in
> ioprio_check_cap refused it.
>    >= IOPRIO_NR_LEVELS
> Before the patches, the value can be greater than 8, so it takes effect.
> After the patches, the value is limited to [0..7], this check always passes.
> 
>> actually totally meaningless since the kernel components that use the priority
>> level all are limited to the range [0..7]. And why the level value 8 could be
>> seen, the effective level would have been 0. So at least, with the changes, we
>> are not lying to the user...
>>
>> I am not sure what this ioprio_set03 test is trying to check.
> 
> I guess it is trying to make sure boundary values do not cause uncertaining.
> The test case can be updated according to intended kernel changes. So does
> other user space applications that may depend on this, or there is none of
> them to worry about.

The checks before the patch was using IOPRIO_PRIO_DATA() to get the
level value, and that macro was doing a mask with IOPRIO_PRIO_MASK (1 <<
13). So if the application was doing:

IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, 8192 + 1)

then the ioprio value would end up being the same as:

IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, 0)

which is a valid ioprio value. So ioprio_check_cap() would not detect
that case either. The fact that class and level are combined into a
single value essentially prevents us to be exhaustive with the checks in
ioprio_check_cap().

I am not sure if this is worth fixing. as no matter what we do, the
above problem remains: we cannot catch all bad cases and end up with a
valid value which will prevent the user from seeing an error and
catching his/hers invalid use of the ioprio values...

We could do something like shown below, but I am not sure if it is worth
it as their are no guarantees that the user is actually using the
definitions in include/uapi/linux/ioprio.h
(/usr/include/linux/iorprio.h). E.g. see fio code which redefines the
values and macros locally.

diff --git a/include/uapi/linux/ioprio.h b/include/uapi/linux/ioprio.h
index 607c7617b9d2..c09cfbee9117 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
@@ -73,15 +71,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 +96,22 @@ enum {
 	IOPRIO_HINT_DEV_DURATION_LIMIT_7 = 7,
 };

+/*
+ * Return an I/O priority value based on a class, a level and hints.
+ */
+static inline u16 ioprio_value(int class, int level, int hint)
+{
+	if (class < 0 || class >= IOPRIO_NR_CLASSES ||
+	    level < 0 || level >= IOPRIO_NR_LEVELS ||
+	    hint < 0 || hint >= IOPRIO_NR_HINTS)
+		return USHRT_MAX;
+	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 */

-- 
Damien Le Moal
Western Digital Research

Re: ioprio_set can take 8 as the PROCESS CLASS_BE ioprio value

[Murphy Zhou]

On Mon, May 29, 2023 at 10:28 AM Damien Le Moal <dlemoal@kernel.org> wrote:
>
> +Jens
>
> and also +Jan as he did some work on ioprio previously.
>
> On 5/27/23 09:02, Murphy Zhou wrote:
> > On Fri, May 26, 2023 at 3:42 PM Damien Le Moal <dlemoal@kernel.org> wrote:
> >>
> >> On 5/26/23 16:27, Murphy Zhou wrote:
> >>> Hi Damien,
> >>>
> >>> Since these commits:
> >>>
> >>>   scsi: block: Introduce ioprio hints
> >>>   scsi: block: ioprio: Clean up interface definition
> >>>
> >>> go into linux-next tree, ioprio_set can take the value of 8
> >>> as the PROCESS CLASS_BE ioprio parameter, returning
> >>> success but actually it is setting to 0 due to the mask roundup.
> >>>
> >>> The LTP test case ioprio_set03[1] covers this boundary value
> >>> testing, which starts to fail since then.
> >>>
> >>> This does not look as expected. Could you help to take a look?
> >>
> >> Before the patches, the ioprio level of 8 could indeed be set, but that was
> >
> > Before the patches, it can't be set to 8 because the check in
> > ioprio_check_cap refused it.
> >    >= IOPRIO_NR_LEVELS
> > Before the patches, the value can be greater than 8, so it takes effect.
> > After the patches, the value is limited to [0..7], this check always passes.
> >
> >> actually totally meaningless since the kernel components that use the priority
> >> level all are limited to the range [0..7]. And why the level value 8 could be
> >> seen, the effective level would have been 0. So at least, with the changes, we
> >> are not lying to the user...
> >>
> >> I am not sure what this ioprio_set03 test is trying to check.
> >
> > I guess it is trying to make sure boundary values do not cause uncertaining.
> > The test case can be updated according to intended kernel changes. So does
> > other user space applications that may depend on this, or there is none of
> > them to worry about.
>
> The checks before the patch was using IOPRIO_PRIO_DATA() to get the
> level value, and that macro was doing a mask with IOPRIO_PRIO_MASK (1 <<
> 13). So if the application was doing:
>
> IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, 8192 + 1)
>
> then the ioprio value would end up being the same as:
>
> IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, 0)
>
> which is a valid ioprio value. So ioprio_check_cap() would not detect
> that case either. The fact that class and level are combined into a
> single value essentially prevents us to be exhaustive with the checks in
> ioprio_check_cap().
>
> I am not sure if this is worth fixing. as no matter what we do, the
> above problem remains: we cannot catch all bad cases and end up with a
> valid value which will prevent the user from seeing an error and
> catching his/hers invalid use of the ioprio values...

Agree.  We can't prevent that. Please go ahead with any solution that
makes sense to you.

Thanks,
Murphy
>
> We could do something like shown below, but I am not sure if it is worth
> it as their are no guarantees that the user is actually using the
> definitions in include/uapi/linux/ioprio.h
> (/usr/include/linux/iorprio.h). E.g. see fio code which redefines the
> values and macros locally.
>
> diff --git a/include/uapi/linux/ioprio.h b/include/uapi/linux/ioprio.h
> index 607c7617b9d2..c09cfbee9117 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
> @@ -73,15 +71,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 +96,22 @@ enum {
>         IOPRIO_HINT_DEV_DURATION_LIMIT_7 = 7,
>  };
>
> +/*
> + * Return an I/O priority value based on a class, a level and hints.
> + */
> +static inline u16 ioprio_value(int class, int level, int hint)
> +{
> +       if (class < 0 || class >= IOPRIO_NR_CLASSES ||
> +           level < 0 || level >= IOPRIO_NR_LEVELS ||
> +           hint < 0 || hint >= IOPRIO_NR_HINTS)
> +               return USHRT_MAX;
> +       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 */
>
> --
> Damien Le Moal
> Western Digital Research
>

Re: [LTP] ioprio_set can take 8 as the PROCESS CLASS_BE ioprio value

[Murphy Zhou]

Adding ioprio_set03.c author Wallejj.

On Mon, May 29, 2023 at 1:46 PM Murphy Zhou <jencce.kernel@gmail.com> wrote:
>
> On Mon, May 29, 2023 at 10:28 AM Damien Le Moal <dlemoal@kernel.org> wrote:
> >
> > +Jens
> >
> > and also +Jan as he did some work on ioprio previously.
> >
> > On 5/27/23 09:02, Murphy Zhou wrote:
> > > On Fri, May 26, 2023 at 3:42 PM Damien Le Moal <dlemoal@kernel.org> wrote:
> > >>
> > >> On 5/26/23 16:27, Murphy Zhou wrote:
> > >>> Hi Damien,
> > >>>
> > >>> Since these commits:
> > >>>
> > >>>   scsi: block: Introduce ioprio hints
> > >>>   scsi: block: ioprio: Clean up interface definition
> > >>>
> > >>> go into linux-next tree, ioprio_set can take the value of 8
> > >>> as the PROCESS CLASS_BE ioprio parameter, returning
> > >>> success but actually it is setting to 0 due to the mask roundup.
> > >>>
> > >>> The LTP test case ioprio_set03[1] covers this boundary value
> > >>> testing, which starts to fail since then.
> > >>>
> > >>> This does not look as expected. Could you help to take a look?
> > >>
> > >> Before the patches, the ioprio level of 8 could indeed be set, but that was
> > >
> > > Before the patches, it can't be set to 8 because the check in
> > > ioprio_check_cap refused it.
> > >    >= IOPRIO_NR_LEVELS
> > > Before the patches, the value can be greater than 8, so it takes effect.
> > > After the patches, the value is limited to [0..7], this check always passes.
> > >
> > >> actually totally meaningless since the kernel components that use the priority
> > >> level all are limited to the range [0..7]. And why the level value 8 could be
> > >> seen, the effective level would have been 0. So at least, with the changes, we
> > >> are not lying to the user...
> > >>
> > >> I am not sure what this ioprio_set03 test is trying to check.
> > >
> > > I guess it is trying to make sure boundary values do not cause uncertaining.
> > > The test case can be updated according to intended kernel changes. So does
> > > other user space applications that may depend on this, or there is none of
> > > them to worry about.
> >
> > The checks before the patch was using IOPRIO_PRIO_DATA() to get the
> > level value, and that macro was doing a mask with IOPRIO_PRIO_MASK (1 <<
> > 13). So if the application was doing:
> >
> > IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, 8192 + 1)
> >
> > then the ioprio value would end up being the same as:
> >
> > IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, 0)
> >
> > which is a valid ioprio value. So ioprio_check_cap() would not detect
> > that case either. The fact that class and level are combined into a
> > single value essentially prevents us to be exhaustive with the checks in
> > ioprio_check_cap().
> >
> > I am not sure if this is worth fixing. as no matter what we do, the
> > above problem remains: we cannot catch all bad cases and end up with a
> > valid value which will prevent the user from seeing an error and
> > catching his/hers invalid use of the ioprio values...
>
> Agree.  We can't prevent that. Please go ahead with any solution that
> makes sense to you.
>
> Thanks,
> Murphy
> >
> > We could do something like shown below, but I am not sure if it is worth
> > it as their are no guarantees that the user is actually using the
> > definitions in include/uapi/linux/ioprio.h
> > (/usr/include/linux/iorprio.h). E.g. see fio code which redefines the
> > values and macros locally.
> >
> > diff --git a/include/uapi/linux/ioprio.h b/include/uapi/linux/ioprio.h
> > index 607c7617b9d2..c09cfbee9117 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
> > @@ -73,15 +71,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 +96,22 @@ enum {
> >         IOPRIO_HINT_DEV_DURATION_LIMIT_7 = 7,
> >  };
> >
> > +/*
> > + * Return an I/O priority value based on a class, a level and hints.
> > + */
> > +static inline u16 ioprio_value(int class, int level, int hint)
> > +{
> > +       if (class < 0 || class >= IOPRIO_NR_CLASSES ||
> > +           level < 0 || level >= IOPRIO_NR_LEVELS ||
> > +           hint < 0 || hint >= IOPRIO_NR_HINTS)
> > +               return USHRT_MAX;
> > +       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 */
> >
> > --
> > Damien Le Moal
> > Western Digital Research
> >

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: ioprio_set can take 8 as the PROCESS CLASS_BE ioprio value

[Murphy Zhou]

Adding ioprio_set03.c author Wallejj.

On Mon, May 29, 2023 at 1:46 PM Murphy Zhou <jencce.kernel@gmail.com> wrote:
>
> On Mon, May 29, 2023 at 10:28 AM Damien Le Moal <dlemoal@kernel.org> wrote:
> >
> > +Jens
> >
> > and also +Jan as he did some work on ioprio previously.
> >
> > On 5/27/23 09:02, Murphy Zhou wrote:
> > > On Fri, May 26, 2023 at 3:42 PM Damien Le Moal <dlemoal@kernel.org> wrote:
> > >>
> > >> On 5/26/23 16:27, Murphy Zhou wrote:
> > >>> Hi Damien,
> > >>>
> > >>> Since these commits:
> > >>>
> > >>>   scsi: block: Introduce ioprio hints
> > >>>   scsi: block: ioprio: Clean up interface definition
> > >>>
> > >>> go into linux-next tree, ioprio_set can take the value of 8
> > >>> as the PROCESS CLASS_BE ioprio parameter, returning
> > >>> success but actually it is setting to 0 due to the mask roundup.
> > >>>
> > >>> The LTP test case ioprio_set03[1] covers this boundary value
> > >>> testing, which starts to fail since then.
> > >>>
> > >>> This does not look as expected. Could you help to take a look?
> > >>
> > >> Before the patches, the ioprio level of 8 could indeed be set, but that was
> > >
> > > Before the patches, it can't be set to 8 because the check in
> > > ioprio_check_cap refused it.
> > >    >= IOPRIO_NR_LEVELS
> > > Before the patches, the value can be greater than 8, so it takes effect.
> > > After the patches, the value is limited to [0..7], this check always passes.
> > >
> > >> actually totally meaningless since the kernel components that use the priority
> > >> level all are limited to the range [0..7]. And why the level value 8 could be
> > >> seen, the effective level would have been 0. So at least, with the changes, we
> > >> are not lying to the user...
> > >>
> > >> I am not sure what this ioprio_set03 test is trying to check.
> > >
> > > I guess it is trying to make sure boundary values do not cause uncertaining.
> > > The test case can be updated according to intended kernel changes. So does
> > > other user space applications that may depend on this, or there is none of
> > > them to worry about.
> >
> > The checks before the patch was using IOPRIO_PRIO_DATA() to get the
> > level value, and that macro was doing a mask with IOPRIO_PRIO_MASK (1 <<
> > 13). So if the application was doing:
> >
> > IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, 8192 + 1)
> >
> > then the ioprio value would end up being the same as:
> >
> > IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, 0)
> >
> > which is a valid ioprio value. So ioprio_check_cap() would not detect
> > that case either. The fact that class and level are combined into a
> > single value essentially prevents us to be exhaustive with the checks in
> > ioprio_check_cap().
> >
> > I am not sure if this is worth fixing. as no matter what we do, the
> > above problem remains: we cannot catch all bad cases and end up with a
> > valid value which will prevent the user from seeing an error and
> > catching his/hers invalid use of the ioprio values...
>
> Agree.  We can't prevent that. Please go ahead with any solution that
> makes sense to you.
>
> Thanks,
> Murphy
> >
> > We could do something like shown below, but I am not sure if it is worth
> > it as their are no guarantees that the user is actually using the
> > definitions in include/uapi/linux/ioprio.h
> > (/usr/include/linux/iorprio.h). E.g. see fio code which redefines the
> > values and macros locally.
> >
> > diff --git a/include/uapi/linux/ioprio.h b/include/uapi/linux/ioprio.h
> > index 607c7617b9d2..c09cfbee9117 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
> > @@ -73,15 +71,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 +96,22 @@ enum {
> >         IOPRIO_HINT_DEV_DURATION_LIMIT_7 = 7,
> >  };
> >
> > +/*
> > + * Return an I/O priority value based on a class, a level and hints.
> > + */
> > +static inline u16 ioprio_value(int class, int level, int hint)
> > +{
> > +       if (class < 0 || class >= IOPRIO_NR_CLASSES ||
> > +           level < 0 || level >= IOPRIO_NR_LEVELS ||
> > +           hint < 0 || hint >= IOPRIO_NR_HINTS)
> > +               return USHRT_MAX;
> > +       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 */
> >
> > --
> > Damien Le Moal
> > Western Digital Research
> >