Re: [PATCH] hv/vmbus: copy dynamic IDs before dropping the lock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] hv/vmbus: NULL dev_id passed to drv->probe() in vmbus_probe() on dynid race
--

commit 3bc8de7f762094d04ea5ef5ada2811cd26424021
Author: Jiale Yao <yaojiale02@163.com>

hv/vmbus: copy dynamic IDs before dropping the lock

This commit copies matching dynamic device IDs into a caller-provided stack
variable while holding drv->dynids.lock so that driver probe callbacks do not
receive a pointer to a dynamic ID entry that was concurrently freed by
remove_id_store().

> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 4692687cdbf3b..96ad0dfaa50ba 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c

[ ... ]

> @@ -860,9 +864,10 @@ static int vmbus_probe(struct device *child_device)
>  	struct hv_driver *drv =
>  			drv_to_hv_drv(child_device->driver);
>  	struct hv_device *dev = device_to_hv_device(child_device);
> +	struct hv_vmbus_device_id dev_id_copy;
>  	const struct hv_vmbus_device_id *dev_id;
>  
> -	dev_id = hv_vmbus_get_id(drv, dev);
> +	dev_id = hv_vmbus_get_id(drv, dev, &dev_id_copy);
>  	if (drv->probe) {
>  		ret = drv->probe(dev, dev_id);
>  		if (ret != 0)

[Severity: High]
What happens if remove_id_store() removes the matching dynamic ID after
vmbus_match() succeeds, but before vmbus_probe() calls hv_vmbus_get_id()?

Since vmbus_match() and vmbus_probe() acquire and release drv->dynids.lock
separately via hv_vmbus_get_id() -> hv_vmbus_dynid_match(), a concurrent
remove_id_store() can delete the matching entry in between:

drivers/hv/vmbus_drv.c:remove_id_store() {
	...
	spin_lock(&drv->dynids.lock);
	list_for_each_entry_safe(dynid, n, &drv->dynids.list, node) {
		struct hv_vmbus_device_id *id = &dynid->id;

		if (guid_equal(&id->guid, &guid)) {
			list_del(&dynid->node);
			kfree(dynid);
			retval = count;
			break;
		}
	}
	spin_unlock(&drv->dynids.lock);
	...
}

When that happens (or if driver_override is cleared between vmbus_match()
and vmbus_probe()), hv_vmbus_get_id() in vmbus_probe() returns NULL, and
vmbus_probe() passes dev_id = NULL to drv->probe(dev, dev_id).

Drivers such as storvsc and hv_util unconditionally dereference dev_id in
their probe callbacks:

drivers/scsi/storvsc_drv.c:storvsc_probe() {
	...
	bool dev_is_ide = dev_id->driver_data == IDE_GUID;
	bool is_fc = dev_id->driver_data == SFC_GUID;
	...
}

drivers/hv/hv_util.c:util_probe() {
	struct hv_util_service *srv =
		(struct hv_util_service *)dev_id->driver_data;
	...
}

And for drivers that do not dereference dev_id (such as uio_hv_generic),
probe will still succeed and bind the device even though the dynamic ID was
already removed.

Should vmbus_probe() check if (!dev_id && !is_hvsock_channel(dev->channel))
and return -ENODEV before calling drv->probe(), similar to how
__pci_device_probe() and usb_probe_interface() handle a removed dynamic ID?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019085544.1834700-1-yaojiale02@163.com?part=1