[PATCH] iommu/vt-d: Do not BUG_ON in intel_unmap if no domain

[PATCH] iommu/vt-d: Do not BUG_ON in intel_unmap if no domain

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

This BUG_ON is easy to trigger with device-hotplug (e.g.
SR-IOV). The device_notifier function in the Intel IOMMU
driver listens to the BUS_NOTIFY_DEL_DEVICE event and frees
the domain for the device if it is reveived.

But this event is triggered before the device driver is
unbound from the device. When the driver core actually
removes the device the driver may release pending DMA
resources, which ends up in intel_unmap and triggers the
BUG_ON.

Not listening to BUS_NOTIFY_DEL_DEVICE would cause resource
leakage with devices that have never been assigned to any
driver, so fix this issue by just making unmap a nop when
the domain is already released.

Cc: Jiang Liu <jiang.liu@linux.intel.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/intel-iommu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index d1f5caa..7d689d7 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -3196,7 +3196,8 @@ static void intel_unmap(struct device *dev, dma_addr_t dev_addr)
 		return;
 
 	domain = find_domain(dev);
-	BUG_ON(!domain);
+	if (!domain)
+		return;
 
 	iommu = domain_get_iommu(domain);
 
-- 
1.9.1

Re: [PATCH] iommu/vt-d: Do not BUG_ON in intel_unmap if no domain

[Borislav Petkov]

On Mon, Aug 04, 2014 at 01:23:06PM +0200, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
> 
> This BUG_ON is easy to trigger with device-hotplug (e.g.
> SR-IOV). The device_notifier function in the Intel IOMMU
> driver listens to the BUS_NOTIFY_DEL_DEVICE event and frees
> the domain for the device if it is reveived.
> 
> But this event is triggered before the device driver is
> unbound from the device. When the driver core actually
> removes the device the driver may release pending DMA
> resources, which ends up in intel_unmap and triggers the
> BUG_ON.
> 
> Not listening to BUS_NOTIFY_DEL_DEVICE would cause resource
> leakage with devices that have never been assigned to any
> driver, so fix this issue by just making unmap a nop when
> the domain is already released.
> 
> Cc: Jiang Liu <jiang.liu@linux.intel.com>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>  drivers/iommu/intel-iommu.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index d1f5caa..7d689d7 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -3196,7 +3196,8 @@ static void intel_unmap(struct device *dev, dma_addr_t dev_addr)
>  		return;
>  
>  	domain = find_domain(dev);
> -	BUG_ON(!domain);
> +	if (!domain)
> +		return;

It is always questionable when people remove BUG_ONs because relaxing
assertions sound like a temporary fix more often than not. Sounds to me
that the original commit which deals with BUS_NOTIFY_DEL_DEVICE needs to
try again with the fix. :-)

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH] iommu/vt-d: Do not BUG_ON in intel_unmap if no domain

[Joerg Roedel]

On Mon, Aug 04, 2014 at 01:42:05PM +0200, Borislav Petkov wrote:
> It is always questionable when people remove BUG_ONs because relaxing
> assertions sound like a temporary fix more often than not. Sounds to me
> that the original commit which deals with BUS_NOTIFY_DEL_DEVICE needs to
> try again with the fix. :-)

Right, but the "real" fix would be to add a bus notifier that is called
after the device driver released the device and use this one instead of
BUS_NOTIFY_DEL_DEVICE.

But that change wouldn't be suitable for -stable and just removing the
event handler for BUS_NOTIFY_DEL_DEVICE would cause resource leakage
again on hotplug, so I think this "fix" is the best for now.

We can re-add the BUG_ON when we have a real fix upstream.


	Joerg

[PATCH] iommu/vt-d: Defer domain removal if device is assigned to a driver

[Joerg Roedel]

Hi,

On Mon, Aug 04, 2014 at 01:42:05PM +0200, Borislav Petkov wrote:
> It is always questionable when people remove BUG_ONs because relaxing
> assertions sound like a temporary fix more often than not. Sounds to me
> that the original commit which deals with BUS_NOTIFY_DEL_DEVICE needs to
> try again with the fix. :-)

Actually, as I thought about it again, there is a better fix for this
issue that does not require to remove the BUG_ON :) See attached patch:

>From 57e2519d5b6e4d8ee840a921300d201ff742c826 Mon Sep 17 00:00:00 2001
From: Joerg Roedel <jroedel@suse.de>
Date: Tue, 5 Aug 2014 12:55:45 +0200
Subject: [PATCH] iommu/vt-d: Defer domain removal if device is assigned to a
 driver

When the BUS_NOTIFY_DEL_DEVICE event is received the device
might still be attached to a driver. In this case the domain
can't be released as the mappings might still be in use.

Defer the domain removal in this case until we receivce the
BUS_NOTIFY_UNBOUND_DRIVER event.

Cc: Jiang Liu <jiang.liu@linux.intel.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: stable@vger.kernel.org   # v3.15, v3.16
Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/intel-iommu.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index d1f5caa..5619f26 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -3869,6 +3869,14 @@ static int device_notifier(struct notifier_block *nb,
 	    action != BUS_NOTIFY_DEL_DEVICE)
 		return 0;
 
+	/*
+	 * If the device is still attached to a device driver we can't
+	 * tear down the domain yet as DMA mappings may still be in use.
+	 * Wait for the BUS_NOTIFY_UNBOUND_DRIVER event to do that.
+	 */
+	if (action == BUS_NOTIFY_DEL_DEVICE && dev->driver != NULL)
+		return 0;
+
 	domain = find_domain(dev);
 	if (!domain)
 		return 0;
-- 
1.8.4.5