Re: [PATCH v3 02/17] scsi-multipath: introduce scsi_device head structure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Self-deadlock in scsi_mpath_find_head() due to kref_put() triggering release while holding scsi_mpath_heads_lock.
- [High] Double-free and use-after-free of scsi_mpath_head in the error path of scsi_mpath_alloc_head().
- [High] NULL pointer dereference and invalid device_unregister() execution when tearing down a failed device_add() in scsi_mpath_dev_alloc().
--

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

scsi-multipath: introduce scsi_device head structure

This patch introduces a scsi_mpath_head structure to manage multipathing
for a scsi_device. It maintains a list of scsi_mpath_head structures
to look up matching multipathed devices by their unique ID. A new sysfs
device class is also added to manage the scsi_mpath_head device members.

> diff --git a/drivers/scsi/scsi_multipath.c b/drivers/scsi/scsi_multipath.c
> index ff37cfdf2f9d1..65ee3da5cc7fc 100644
> --- a/drivers/scsi/scsi_multipath.c
> +++ b/drivers/scsi/scsi_multipath.c
[ ... ]
> @@ -88,6 +146,71 @@ static int scsi_multipath_sdev_init(struct scsi_device *sdev)
[ ... ]
> +static struct scsi_mpath_head *scsi_mpath_alloc_head(void)
> +{
[ ... ]
> +	ret = dev_set_name(&scsi_mpath_head->dev, "scsi_mpath_device%d",
> +				scsi_mpath_head->index);
> +	if (ret) {
> +		put_device(&scsi_mpath_head->dev);
> +		goto out_free_ida;
> +	}
> +
> +	return scsi_mpath_head;
> +
> +out_free_ida:
> +	ida_free(&scsi_multipath_dev_ida, scsi_mpath_head->index);
> +out_put_head:
> +	mpath_put_head(&scsi_mpath_head->mpath_head);
> +out_free:
> +	kfree(scsi_mpath_head);
> +	return NULL;
> +}

[Severity: High]
Will this cause a double-free on the error path?

If dev_set_name() fails, put_device() is called which triggers the
scsi_mpath_head_release() callback, freeing the IDA and the structure itself.

The code then falls through to the out_free_ida label which calls ida_free()
and kfree() a second time on the already freed pointer.

> +
> +static struct scsi_mpath_head *scsi_mpath_find_head(
> +			struct scsi_mpath_device *scsi_mpath_dev)
> +{
[ ... ]
> +	list_for_each_entry(scsi_mpath_head, &scsi_mpath_heads_list, entry) {
> +		ret = scsi_mpath_get_head(scsi_mpath_head);
> +		if (ret)
> +			continue;
> +		if (strncmp(scsi_mpath_head->vpd_id,
> +			scsi_mpath_dev->device_id_str,
> +			SCSI_MPATH_DEVICE_ID_LEN) == 0) {
> +
> +			return scsi_mpath_head;
> +		}
> +		scsi_mpath_put_head(scsi_mpath_head);
> +	}
> +
> +	return NULL;
> +}
[ ... ]
> @@ -116,13 +240,58 @@ int scsi_mpath_dev_alloc(struct scsi_device *sdev)
[ ... ]
> +	mutex_lock(&scsi_mpath_heads_lock);
> +	scsi_mpath_head = scsi_mpath_find_head(sdev->scsi_mpath_dev);
> +	if (scsi_mpath_head)
> +		goto found;
> +	scsi_mpath_head = scsi_mpath_alloc_head();
> +	if (!scsi_mpath_head) {
> +		sdev_printk(KERN_NOTICE, sdev, "could not allocate multipath head, device multipathing disabled\n");
> +		mutex_unlock(&scsi_mpath_heads_lock);
> +		goto out_uninit;
> +	}
> +
> +	strscpy(scsi_mpath_head->vpd_id, sdev->scsi_mpath_dev->device_id_str,
> +			SCSI_MPATH_DEVICE_ID_LEN);
> +
> +	ret = device_add(&scsi_mpath_head->dev);
> +	if (ret) {
> +		mutex_unlock(&scsi_mpath_heads_lock);
> +		goto out_put_head;
> +	}
> +
> +	list_add_tail(&scsi_mpath_head->entry, &scsi_mpath_heads_list);
> +found:
> +	mutex_unlock(&scsi_mpath_heads_lock);
[ ... ]
> +out_put_head:
> +	scsi_mpath_put_head(scsi_mpath_head);
>  out_uninit:
>  	scsi_multipath_sdev_uninit(sdev);
>  	return ret;
>  }
[ ... ]
> +static void scsi_mpath_free_head(struct kref *ref)
> +{
> +	struct scsi_mpath_head *scsi_mpath_head =
> +		container_of(ref, struct scsi_mpath_head, ref);
> +
[ ... ]
> +	mutex_lock(&scsi_mpath_heads_lock);
> +	list_del_init(&scsi_mpath_head->entry);
> +	mutex_unlock(&scsi_mpath_heads_lock);
> +
> +	device_unregister(&scsi_mpath_head->dev);
> +}

[Severity: High]
Could this lead to an AB-BA self-deadlock?

The scsi_mpath_dev_alloc() function acquires scsi_mpath_heads_lock before
calling scsi_mpath_find_head(). During iteration, scsi_mpath_find_head()
takes a temporary reference to each head.

If a concurrent thread drops its reference to a head just as we process it,
our scsi_mpath_put_head() call might drop the refcount to 0.

This would trigger scsi_mpath_free_head(), which unconditionally tries to
acquire scsi_mpath_heads_lock. Since the non-recursive mutex is already held
by the current thread in scsi_mpath_dev_alloc(), it appears this would freeze
the thread.

[Severity: High]
Is there a risk of a NULL pointer dereference and invalid device unregistration
here if device_add() fails?

If device_add() fails in scsi_mpath_dev_alloc(), we jump to out_put_head and
call scsi_mpath_put_head(). This drops the refcount to 0 and triggers this
cleanup function.

However, the scsi_mpath_head was just allocated with kzalloc and never added
to the list, so entry.next and entry.prev are NULL. Calling list_del_init()
would dereference these NULL pointers.

Furthermore, calling device_unregister() will invoke device_del() on a device
that failed device_add(), which violates the driver core API.

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