iommu/vt-d: Do not BUG_ON in intel_unmap if no domain
Joerg Roedel
maintainerPATCH 1/1 iommu/vt-d: Do not BUG_ON in intel_unmap if no domain
From: Joerg Roedel <[email protected]>
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.
--- drivers/iommu/intel-iommu.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/iommu/intel-iommu.c +2 -1
@@ -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);
Signature · 1 line
1.9.1
Quoted 30 lines · Joerg Roedel
From: Joerg Roedel <[email protected]>
This
BUG_ONis easy to trigger with device-hotplug (e.g. SR-IOV). Thedevice_notifierfunction in the Intel IOMMU driver listens to theBUS_NOTIFY_DEL_DEVICEevent 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_unmapand triggers theBUG_ON.Not listening to
BUS_NOTIFY_DEL_DEVICEwould 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/[email protected]>Cc David Woodhouse <[email protected]>Signed-off-by Joerg Roedel <[email protected]>--- drivers/iommu/intel-iommu.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)drivers/iommu/intel-iommu.c +0
-0
drivers/iommu/intel-iommu.c static void intel_unmap(struct device *dev, dma_addr_t dev_addr) · line 3200
Quoted 6 lines
drivers/iommu/intel-iommu.c +2
-1@@ -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. :-)
Signature · 5 lines
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--Quoted 4 lines · Borislav Petkov
It is always questionable when people remove
BUG_ONsbecause relaxing assertions sound like a temporary fix more often than not. Sounds to me that the original commit which deals withBUS_NOTIFY_DEL_DEVICEneeds 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 1/1 iommu/vt-d: Defer domain removal if device is assigned to a driver
In reply to Borislav PetkovHi,
Quoted 4 lines · Borislav Petkov
It is always questionable when people remove
BUG_ONsbecause relaxing assertions sound like a temporary fix more often than not. Sounds to me that the original commit which deals withBUS_NOTIFY_DEL_DEVICEneeds 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:
Quoted 1 line
From
57e2519d5b6e4d8ee840a921300d201ff742c826Mon Sep 17 00:00:00 2001
From: Joerg Roedel <[email protected]>
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.
--- drivers/iommu/intel-iommu.c | 8 ++++++++ 1 file changed, 8 insertions(+)
drivers/iommu/intel-iommu.c +8 -0
@@ -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 ;
Signature · 1 line
1.8.4.5