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

[John Garry]

This response took almost 3 weeks. The previous response took again 
almost 3 weeks. kernel development may be relatively slow moving, but 
it's not that slow. You need to respond much more promptly to keep up 
with current development.

> Here is the concrete change (drivers/nvme/host/core.c,
> nvme_update_ns_info_block()):
>
>    unsigned int memflags;
>    sector_t capacity;
>    unsigned lbaf;
> + u64 nsze;
>    int ret;
> ...
> - if (id->lbaf[lbaf].ds < SECTOR_SHIFT ||
> -    check_shl_overflow(le64_to_cpu(id->nsze),
> -       id->lbaf[lbaf].ds - SECTOR_SHIFT,
> -       &capacity)) {
> + /*
> + * check_shl_overflow() also rejects a data size below SECTOR_SHIFT,
> + * as that makes the shift count negative, so no separate lower-bound
> + * test is needed.  Feed nsze through a plain u64 so sparse does not
> + * trip over the __le64 provenance of le64_to_cpu().
> + */
> + nsze = le64_to_cpu(id->nsze);
> + if (check_shl_overflow(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;
>    }
>
> Two things:
>
> 1. Drops the explicit ds < SECTOR_SHIFT test as redundant --
>     check_shl_overflow() already returns true for the resulting negative
>     shift. I confirmed ds=0 and ds=8 are still rejected without it.
>
> 2. Routes le64_to_cpu(id->nsze) through a plain u64 local, which is what
>     silences the C=1 warning (sparse loses the __le64 provenance when the
>     value is passed straight into check_shl_overflow()).
>
> Behavior is otherwise unchanged. Since the original is already in
> v7.2-rc1, I'll send this as a standalone patch on top of mainline
> (Reported-by: you) unless you'd prefer a different form.

My fix is now in mainline 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v7.2-rc3&id=92f58587a04c94985fd4a9e3575720b054c432bf, 
so any change which you want to make must be on top of that.

> Thanks,
> Chao
>
> On Wed, Jun 24, 2026 at 9:15 AM John Garry <john.g.garry@xxxxxxxxxx> wrote:
> > On 23/06/2026 21:37, Chao S wrote:
> > > > BTW, I have thought that check_shl_overflow would catch
> > > > id->lbaf[lbaf].ds < SECTOR_SHIFT (so that we don't need the extra check).
> > > Confirmed -- check_shl_overflow() returns true for the negative shift
> > > that ds < SECTOR_SHIFT produces (_to_shift collapses to 0 and the
> > > _to_shift != _s test fires). I checked ds=0 and ds=8: both are still
> > > rejected with the explicit lower-bound test removed, so it is redundant.
> > >
> > > For the C=1 warning, the minimal fix is to drop that redundant check and
> > > feed nsze through a plain u64 local -- as Keith found, laundering the
> > > le64_to_cpu() result through a non-__le64 type makes the warning go away:
> > >
> > > u64 nsze;
> > > ...
> > > nsze = le64_to_cpu(id->nsze);
> > > if (check_shl_overflow(nsze, id->lbaf[lbaf].ds - SECTOR_SHIFT,
> > >         &capacity)) {
> > > dev_warn_once(...);
> > > ret = -ENODEV;
> > > goto out;
> > > }
> > >
> > > This keeps check_shl_overflow() in one tested helper and avoids a
> > > wrapper. John's nvme_valid_ds() works too; if we prefer that, I'd name it
> > > for its actual sense (it returns true on overflow, i.e. invalid), e.g.
> > > nvme_ds_overflows().
> > >
> > > One note: I'd lean toward keeping check_shl_overflow() rather than
> > > open-coding the bound. It folds the lower-bound (negative shift) and the
> > > overflow case into one tested helper, so we don't have to re-derive the
> > > boundary by hand -- e.g. the lower bound is on ds itself, not on the
> > > post-subtract (ds - SECTOR_SHIFT) shift, which I found easy to get
> > > subtly wrong.
> >
> > What exactly is your proposed change (to what is in the tree)?
> >
> > > Keith, since v3 is already in your tree: do you want an incremental fixup
> > > on top, or a v4 to replace the applied commit? I have both ready.