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

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

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