Re: [PATCH v2] nvme: skip the zoned limits update if the zone info query failed

On 8/17/26 04:17, Chao Shi wrote:
> nvme_query_zone_info() returns either a negative errno or a positive
> NVMe status code, but nvme_update_ns_info_block() only tests for the

That seems like a bad design of nvme_query_zone_info(). Why not fix it so that
on error it returns either negative values OR NVMe status code? That would
avoid the pitfall of the error check that you found.

> negative case:
> 
> 	ret = nvme_query_zone_info(ns, lbaf, &zi);
> 	if (ret < 0)
> 		goto out;
> 
> If the device fails the Identify Namespace (I/O Command Set specific)
> command, or the Identify Controller command issued by
> nvme_set_max_append(), the positive status falls through and setup
> continues with the zero-initialized zone info.  nvme_update_zone_info()
> then marks the queue zoned with chunk_sectors and ns->head->zsze set to
> zero.
> 
> blk_validate_zoned_limits() does not check chunk_sectors, so the limits
> commit succeeds.  blk_revalidate_disk_zones() does reject the zero zone
> size, but by then the limits are live and nothing rolls them back, so
> I/O keeps being submitted to a zoned queue with a zero zone size and
> disk_zone_no() shifts by ilog2(0):
> 
>   nvme0n1: Invalid non power of two zone size (0)
>   UBSAN: shift-out-of-bounds in include/linux/blkdev.h:747:16
>   shift exponent -1 is negative
>    disk_zone_no include/linux/blkdev.h:747 [inline]
>    bio_straddles_zones include/linux/blkdev.h:1058 [inline]
>    blk_zone_wplug_handle_write block/blk-zoned.c:1423 [inline]
>    blk_zone_plug_bio.cold+0x25/0x1c8 block/blk-zoned.c:1605
>    blk_mq_submit_bio+0x18fb/0x2870 block/blk-mq.c:3196
>    submit_bh_wbc+0x575/0x740 fs/buffer.c:2824
>    __block_write_full_folio+0x728/0xdd0 fs/buffer.c:1933
> 
> Any device, firmware or NVMe-oF target that fails this one command
> reaches this.
> 
> Skip the zoned limits update in that case.  The namespace stays
> registered and usable for admin commands, but the queue is not
> configured from zone info that was never read.
> 
> zi.zone_size is an exact indicator: every path that returns a positive
> status returns before it is assigned, and after that the only failure
> left is -ENODEV, which the caller already handles.
> 
> Fixes: c85c9ab926a5 ("nvme: split nvme_update_zone_info")
> Cc: [email protected]
> Cc: Weidong Zhu <[email protected]>
> Suggested-by: Keith Busch <[email protected]>
> Found by FuzzNvme.
> 
> Signed-off-by: Chao Shi <[email protected]>
> ---
> Changes since v1:
> - Only skip the zoned limits instead of failing the update, as
>   suggested by Keith.
> - Gate on zi.zone_size, not zi.max_open_zones, where 0 is legal
>   (reasoning in my reply on v1).
> - Drop the "malicious device" wording.
> 
> v1: https://lore.kernel.org/linux-nvme/[email protected]/
> 
>  drivers/nvme/host/core.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 453c1f0b2dd0..87e0534cde1c 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2447,8 +2447,13 @@ static int nvme_update_ns_info_block(struct nvme_ns *ns,
>  	if (!nvme_update_disk_info(ns, id, nvm, &lim))
>  		capacity = 0;
>  
> +	/*
> +	 * A failed zone info query leaves zi zero-initialized.  Leave the
> +	 * namespace registered so that it can still be used as a device
> +	 * handle, but do not configure the zoned limits from it.
> +	 */
>  	if (IS_ENABLED(CONFIG_BLK_DEV_ZONED) &&
> -	    ns->head->ids.csi == NVME_CSI_ZNS)
> +	    ns->head->ids.csi == NVME_CSI_ZNS && zi.zone_size)
>  		nvme_update_zone_info(ns, &lim, &zi);
>  
>  	if ((ns->ctrl->vwc & NVME_CTRL_VWC_PRESENT) && !info->no_vwc)


-- 
Damien Le Moal
Western Digital Research