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

Hi John,

I apologize for the delayed response. I may have missed the notifications,
since at the same time, we have some crashes and bug patches working
in parallel. So the response process is a little slower. Thanks for
letting me know
the newest fixup. Apologize again!

Best,
Chao

On Tue, Jul 14, 2026 at 4:18 AM John Garry <[email protected]> wrote:
>
> 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 <[email protected]> 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.
> >>>
> >>
> >>
> >>
>