Re: [PATCH v5 07/27] libmultipath: Add sysfs helpers

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mpath_alloc_head_disk leaves mpath_head->disk set to an ERR_PTR on blk_alloc_disk failure, leading to a crash in mpath_put_disk.
- [Medium] Updating the multipath iopolicy via mpath_iopolicy_store() does not clear the existing current_path arrays, resulting in stale path selection.
--

--- Patch [7]: [PATCH v5 07/27] libmultipath: Add sysfs helpers ---
commit fdada4b0515eb9327fe00efd638199926cadf8fd
Author: John Garry <[email protected]>

libmultipath: Add sysfs helpers

Add helpers for driver sysfs code for the following functionality:
- get/set iopolicy with mpath_iopolicy_store() and mpath_iopolicy_show()
- show device path per NUMA node
- "multipath" attribute group, equivalent to nvme_ns_mpath_attr_group
- device groups attribute array, similar to nvme_ns_attr_groups but not
  containing NVMe members.

> diff --git a/lib/multipath.c b/lib/multipath.c
> index ede06e5e901c8..c78d46a446cf3 100644
> --- a/lib/multipath.c
> +++ b/lib/multipath.c

[ ... ]

> +bool mpath_iopolicy_store(enum mpath_iopolicy_e *iopolicy, const char *buf)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(mpath_iopolicy_names); i++) {
> +		if (sysfs_streq(buf, mpath_iopolicy_names[i])) {
> +			mpath_iopolicy_update(iopolicy, i);
> +			return true;
> +		}
> +	}
> +
> +	return false;
> +}

[Severity: Medium]
When updating the multipath iopolicy via mpath_iopolicy_store(), does this
leave stale paths in the current_path arrays?

If the policy is changed (e.g., from round-robin to NUMA), the policy enum is
updated but the cached current_path is not cleared. When mpath_numa_path()
subsequently executes, it reads the stale current_path. As long as the stale
path is MPATH_STATE_OPTIMIZED, the NUMA selector might accept it without ever
evaluating node_distance to find the true optimal NUMA path:

lib/multipath.c:mpath_numa_path()
    mpath_device = srcu_dereference(mpath_head->current_path[node],
                                    &mpath_head->srcu);
    if (unlikely(!mpath_device))
        return __mpath_find_path(mpath_head, node);
    if (unlikely(mpath_path_is_disabled(mpath_head, mpath_device)))
        return __mpath_find_path(mpath_head, node);
    if (unlikely(!mpath_path_is_optimized(mpath_head, mpath_device)))
        return __mpath_find_path(mpath_head, node);
    return mpath_device;

Outside of the diff provided in this patch, there is another issue in the
broader multipath code to consider.

[Severity: High]
If blk_alloc_disk() fails in mpath_alloc_head_disk() (e.g., due to ENOMEM),
mpath_head->disk is assigned an ERR_PTR but is not reset to NULL before
returning the error:

lib/multipath.c:mpath_alloc_head_disk()
    mpath_head->disk = blk_alloc_disk(lim, numa_node);
    if (IS_ERR(mpath_head->disk))
        return PTR_ERR(mpath_head->disk);

If the caller handles this failure by invoking mpath_put_disk(), the cleanup
function assumes any non-NULL value is valid and calls put_disk() on the
ERR_PTR, which could lead to a crash:

lib/multipath.c:mpath_put_disk()
    if (!mpath_head->disk)
        return;

    /* make sure all pending bios are cleaned up */
    kblockd_schedule_work(&mpath_head->requeue_work);
    flush_work(&mpath_head->requeue_work);
    flush_work(&mpath_head->partition_scan_work);
    put_disk(mpath_head->disk);

Should mpath_head->disk be set to NULL on the blk_alloc_disk() error path?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7