|WARNING| [v2,10/10] bus/vmbus: support unplug

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: WARNING
https://dpdk.org/patch/165694

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-06-18

# DPDK Patch Review

## Patch 01/10: bus: fix reference to plug callback

**No issues found.**

---

## Patch 02/10: dma/idxd: remove next pointer in bus specific device

**No issues found.**

---

## Patch 03/10: bus/vdev: remove driver setting in probe

**No issues found.**

---

## Patch 04/10: bus/pci: fix mapping leak in bus cleanup

### Errors

**Resource leak on error path:**
```c
static int
pci_cleanup(void)
{
	...
	RTE_BUS_FOREACH_DEV(dev, &rte_pci_bus) {
		int ret = 0;

		if (rte_dev_is_probed(&dev->device)) {
			ret = pci_unplug_device(&dev->device);
			if (ret < 0) {
				rte_errno = errno;
				error = -1;
			}
		}

		rte_devargs_remove(dev->device.devargs);
		rte_bus_remove_device(&rte_pci_bus, &dev->device);
		pci_free(RTE_PCI_DEVICE_INTERNAL(dev));  // LEAK: freed even if unplug failed
	}
	...
}
```

When `pci_unplug_device()` fails, the function sets `error = -1` but continues to free the device structure via `pci_free()`. The device was not successfully cleaned up (unmap/intr cleanup failed), yet its memory is freed. This leaves the device in an inconsistent state -- references may still exist (e.g., in driver structures, interrupt subsystems) but the device object is gone. This is a use-after-free waiting to happen if any subsequent code tries to access the device.

**Fix:** Only proceed with `rte_devargs_remove()`, `rte_bus_remove_device()`, and `pci_free()` when `pci_unplug_device()` succeeds. If unplug fails, log the error and skip freeing the device.

```c
if (rte_dev_is_probed(&dev->device)) {
	ret = pci_unplug_device(&dev->device);
	if (ret < 0) {
		rte_errno = errno;
		error = -1;
		continue;  // Skip freeing on failure
	}
}

rte_devargs_remove(dev->device.devargs);
rte_bus_remove_device(&rte_pci_bus, &dev->device);
pci_free(RTE_PCI_DEVICE_INTERNAL(dev));
```

### Warnings

**Missing interrupt handle cleanup on error path in `pci_cleanup()`:**

The patch adds `rte_pci_unmap_device()` to `pci_cleanup()` via the call to `pci_unplug_device()`, which is correct. However, patch 04 shows the original `pci_cleanup()` code that freed interrupt handles (`dev->intr_handle` and `dev->vfio_req_intr_handle`) -- these lines are removed in the refactored version without being moved into `pci_unplug_device()`. The patch does not show `pci_unplug_device()` freeing these interrupt handles either. If they are not freed anywhere, this is a resource leak.

**Verify** that `pci_unplug_device()` (or a later patch in the series) frees both `dev->intr_handle` and `dev->vfio_req_intr_handle`. If not, add them to the unplug path.

---

## Patch 05/10: bus/vmbus: fix interrupt leak in cleanup

**No issues found.** The patch correctly adds `rte_intr_instance_free(dev->intr_handle)` to the cleanup path.

---

## Patch 06/10: bus/vmbus: allocate interrupt during probing

### Errors

**Use-after-free on error path:**
```c
static int
vmbus_probe_device(struct rte_driver *drv, struct rte_device *dev)
{
	...
	/* allocate interrupt handle instance */
	vmbus_dev->intr_handle =
		rte_intr_instance_alloc(RTE_INTR_INSTANCE_F_PRIVATE);
	if (vmbus_dev->intr_handle == NULL)
		return -ENOMEM;

	/* map resources for device */
	ret = rte_vmbus_map_device(vmbus_dev);
	if (ret != 0)
		goto free_intr;

	...
	ret = vmbus_drv->probe(vmbus_drv, vmbus_dev);
	if (ret != 0)
		goto unmap;

	return 0;

unmap:
	rte_vmbus_unmap_device(vmbus_dev);
free_intr:
	rte_intr_instance_free(vmbus_dev->intr_handle);
	vmbus_dev->intr_handle = NULL;

	return ret;
}
```

If `vmbus_drv->probe()` fails, the code jumps to `unmap:`, which calls `rte_vmbus_unmap_device()`, then falls through to `free_intr:` which frees the interrupt handle. However, `rte_vmbus_unmap_device()` may internally **access** `vmbus_dev->intr_handle` (this is common in device mapping/unmapping code that interacts with interrupt subsystems). If so, the handle is used after being freed on this error path.

**Verify** the implementation of `rte_vmbus_unmap_device()`:
- If it does NOT access `intr_handle`, the code is safe.
- If it DOES access `intr_handle`, then the cleanup order is wrong. The handle should be freed **after** unmap completes.

If `rte_vmbus_unmap_device()` accesses the interrupt handle, reorder cleanup:

```c
unmap:
	rte_vmbus_unmap_device(vmbus_dev);
	rte_intr_instance_free(vmbus_dev->intr_handle);
	vmbus_dev->intr_handle = NULL;
	return ret;

free_intr:
	rte_intr_instance_free(vmbus_dev->intr_handle);
	vmbus_dev->intr_handle = NULL;
	return ret;
```

Or collapse to a single cleanup label if unmap is safe to call even when mapping was not attempted (check unmap implementation).

---

