Re: [PATCH v3 10/10] nvme-multipath: switch to use libmultipath

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Diagnostic sysfs attribute functions incorrectly cast the head disk's `private_data` to `struct nvme_ns_head *`, leading to out-of-bounds reads and writes.
- [High] Unconditional clearing of NVME_NS_READY for all paths during revalidation.
- [High] Silent removal of BLK_FEAT_PCI_P2PDMA disables P2P DMA on multipath devices.
- [High] Incorrect teardown function `mpath_put_head` used in `nvme_alloc_ns_head` error path instead of `mpath_head_uninit`.
--

commit 7fcfa1e0451b9e00996c66fccb6e7ea67da8b9ea
Author: John Garry <john.g.garry@oracle.com>

    nvme-multipath: switch to use libmultipath

    Now that much unused libmulipath-based code has been added, do the full
    switch over. The major change is that the multipath management is moved
    out of the nvme_ns_head structure and into the mpath_head structure.

> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 453c1f0b2dd09..476a0cd2452ce 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
[ ... ]
> @@ -3998,22 +3992,32 @@ static struct nvme_ns_head *nvme_alloc_ns_head(struct nvme_ctrl *ctrl,
>  	if (head->ids.csi) {
>  		ret = nvme_get_effects_log(ctrl, head->ids.csi, &head->effects);
>  		if (ret)
> -			goto out_cleanup_srcu;
> +			goto out_ida_free;
>  	} else
>  		head->effects = ctrl->effects;
>  
> +	ret = mpath_head_init(&head->mpath_head);
> +	if (ret)
> +		goto out_ida_free;
> +
> +	head->mpath_head.drv_module = THIS_MODULE;
> +	head->mpath_head.disk_groups = nvme_ns_attr_groups;
> +	head->mpath_head.parent = &subsys->dev;
> +	head->mpath_head.iopolicy = &head->subsys->mpath_iopolicy;
>  
>  	ret = nvme_mpath_alloc_disk(ctrl, head);
>  	if (ret)
> -		goto out_cleanup_srcu;
> +		goto out_mpath_head_free;
>  
>  	list_add_tail(&head->entry, &ctrl->subsys->nsheads);
>  
>  	kref_get(&ctrl->subsys->ref);
>  
>  	return head;
> -out_cleanup_srcu:
> -	cleanup_srcu_struct(&head->srcu);
> -out_ida_remove:
> +
> +out_mpath_head_free:
> +	mpath_put_head(&head->mpath_head);
> +out_ida_free:
>  	ida_free(&ctrl->subsys->ns_ida, head->instance);
>  out_free_head:
>  	kfree(head);

[Severity: High]
Can this error path lead to a NULL pointer dereference or double-free?

If mpath_put_head() triggers mpdt->remove_head(), it might call
list_del_init(&head->entry) on a list that hasn't been added yet (as
list_add_tail() is called later). Should this use mpath_head_uninit() instead
to properly clean up the SRCU struct without prematurely freeing resources?

> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index b27b45c59c883..f4cf2dc2c1e43 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
[ ... ]
> -void nvme_mpath_revalidate_paths(struct nvme_ns_head *head)
> +static void nvme_mpath_revalidate_paths_cb(struct mpath_device *mpath_device)
>  {
> -	sector_t capacity = get_capacity(head->disk);
> -	struct nvme_ns *ns;
> -	int node;
> -	int srcu_idx;
> -
> -	srcu_idx = srcu_read_lock(&head->srcu);
> -	list_for_each_entry_srcu(ns, &head->list, siblings,
> -				 srcu_read_lock_held(&head->srcu)) {
> -		if (capacity != get_capacity(ns->disk))
> -			clear_bit(NVME_NS_READY, &ns->flags);
> -	}
> -	srcu_read_unlock(&head->srcu, srcu_idx);
> +	clear_bit(NVME_NS_READY, &nvme_mpath_to_ns(mpath_device)->flags);
> +}

[Severity: High]
Is it correct to unconditionally clear NVME_NS_READY here?

In the previous implementation, nvme_mpath_revalidate_paths() checked if the
capacity of each path matched the head (capacity != get_capacity(ns->disk))
before clearing the flag. Does this unconditionally disable healthy paths
during capacity revalidation?

[ ... ]
> @@ -1253,24 +661,26 @@ static ssize_t io_requeue_no_usable_path_count_show(struct device *dev,
>  {
>  	struct gendisk *disk = dev_to_disk(dev);
>  	struct nvme_ns_head *head = disk->private_data;
> +	struct mpath_head *mpath_head = &head->mpath_head;
> +	unsigned long val =
> +		atomic_long_read(&mpath_head->requeue_no_usable_path_cnt);
>  
> -	return sysfs_emit(buf, "%lu\n",
> -		    atomic_long_read(&head->io_requeue_no_usable_path_count));
> +	return sysfs_emit(buf, "%lu\n", val);
>  }

[Severity: Critical]
For multipath disks, does disk->private_data now point to struct mpath_head
instead of struct nvme_ns_head?

If so, casting it to struct nvme_ns_head * and evaluating &head->mpath_head
will compute an address past the end of the allocated object. Could this lead
to out-of-bounds reads and writes when reading or writing these sysfs
attributes? 

This also appears to affect io_requeue_no_usable_path_count_store(),
io_fail_no_available_path_count_show(), and 
io_fail_no_available_path_count_store() below.

[ ... ]
> +int nvme_mpath_alloc_disk(struct nvme_ctrl *ctrl, struct nvme_ns_head *head)
> +{
> +	struct queue_limits lim;
> +	int ret;
> +
> +	head->mpath_head.mpdt = &mpdt;
> +
> +	/*
> +	 * If "multipath_always_on" is enabled, a multipath node is added
> +	 * regardless of whether the disk is single/multi ported, and whether
> +	 * the namespace is shared or private. If "multipath_always_on" is not
> +	 * enabled, a multipath node is added only if the subsystem supports
> +	 * multiple controllers and the "multipath" option is configured. In
> +	 * either case, for private namespaces, we ensure that the NSID is
> +	 * unique.
> +	 */
> +	if (!multipath_always_on) {
> +		if (!(ctrl->subsys->cmic & NVME_CTRL_CMIC_MULTI_CTRL) ||
> +				!multipath)
> +			return 0;
> +	}
> +
> +	if (!nvme_is_unique_nsid(ctrl, head))
> +		return 0;
> +
> +	blk_set_stacking_limits(&lim);
> +	lim.dma_alignment = 3;
> +	lim.features |= BLK_FEAT_IO_STAT | BLK_FEAT_NOWAIT |
> +		BLK_FEAT_POLL | BLK_FEAT_ATOMIC_WRITES;

[Severity: High]
Should BLK_FEAT_PCI_P2PDMA be included in the features bitmask here?

Previously, it was added unconditionally to the multipath head's queue limits.
Will its removal cause the block layer to reject PCI Peer-to-Peer DMA bios
submitted to the multipath node?

> +	if (head->ids.csi == NVME_CSI_ZNS)
> +		lim.features |= BLK_FEAT_ZONED;
> +
> +	ret = mpath_alloc_head_disk(&head->mpath_head, &lim, ctrl->numa_node);
> +	if (ret)
> +		return ret;
> +
> +	sprintf(head->mpath_head.disk->disk_name, "nvme%dn%d",
> +			ctrl->subsys->instance, head->instance);
> +	nvme_tryget_ns_head(head);
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703103204.3724406-1-john.g.garry@oracle.com?part=10