Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
Message-ID: <160ecdfc-cb58-47fe-b9ce-fd126acc10fe@kernel.org>
Date:   Fri, 24 Nov 2023 14:53:52 +0900
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v1] block: ioprio: Fix ioprio_check_cap() validation logic
To:     Wei Gao <wegao@suse.com>, axboe@kernel.dk, hare@suse.de,
        hch@lst.de, niklas.cassel@wdc.com, martin.petersen@oracle.com,
        linux-block@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20231124030525.31426-1-wegao@suse.com>
Content-Language: en-US
From:   Damien Le Moal <dlemoal@kernel.org>
Organization: Western Digital Research
In-Reply-To: <20231124030525.31426-1-wegao@suse.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 11/24/23 12:05, Wei Gao wrote:
> The current logic "if (level >= IOPRIO_NR_LEVELS)" can not be reached since
> level value get from IOPRIO_PRIO_LEVEL ONLY extract lower 3-bits of ioprio.
> (IOPRIO_NR_LEVELS=8)
> 
> So this trigger LTP test case ioprio_set03 failed, the test case expect
> error when set IOPRIO_CLASS_BE prio 8, in current implementation level
> value will be 0 and obviously can not return error.
> 
> Fixes: eca2040972b4 ("scsi: block: ioprio: Clean up interface definition")

No. Please see below.

> Signed-off-by: Wei Gao <wegao@suse.com>
> ---
>  block/ioprio.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/block/ioprio.c b/block/ioprio.c
> index b5a942519a79..f83029208f2a 100644
> --- a/block/ioprio.c
> +++ b/block/ioprio.c
> @@ -33,7 +33,7 @@
>  int ioprio_check_cap(int ioprio)
>  {
>  	int class = IOPRIO_PRIO_CLASS(ioprio);
> -	int level = IOPRIO_PRIO_LEVEL(ioprio);
> +	int data = IOPRIO_PRIO_DATA(ioprio);
>  
>  	switch (class) {
>  		case IOPRIO_CLASS_RT:
> @@ -49,13 +49,13 @@ int ioprio_check_cap(int ioprio)
>  			fallthrough;
>  			/* rt has prio field too */
>  		case IOPRIO_CLASS_BE:
> -			if (level >= IOPRIO_NR_LEVELS)
> +			if (data >= IOPRIO_NR_LEVELS || data < 0)

This is incorrect: data is the combination of level AND hints, so that value can
be larger than or equal to 8 with the level still being valid. Hard NACK on this.

The issue with LTP test case has been fixed in LTP and by changing the ioprio.h
header file. See commit 01584c1e2337 ("scsi: block: Improve ioprio value
validity checks") which introduces IOPRIO_BAD_VALUE() macro for that.

And for ltp, the commits are:
6b7f448fe392 ("ioprio: Use IOPRIO_PRIO_NUM to check prio range")
7c84fa710f75 ("ioprio: use ioprio.h kernel header if it exists")

So please update your setup, including your install of kernel user API header files.

>  				return -EINVAL;
>  			break;
>  		case IOPRIO_CLASS_IDLE:
>  			break;
>  		case IOPRIO_CLASS_NONE:
> -			if (level)
> +			if (data)
>  				return -EINVAL;
>  			break;
>  		case IOPRIO_CLASS_INVALID:

-- 
Damien Le Moal
Western Digital Research