[PATCH] iommu/amd: Clear DMA ops when switching domain

* [PATCH] iommu/amd: Clear DMA ops when switching domain
@ 2021-04-22  9:42 Jean-Philippe Brucker
  2021-05-18  9:19 ` Joerg Roedel
  0 siblings, 1 reply; 2+ messages in thread
From: Jean-Philippe Brucker @ 2021-04-22  9:42 UTC (permalink / raw)
  To: joro; +Cc: robin.murphy, iommu, will, Jean-Philippe Brucker

Since commit 08a27c1c3ecf ("iommu: Add support to change default domain
of an iommu group") a user can switch a device between IOMMU and direct
DMA through sysfs. This doesn't work for AMD IOMMU at the moment because
dev->dma_ops is not cleared when switching from a DMA to an identity
IOMMU domain. The DMA layer thus attempts to use the dma-iommu ops on an
identity domain, causing an oops:

  # echo 0000:00:05.0 > /sys/sys/bus/pci/drivers/e1000e/unbind
  # echo identity > /sys/bus/pci/devices/0000:00:05.0/iommu_group/type
  # echo 0000:00:05.0 > /sys/sys/bus/pci/drivers/e1000e/bind
   ...
  BUG: kernel NULL pointer dereference, address: 0000000000000028
   ...
   Call Trace:
    iommu_dma_alloc
    e1000e_setup_tx_resources
    e1000e_open

Since iommu_change_dev_def_domain() calls probe_finalize() again, clear
the dma_ops there like Vt-d does.

Fixes: 08a27c1c3ecf ("iommu: Add support to change default domain of an iommu group")
Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>

---
It could be factored into iommu_setup_dma_ops(), but this is easier to
backport and less likely to break other platforms. Since I need the same
for virtio-iommu, I'll do the factoring there.

My previous attempt at fixing this by implementing
arch_teardown_dma_ops() [1] was misguided. It's how arm64 deals with the
problem but it cannot reliably work on x86, because there the DMA ops
are set on device add, not on driver bind.

Thankfully Boris reported a regression on his test box and dropped that
patch [2]. Although I couldn't reproduce it I'm guessing what happens
is:

* probe_finalize(), called from device_add() or bus_set_iommu() sets up
  the DMA ops.
* Some driver, possibly ata_generic, probes the device and returns
  -ENODEV. arch_teardown_dma_ops() clears the DMA ops.
* Another driver probes the device and starts DMA. Now the direct DMA
  ops are used even though IOMMU protection is active, DMA faults.

[1] https://lore.kernel.org/linux-iommu/20210414082633.877461-1-jean-philippe@linaro.org/
[2] https://lore.kernel.org/lkml/20210417120644.GA5235@zn.tnic/
---
 drivers/iommu/amd/iommu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index a69a8b573e40..7de7a260706b 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1747,6 +1747,8 @@ static void amd_iommu_probe_finalize(struct device *dev)
 	domain = iommu_get_domain_for_dev(dev);
 	if (domain->type == IOMMU_DOMAIN_DMA)
 		iommu_setup_dma_ops(dev, IOVA_START_PFN << PAGE_SHIFT, 0);
+	else
+		set_dma_ops(dev, NULL);
 }
 
 static void amd_iommu_release_device(struct device *dev)
-- 
2.31.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply related	[flat|nested] 2+ messages in thread