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

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

On 03/07/2026 11:49, sashiko-bot@kernel.org wrote:
> 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.

I will fix it

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

I will check it

> 
> [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.
> 
I will check it.