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

[Chao S]

Hi John, Keith,

Thanks for catching the sparse warning, and sorry for the delayed response.

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

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.

Thanks,
Chao

On Wed, Jun 3, 2026 at 6:08 AM John Garry <john.g.garry at oracle.com> wrote:
>
> On 02/06/2026 17:18, John Garry wrote:
> >
> >> I say we can ignore the sparse warning, or we
> >> can rewrite this to avoid the check_shl_overflow entirely.
> >
>
> FWIW, adding a separate function keeps sparse happy for me:
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index ea837b94d3e5..3ec98038668e 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2364,6 +2364,11 @@ static int nvme_query_fdp_info(struct nvme_ns
> *ns, struct nvme_ns_info *info)
>         return ret;
>   }
>
> +static bool nvme_valid_ds(u64 nsze, signed int shift, u64 *capacity)
> +{
> +       return check_shl_overflow(nsze, shift, capacity);
> +}
> +
>   static int nvme_update_ns_info_block(struct nvme_ns *ns,
>                 struct nvme_ns_info *info)
>   {
> @@ -2407,10 +2412,8 @@ 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)) {
> +       if (nvme_valid_ds(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);
>