Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Date:   Fri, 24 Nov 2023 09:05:37 -0500
From:   Wei Gao <wegao@suse.com>
To:     Damien Le Moal <dlemoal@kernel.org>
Cc:     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
Subject: Re: [PATCH v1] block: ioprio: Fix ioprio_check_cap() validation logic
Message-ID: <ZWCtsVw9cHhmkPf9@wegao>
References: <20231124030525.31426-1-wegao@suse.com>
 <160ecdfc-cb58-47fe-b9ce-fd126acc10fe@kernel.org>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <160ecdfc-cb58-47fe-b9ce-fd126acc10fe@kernel.org>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?TyjHbfQ93uvpinIX2OZOO4nF49XiRSCy/R3O1ZI7ChWiPu7HeNQG9cZIed1k?=
 =?us-ascii?Q?nXJI50I0l1OmsdmpSTFpn+ovs6la9gkCSqRAaHuO/a7SvpwUtwZNYqPHzYsP?=
 =?us-ascii?Q?m5L0NXp5nqlHzF603z+FGelciSS70wUjy//LX08SF7H5RSL5/xSbQU08kVAA?=
 =?us-ascii?Q?YjdrzZyQfeVeFvebbUOUmNly8Hk0cNsOt/ptyCDewiQ+c3FeKqOrUyyqUJaZ?=
 =?us-ascii?Q?PgnjL4R+ENgfeyoL11rYTSe4EF1JiDBtKJnv/Xry9JuBeFlfO//O5c2Chc2b?=
 =?us-ascii?Q?JdZn+WQRbq3KVBmUEYPLZeU4sYLGlgmjplnYqfXsH57he6f7yW9eLHhxsvs5?=
 =?us-ascii?Q?5Axw4C9FZFdMGECHlpVTx4T41N4iKvYwinxOZQ2lssHxtB41OZN83oFvPgTW?=
 =?us-ascii?Q?ozq6rjseRhi6UIGuao11hyOFOHq2wyHCG5mBWo1tVx74o049yPaiXui8CUMv?=
 =?us-ascii?Q?eK/A9kj8eS1tDmb0p0KVt5j/fIhpIHsgUj4iIx/Yfwvif+uS08s2Hv/eJiI+?=
 =?us-ascii?Q?KbXOot3j5j3555+XxvKVneMjyZFRw4auruRfFBbX1hcOoRVcUL1CCtRSDQ7X?=
 =?us-ascii?Q?5vm1aDXsSmnwJCkwhEXhG5/WvBwaoaBnUcgUuCLVyfwuEpurIa0S3Xy1aUGg?=
 =?us-ascii?Q?LoHfZnZuER+VhoONSp0zosKxOiBDbw902x397hQo9m6s5lU7u2ZJ851dVCzS?=
 =?us-ascii?Q?g5EXDT6Iq8egMCiTLLF4kvMLfd1hXVJ4cNYI+jqwSUoyONwu46Dy25VreP5y?=
 =?us-ascii?Q?Y40RMN80WjIt7jbuVGxPct36JAZsCb8B++H3Edw1ru+daL1JCqUfZ6+39ZNP?=
 =?us-ascii?Q?H3KTku2ebBiIwUjplD3Fqhwon+1F0RsiGEZu/QA0YbujgfJISjSYzIjBkUZm?=
 =?us-ascii?Q?Dgu2qs1KAdaHIsSOaBXbfwwvrZmQBTIXp5EArTTLVOvuDpVXFN/j4Y7doAMK?=
 =?us-ascii?Q?VEJnOXcULofkuyU/d/fZpBYWa2v8WQmQLg61tNqwTj6xRRYuGeBxgMUr9wEi?=
 =?us-ascii?Q?y8QMtDOkTzlUL8/3xRWEZha8E4p1zgltjV/dVSr5kYrkxInEtLCZFsh1iyqK?=
 =?us-ascii?Q?iBFshx8P/oqgBdMP190M6f/BWbv2k+bAwSdv2R++0hxpoGm4Hq4h6oFH32at?=
 =?us-ascii?Q?yyAai9/ATdP2SSYDI/J11fRnw2f8+j92Vfr3NPKFoILRqrNfx0kOPCo6Ih2d?=
 =?us-ascii?Q?PTBRpP96OrKipezZK1N2tYH7Heq28JI/H76VPTrG89sihyLrHOKggcXjLA3V?=
 =?us-ascii?Q?pd11tm5Mp6CRZG0woJk8XzmSIT7yAWbY6bbTCSKerDI2efiGzesUMJRg6ETJ?=
 =?us-ascii?Q?t4+wSS6wrty8ZnAizRH6BQcACX2ceCeoIKNomY98ZOMQILbD8eh4BgdkVwet?=
 =?us-ascii?Q?7gqkKllbT8quE5k430QHYTmEZyR3WdtXXfp8O1QN2kLoPFrCWTZnigB2s2L7?=
 =?us-ascii?Q?gwpzGQbd7DQ/8UPzF50fHZ1zFg0AiWNhnTWGXlBW/HtHkriKaURqrBXRN5Ol?=
 =?us-ascii?Q?gyEWWoMlg6x/C7lvbd3T4NzfZuFka5e0k3dYsCl9v91AGwTN5Dv2nBryEftx?=
 =?us-ascii?Q?D8vvywn+12JpdFRJUPM=3D?=
X-OriginatorOrg: suse.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 63cfce9d-aeb1-46b8-9597-08dbecf672ff
X-MS-Exchange-CrossTenant-AuthSource: AS8PR04MB8199.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 24 Nov 2023 14:05:44.3770
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: f7a17af6-1c5c-4a36-aa8b-f5be247aa4ba
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: ptTzZK/AjxDpVWl8WasgOYgtvhV5uW+e6VpJftVJnXd1bM534e3LXhTr9eq/Lu6E
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DU2PR04MB9131
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
	DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,
	SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable
	autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on groat.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (groat.vger.email [0.0.0.0]); Fri, 24 Nov 2023 06:06:04 -0800 (PST)

On Fri, Nov 24, 2023 at 02:53:52PM +0900, Damien Le Moal wrote:
> 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.
> 

Thanks a lot for your quick feedback and detail explaination, if i am guess correctly, 
my test kernel include eca2040972b4 ("scsi: block: ioprio: Clean up interface definition") but 
not include 01584c1e2337 ("scsi: block: Improve ioprio value validity checks") by coincidence.

> >  				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
>