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

Hi John,

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.

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