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

2021-05-18 Thread Joerg Roedel
On Thu, Apr 22, 2021 at 11:42:19AM +0200, Jean-Philippe Brucker wrote:
> 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 :00:05.0 > /sys/sys/bus/pci/drivers/e1000e/unbind
>   # echo identity > /sys/bus/pci/devices/:00:05.0/iommu_group/type
>   # echo :00:05.0 > /sys/sys/bus/pci/drivers/e1000e/bind
>...
>   BUG: kernel NULL pointer dereference, address: 0028
>...
>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 

Applied for v5.13, thanks Jean-Philippe.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


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

2021-04-22 Thread 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 :00:05.0 > /sys/sys/bus/pci/drivers/e1000e/unbind
  # echo identity > /sys/bus/pci/devices/:00:05.0/iommu_group/type
  # echo :00:05.0 > /sys/sys/bus/pci/drivers/e1000e/bind
   ...
  BUG: kernel NULL pointer dereference, address: 0028
   ...
   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 

---
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-phili...@linaro.org/
[2] https://lore.kernel.org/lkml/20210417120644.ga5...@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