All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] iommu/vt-d: Do not BUG_ON in intel_unmap if no domain
@ 2014-08-04 11:23 ` Joerg Roedel
  0 siblings, 0 replies; 6+ messages in thread
From: Joerg Roedel @ 2014-08-04 11:23 UTC (permalink / raw)
  To: iommu; +Cc: linux-kernel, joro, Joerg Roedel, Jiang Liu, David Woodhouse

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


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH] iommu/vt-d: Do not BUG_ON in intel_unmap if no domain
@ 2014-08-04 11:23 ` Joerg Roedel
  0 siblings, 0 replies; 6+ messages in thread
From: Joerg Roedel @ 2014-08-04 11:23 UTC (permalink / raw)
  To: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA
  Cc: Joerg Roedel, David Woodhouse,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Jiang Liu

From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>

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-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Cc: David Woodhouse <dwmw2-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
Signed-off-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
---
 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

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] iommu/vt-d: Do not BUG_ON in intel_unmap if no domain
@ 2014-08-04 11:42   ` Borislav Petkov
  0 siblings, 0 replies; 6+ messages in thread
From: Borislav Petkov @ 2014-08-04 11:42 UTC (permalink / raw)
  To: Joerg Roedel
  Cc: iommu, linux-kernel, Joerg Roedel, Jiang Liu, David Woodhouse

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.
--

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] iommu/vt-d: Do not BUG_ON in intel_unmap if no domain
@ 2014-08-04 11:42   ` Borislav Petkov
  0 siblings, 0 replies; 6+ messages in thread
From: Borislav Petkov @ 2014-08-04 11:42 UTC (permalink / raw)
  To: Joerg Roedel
  Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, Joerg Roedel,
	David Woodhouse, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Jiang Liu

On Mon, Aug 04, 2014 at 01:23:06PM +0200, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
> 
> 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-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> Cc: David Woodhouse <dwmw2-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
> Signed-off-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
> ---
>  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.
--

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] iommu/vt-d: Do not BUG_ON in intel_unmap if no domain
  2014-08-04 11:42   ` Borislav Petkov
  (?)
@ 2014-08-04 11:49   ` Joerg Roedel
  -1 siblings, 0 replies; 6+ messages in thread
From: Joerg Roedel @ 2014-08-04 11:49 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Joerg Roedel, iommu, linux-kernel, Jiang Liu, David Woodhouse

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


^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH] iommu/vt-d: Defer domain removal if device is assigned to a driver
  2014-08-04 11:42   ` Borislav Petkov
  (?)
  (?)
@ 2014-08-05 11:05   ` Joerg Roedel
  -1 siblings, 0 replies; 6+ messages in thread
From: Joerg Roedel @ 2014-08-05 11:05 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Joerg Roedel, iommu, linux-kernel, Jiang Liu, David Woodhouse

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


^ permalink raw reply related	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-08-05 11:05 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-08-04 11:23 [PATCH] iommu/vt-d: Do not BUG_ON in intel_unmap if no domain Joerg Roedel
2014-08-04 11:23 ` Joerg Roedel
2014-08-04 11:42 ` Borislav Petkov
2014-08-04 11:42   ` Borislav Petkov
2014-08-04 11:49   ` Joerg Roedel
2014-08-05 11:05   ` [PATCH] iommu/vt-d: Defer domain removal if device is assigned to a driver Joerg Roedel

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.