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

[John Garry <john.g.garry@oracle.com>]

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@oracle.com> 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.
>>>
>>
>>
>>