iommu/virtio: design question: hot-removal allowed?

[Cam Miller <cam@linux.ibm.com>]

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