Re: [PATCH v3] nvme: core: reject invalid LBA data size from Identify Namespace

[John Garry]

On 02/06/2026 16:42, Keith Busch wrote:
> On Tue, Jun 02, 2026 at 04:15:41PM +0100, Keith Busch wrote:
> > On Tue, Jun 02, 2026 at 02:10:07PM +0100, John Garry wrote:
> > > On 15/05/2026 19:58, Chao Shi wrote:
> > > > +	if (id->lbaf[lbaf].ds < SECTOR_SHIFT ||
> > > > +	    check_shl_overflow(le64_to_cpu(id->nsze),> +			 id->lbaf[lbaf].ds -
> > > SECTOR_SHIFT,
> > > > +			       &capacity)) {
> > > > +		dev_warn_once(ns->ctrl->device,
> > > > +			"invalid LBA data size %u, skipping namespace\n",
> > > > +			id->lbaf[lbaf].ds);
> > > > +		ret = -ENODEV;
> > > > +		goto out;
> > > > +	}
> > >
> > > JFYI, this is giving a C=1 warning:
> > >
> > > drivers/nvme/host/core.c:2411:13: warning: unsigned value that used to  be signed checked against zero?
> > > drivers/nvme/host/core.c:2411:13: signed value source
> > >
> > > I can't seem to quieten it myself, though.
> > >
> > > BTW, I would have thought that check_shl_overflow would catch
> > > id->lbaf[lbaf].ds < SECTOR_SHIFT (so that we don't need the extra check).
> >
> > I see it too. check_shl_overflow has checks that suggest it was
> > expecting a signed type, as all the < 0 checks don't make sense for
> > unsigned. The warning seems harmless, but I'd too like to see it
> > suppressed.
> >
> > I think it's odd that I'm not seeing a similar error for the similar
> > usage in generic_check_addressable() from fs/libfs.c. They look the same
> > to me with respect to the types passed in.
>
> It appears that sparse is having trouble with the type provenance of a
> __bitwise __le64 type. No idea why. As a test, I replaced the
> le64_to_cpu() to a u64 type on stack initialized to a random ULL value
> and the warning goes away.

Yeah, ditto.

> I say we can ignore the sparse warning, or we
> can rewrite this to avoid the check_shl_overflow entirely.

Since sparse is having problems with le64_to_cpu(), I suppose using your 
own version is ok. It would be nice to know the root cause of this 
issue, though...

cheers

> ---
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index cad9d97352615..6409a8218e3eb 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2372,8 +2372,8 @@ static int nvme_update_ns_info_block(struct nvme_ns *ns,
>   	struct nvme_zone_info zi = {};
>   	struct nvme_id_ns *id;
>   	unsigned int memflags;
> -	sector_t capacity;
> -	unsigned lbaf;
> +	unsigned lbaf, shift = 0;
> +	u64 capacity, nsze;
>   	int ret;
>   
>   	ret = nvme_identify_ns(ns->ctrl, info->nsid, &id);
> @@ -2407,10 +2407,13 @@ static int nvme_update_ns_info_block(struct nvme_ns *ns,
>   			goto out;
>   	}
>   
> -	if (id->lbaf[lbaf].ds < SECTOR_SHIFT ||
> -	    check_shl_overflow(le64_to_cpu(id->nsze),
> -			       id->lbaf[lbaf].ds - SECTOR_SHIFT,
> -			       &capacity)) {
> +	nsze = le64_to_cpu(id->nsze);
> +	if (id->lbaf[lbaf].ds >= SECTOR_SHIFT)
> +		shift = id->lbaf[lbaf].ds - SECTOR_SHIFT;
> +
> +	if (shift < SECTOR_SHIFT || shift >= 64 || nsze > U64_MAX >> shift) {
>   		dev_warn_once(ns->ctrl->device,
>   			"invalid LBA data size %u, skipping namespace\n",
>   			id->lbaf[lbaf].ds);



> --