Hi,
Testing on x86, I noticed that userspace supplies a mechanism for hot-removing
a virtio-iommu device.
echo 1 > /sys/bus/pci/devices/<virtio_iommu_dev>/remove
Hot-removing virtio-iommu should NOT be supported, right? The intention is
clear in the QEMU source,
which fences off hot(un)plugging virtio-iommu devices.
If I am correct, then shall we fence off hot-removal (perhaps by simply
omitting this remove file
from sysfs)? Else, keep reading...
...I believe I have found two bugs in viommu_remove() path that I would like to
bring attention to.
The first bug is that the echo above causes the following kernel panic.
BUG: kernel NULL pointer dereference, address: 0000000000000030
--- snip ---
RIP: 0010:sysfs_remove_link_from_group+0x10/0x40
--- snip ---
Call Trace:
<TASK>
--- snip ---
? sysfs_remove_link_from_group+0x10/0x40
iommu_deinit_device+0x2a/0xe0
__iommu_group_remove_device+0xb0/0xe0
? __pfx_remove_iommu_group+0x10/0x10
remove_iommu_group+0x35/0x60
bus_for_each_dev+0x83/0xd0
iommu_device_unregister+0x3d/0xb0
viommu_remove+0x25/0x60
virtio_dev_remove+0x3c/0x90
--- snip ---
Here is a fix:
diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
index 587fc13197f1..a08474eeed58 100644
--- a/drivers/iommu/virtio-iommu.c
+++ b/drivers/iommu/virtio-iommu.c
@@ -1254,8 +1254,8 @@ static void viommu_remove(struct virtio_device *vdev)
{
struct viommu_dev *viommu = vdev->priv;
- iommu_device_sysfs_remove(&viommu->iommu);
iommu_device_unregister(&viommu->iommu);
+ iommu_device_sysfs_remove(&viommu->iommu);
/* Stop all virtqueues */
virtio_reset_device(vdev);
Other IOMMU drivers order iommu_device_unregister() before
iommu_device_sysfs_remove(). I would be
happy to elaborate on how exactly I believe that the reverse ordering leads to
the NULL pointer
dereference, if desired.
After applying the fix shown above, a second problem is exposed. Unregistering
the IOMMU while
endpoints remain active is like pulling out the rug from underneath those
endpoints. The following
warning catches this case.
WARNING: drivers/iommu/iommu.c:743 at
__iommu_group_free_device.constprop.0+0xb9/0xe0, CPU#1:
bash/1250
--- snip ---
Call Trace:
<TASK>
__iommu_group_remove_device+0x95/0xe0
? __pfx_remove_iommu_group+0x10/0x10
remove_iommu_group+0x39/0x60
bus_for_each_dev+0x85/0xd0
iommu_device_unregister+0x3d/0xb0
viommu_remove+0x1d/0x60
--- snip ---
As a consequence, endpoint drivers attempting IO hang.
igb 0000:01:00.0 enp1s0: NETDEV WATCHDOG: CPU: 5: transmit queue 2 timed out
5120 ms
igb 0000:01:00.0 enp1s0: Reset adapter
igb 0000:01:00.0 enp1s0: igb: enp1s0 NIC Link is Up 1000 Mbps Full Duplex,
Flow Control: RX/TX
clocksource: Watchdog remote CPU 7 read timed out
igb 0000:01:00.0: Detected Tx Unit Hang
Moreover shutting down the system at this point leads to a kernel panic. I
believe treating stale
IOVAs as a physical addresses is what causes the page fault.
BUG: unable to handle page fault for address: ffffee9903ffffc0
--- snip ---
RIP: 0010:dma_free_contiguous+0x59/0xb0
--- snip ---
Call Trace:
<TASK>
__igb_close.isra.0+0xb0/0x1c0 [igb]
__igb_shutdown.isra.0+0x27a/0x310 [igb]
igb_shutdown+0x2f/0x90 [igb]
pci_device_shutdown+0x35/0x60
device_shutdown+0x118/0x1b0
kernel_power_off+0x35/0x80
__do_sys_reboot+0x1d4/0x240
do_syscall_64+0xbe/0x470
? vfs_writev+0xe8/0x3c0
? do_writev+0x7f/0x110
? do_writev+0x7f/0x110
? do_syscall_64+0xfa/0x470
? do_user_addr_fault+0x2ae/0x7b0
? irqentry_exit+0x40/0x730
? do_syscall_64+0x37/0x470
? __irq_exit_rcu+0x4c/0x110
entry_SYSCALL_64_after_hwframe+0x76/0x7e
--- snip ---
As a crude test, I added a loop to the beginning of iommu_device_unregister()
that calls
device_release_driver() on each endpoint. With this change, no longer do the
kernel panics/warnings
occur. But still, obviously all PCI IO is remains broken at this point.
...Point being that hot-removing virtio-iommu is currently broken.
In summary:
- Should hot-removal be supported/allowed?
- If no, then how shall we fence it off? Via sysfs?
- Else, there are some bugs that need to be fixed (and I would be happy to work
on that if that is
not already evident).
Thank you,
Cam
