2014-12-17 10:43:49

by Joerg Roedel

[permalink] [raw]
Subject: [PATCH 1/2] iommu/vt-d: Fix dmar_domain leak in iommu_attach_device

From: Joerg Roedel <[email protected]>

Since commit 1196c2f a domain is only destroyed in the
notifier path if it is hot-unplugged. This caused a
domain leakage in iommu_attach_device when a driver was
unbound from the device and bound to VFIO. In this case the
device is attached to a new domain and unlinked from the old
domain. At this point nothing points to the old domain
anymore and its memory is leaked.
Fix this by explicitly freeing the old domain in
iommu_attach_domain.

Fixes: 1196c2f (iommu/vt-d: Fix dmar_domain leak in iommu_attach_device)
Cc: <[email protected]> # v3.18
Signed-off-by: Joerg Roedel <[email protected]>
---
drivers/iommu/intel-iommu.c | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 1232336..7610121 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -4428,6 +4428,10 @@ static int intel_iommu_attach_device(struct iommu_domain *domain,
domain_remove_one_dev_info(old_domain, dev);
else
domain_remove_dev_info(old_domain);
+
+ if (!domain_type_is_vm_or_si(old_domain) &&
+ list_empty(&old_domain->devices))
+ domain_exit(old_domain);
}
}

--
1.9.1


2014-12-17 10:43:47

by Joerg Roedel

[permalink] [raw]
Subject: [PATCH 2/2] iommu/vt-d: Remove dead code in device_notifier

From: Joerg Roedel <[email protected]>

This code only runs when action == BUS_NOTIFY_REMOVED_DEVICE,
so it can't be BUS_NOTIFY_DEL_DEVICE.

Signed-off-by: Joerg Roedel <[email protected]>
---
drivers/iommu/intel-iommu.c | 8 --------
1 file changed, 8 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 7610121..40dfbc0 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -4029,14 +4029,6 @@ static int device_notifier(struct notifier_block *nb,
if (action != BUS_NOTIFY_REMOVED_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.9.1

2014-12-17 21:37:52

by Jerry Hoemann

[permalink] [raw]
Subject: Re: [PATCH 1/2] iommu/vt-d: Fix dmar_domain leak in iommu_attach_device

On Wed, Dec 17, 2014 at 11:43:36AM +0100, Joerg Roedel wrote:
> From: Joerg Roedel <[email protected]>
>
> Since commit 1196c2f a domain is only destroyed in the
> notifier path if it is hot-unplugged. This caused a
> domain leakage in iommu_attach_device when a driver was
> unbound from the device and bound to VFIO. In this case the
> device is attached to a new domain and unlinked from the old
> domain. At this point nothing points to the old domain
> anymore and its memory is leaked.
> Fix this by explicitly freeing the old domain in
> iommu_attach_domain.
>
> Fixes: 1196c2f (iommu/vt-d: Fix dmar_domain leak in iommu_attach_device)
> Cc: <[email protected]> # v3.18
> Signed-off-by: Joerg Roedel <[email protected]>
> ---
> drivers/iommu/intel-iommu.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index 1232336..7610121 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -4428,6 +4428,10 @@ static int intel_iommu_attach_device(struct iommu_domain *domain,
> domain_remove_one_dev_info(old_domain, dev);
> else
> domain_remove_dev_info(old_domain);
> +
> + if (!domain_type_is_vm_or_si(old_domain) &&
> + list_empty(&old_domain->devices))
> + domain_exit(old_domain);
> }
> }
>
> --
> 1.9.1

Joerg,

Before applying this change on a 3.18-rc7 kernel layered on a
RHEL 7.0 root disk, I was able to reproduce the memory
leak that Alex reported when powering on/off a VM w/ a PCI
device assigned to it.

After applying this change, I don't see the memory leak anymore.
Instrumentation shows the new code is being invoked during power on
of the VM.

Looks good.

Thanks!

Tested-by: Jerry Hoemann <[email protected]>
--

----------------------------------------------------------------------------
Jerry Hoemann Software Engineer Hewlett-Packard

3404 E Harmony Rd. MS 36 phone: (970) 898-1022
Ft. Collins, CO 80528 FAX: (970) 898-0707
email: [email protected]
----------------------------------------------------------------------------

2014-12-17 21:41:46

by Jerry Hoemann

[permalink] [raw]
Subject: Re: [PATCH 2/2] iommu/vt-d: Remove dead code in device_notifier

On Wed, Dec 17, 2014 at 11:43:37AM +0100, Joerg Roedel wrote:
> From: Joerg Roedel <[email protected]>
>
> This code only runs when action == BUS_NOTIFY_REMOVED_DEVICE,
> so it can't be BUS_NOTIFY_DEL_DEVICE.
>
> Signed-off-by: Joerg Roedel <[email protected]>
> ---
> drivers/iommu/intel-iommu.c | 8 --------
> 1 file changed, 8 deletions(-)
>
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index 7610121..40dfbc0 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -4029,14 +4029,6 @@ static int device_notifier(struct notifier_block *nb,
> if (action != BUS_NOTIFY_REMOVED_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.9.1

Joerg,

Tested this along w/ other patch of the set.

Instrumentation showed that the removed code wouldn't have been executed
during VM power on/power off.

Thanks!

Tested-by: Jerry Hoemann <[email protected]>


--

----------------------------------------------------------------------------
Jerry Hoemann Software Engineer Hewlett-Packard

3404 E Harmony Rd. MS 36 phone: (970) 898-1022
Ft. Collins, CO 80528 FAX: (970) 898-0707
email: [email protected]
----------------------------------------------------------------------------