## Patch 07/10: bus: align unplug with device probe

### Errors

**Double-free in `vdev_cleanup()`:**
```c
static int
vdev_cleanup(struct rte_bus *bus)
{
	int error;

	rte_spinlock_recursive_lock(&vdev_device_list_lock);
	error = rte_bus_generic_cleanup(bus);  // This calls vdev_unplug_device() for each device
	rte_spinlock_recursive_unlock(&vdev_device_list_lock);

	return error;
}
```

And `rte_bus_generic_cleanup()` from patch 08 (but visible in this patch's context):
```c
int
rte_bus_generic_cleanup(struct rte_bus *bus)
{
	...
	while ((dev = TAILQ_FIRST(&bus->device_list)) != NULL) {
		if (rte_dev_is_probed(dev)) {
			if (bus->unplug_device && bus->unplug_device(dev) < 0) {
				...
			}
		}

		rte_devargs_remove(dev->devargs);
		rte_bus_remove_device(bus, dev);
		bus->free_device(dev);  // Calls vdev_free_device() -> free(dev)
	}
	...
}
```

The lock is held **across** the call to `rte_bus_generic_cleanup()`, which iterates the device list and frees each device. However, `rte_bus_remove_device()` modifies the `bus->device_list` TAILQ while the caller (`vdev_cleanup()`) holds the lock that protects this list. If `rte_bus_remove_device()` tries to acquire the same lock (recursive spinlock allows this, so it may succeed), there is no issue. But if the lock nesting is wrong or if a different code path later tries to acquire the lock while holding a reference to a device that has just been freed, this could cause corruption.

More critically: `vdev_cleanup()` locks, calls generic cleanup which iterates and frees all devices while removing them from the list, then unlocks. The patch shows the old `vdev_cleanup()` also held the lock across the loop. The pattern is acceptable **if and only if** no code outside this function accesses the device list while cleanup is running. Since this is a cleanup function called during shutdown (`rte_eal_cleanup()`), this is likely safe, but the lock adds no value here -- the list is being destroyed.

**Not an error** on second thought, but fragile. The lock protects against concurrent access during cleanup, but cleanup should be the final operation, so no concurrent access should occur. The lock may be defensive.

**No correctness bug** here, but note: holding the lock during iteration and freeing is fine if the lock is recursive and no other code is running. However, the old code also did this, so the patch does not introduce the issue.

---

## Patch 08/10: bus: implement cleanup in EAL

### Errors

**Missing interrupt handle cleanup in PCI unplug:**

Patch 04 removed the lines that freed `dev->intr_handle` and `dev->vfio_req_intr_handle` from `pci_cleanup()`. Patch 08 refactors cleanup to use `rte_bus_generic_cleanup()`, which calls `pci_unplug_device()`. However, `pci_unplug_device()` as shown in patch 07 only does:

```c
static int
pci_unplug_device(struct rte_device *rte_dev)
{
	...
	if (dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING)
		rte_pci_unmap_device(dev);

	return 0;
}
```

It does **not** free the interrupt handles. The original `pci_cleanup()` in patch 04 had:

```c
		rte_intr_instance_free(dev->intr_handle);
		dev->intr_handle = NULL;
		rte_intr_instance_free(dev->vfio_req_intr_handle);
		dev->vfio_req_intr_handle = NULL;
```

If these lines are not added to `pci_unplug_device()` by this patch or a later one, interrupt handles are leaked.

**Fix:** Add interrupt handle cleanup to `pci_unplug_device()`:

```c
static int
pci_unplug_device(struct rte_device *rte_dev)
{
	struct rte_pci_device *dev = RTE_BUS_DEVICE(rte_dev, *dev);
	const struct rte_pci_driver *dr = RTE_BUS_DRIVER(dev->device.driver, *dr);
	int ret = 0;

	if (dr != NULL && dr->remove != NULL) {
		ret = dr->remove(dev);
		if (ret)
			return ret;
	}

	if (dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING)
		rte_pci_unmap_device(dev);

	rte_intr_instance_free(dev->intr_handle);
	dev->intr_handle = NULL;
	rte_intr_instance_free(dev->vfio_req_intr_handle);
	dev->vfio_req_intr_handle = NULL;

	return 0;
}
```

---

## Patch 09/10: bus/vmbus: store name in bus specific device

**No issues found.** The patch eliminates dynamic allocation of the device name by embedding a fixed-size array, simplifying cleanup.

---

## Patch 10/10: bus/vmbus: support unplug

**No issues found.**

---

## Summary

### Critical Issues (Errors)

1. **Patch 04**: `pci_cleanup()` frees device structure even when `pci_unplug_device()` fails, leaving device in inconsistent state (use-after-free risk).
2. **Patch 06**: `vmbus_probe_device()` error path may use interrupt handle after freeing it if `rte_vmbus_unmap_device()` accesses `intr_handle`.
3. **Patch 08**: PCI interrupt handles (`intr_handle`, `vfio_req_intr_handle`) are not freed in `pci_unplug_device()`, causing a resource leak.

### Warnings

1. **Patch 04**: Verify that the refactored code frees PCI interrupt handles (they were removed from `pci_cleanup()` and must appear in `pci_unplug_device()` or a later patch).

### Info

- All patches follow DPDK code style and naming conventions.
- API changes are documented in release notes (patch 10).
- The series correctly refactors bus cleanup to use a generic EAL helper, reducing code duplication.