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 <[email protected]> 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/[email protected]?part=1
