iommu/vt-d: Fixes for v5.17-rc2

Hi Joerg,

Two fixes are queued for v5.17. They aim to fix:

- Fix PCI bus rescan device hot add;
- Fix potential memory leak in intel_setup_irq_remapping().

Please consider them for the iommu/fix branch.

Best regards,
Lu Baolu

Guoqing Jiang (1):
iommu/vt-d: Fix potential memory leak in intel_setup_irq_remapping()

Jacob Pan (1):
iommu/vt-d: Fix PCI bus rescan device hot add

 include/linux/iommu.h               |  1 +
 drivers/iommu/intel/dmar.c          | 71 +++++++++++++++++++++--------
 drivers/iommu/intel/irq_remapping.c | 13 ++++--
 drivers/iommu/iommu.c               |  1 +
 4 files changed, 64 insertions(+), 22 deletions(-)

2.25.1

_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu

From: Jacob Pan <[email protected]>

During PCI bus rescan, adding new devices involve two notifiers.
1. dmar_pci_bus_notifier()
2. iommu_bus_notifier()
The current code sets #1 as low priority (INT_MIN) which resulted in #2 being invoked first. The result is that struct device pointer cannot be found in DRHD search for the new device's DMAR/IOMMU. Subsequently, the device is put under the "catch-all" IOMMU instead of the correct one.

This could cause system hang when device TLB invalidation is sent to the wrong IOMMU. Invalidation timeout error and hard lockup have been observed.

On the reverse direction for device removal, the order should be #2-#1 such that DMAR cleanup is done after IOMMU.

This patch fixes the issue by setting proper priorities for dmar_pci_bus_notifier around IOMMU bus notifier. DRHD search for a new device will find the correct IOMMU. The order with this patch is the following:
1. dmar_pci_bus_add_dev()
2. iommu_probe_device()
3. iommu_release_device()
4. dmar_pci_bus_remove_dev()

---
 include/linux/iommu.h      |  1 +
 drivers/iommu/intel/dmar.c | 71 ++++++++++++++++++++++++++++----------
 drivers/iommu/iommu.c      |  1 +
 3 files changed, 54 insertions(+), 19 deletions(-)

index de0c57a567c8..8e13c69980be 100644
@@ -403,6 +403,7 @@ static inline void iommu_iotlb_gather_init(struct iommu_iotlb_gather *gather)
 	};
 }
 
+#define IOMMU_BUS_NOTIFY_PRIORITY		0
 #define IOMMU_GROUP_NOTIFY_ADD_DEVICE		1 /* Device added */
 #define IOMMU_GROUP_NOTIFY_DEL_DEVICE		2 /* Pre Device removed */
 #define IOMMU_GROUP_NOTIFY_BIND_DRIVER		3 /* Pre Driver bind */

index 8b86406b7162..6103bcde1f65 100644
@@ -1841,6 +1841,7 @@ static int iommu_bus_init(struct bus_type *bus, const struct iommu_ops *ops)
 		return -ENOMEM;
 
 	nb->notifier_call = iommu_bus_notifier;
+	nb->priority = IOMMU_BUS_NOTIFY_PRIORITY;
 
 	err = bus_register_notifier(bus, nb);
 	if (err)

2.25.1

_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Hi Jacob, Baolu,

There are actually iommu_ops pointers invoked from iommu_bus_notifier() into IOMMU driver code. Can those be used to enforce the ordering in a more reliable way?

Regards,

Joerg
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Indeed I very nearly asked whether we couldn't just call these from intel_iommu_{probe,release}_device() directly, but it looks like they also interact with the interrupt remapping stuff which can be built independently of the IOMMU API :(

Robin.
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Okay, but having two notifiers is still ugly. Can we only register a notifier when IRQ-remapping is used without IOMMU-API? In this case a single notifier be sufficient.

Regards,

Joerg
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Hi Joerg,

Do you mean having an IRQ remapping notifier regardless IOMMU-API is enabled, right?
It make sense, I will give it a try.

Thanks,

Jacob
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Yeah, that would be best. I really don't like to use two notifiers just to work around ordering issues.

Regards,

Joerg
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Hi Joerg,

Another option Ashok and I discussed is that we can make DMAR cache persist (which should be for explicitly listed devices in each DRHD) across PCI remove-rescan cycle, then we don't need the DMAR PCI bus notifier at all.

This bug only impacts RCIEP device hotplug, which is not the most reasonable use case, we have the space to look into a proper fix.

Thanks,

Jacob
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Hi Jacob,

Even better if you can make the notifier obsolete. Looking forward to the fix.

Regards,

Joerg

_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Hi Joerg,

The race condition is between PCI/ACPI and IOMMU subsystems, I don't see how having IOMMU internal ops can solve the external race. Perhaps we should merge into one notifier to have direct control of the ordering, is that what you are suggesting? It seems to be a good albeit larger clean-up I can look into.

Thanks,

Jacob
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu

From: Guoqing Jiang <[email protected]>

After commit e3beca48a45b ("irqdomain/treewide: Keep firmware node unconditionally allocated"). For tear down scenario, fn is only freed after fail to allocate ir_domain, though it also should be freed in case dmar_enable_qi returns error.

Besides free fn, irq_domain and ir_msi_domain need to be removed as well if intel_setup_irq_remapping fails to enable queued invalidation.

Improve the rewinding path by add out_free_ir_domain and out_free_fwnode lables per Baolu's suggestion.

---
 drivers/iommu/intel/irq_remapping.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

index f912fe45bea2..a67319597884 100644
@@ -569,9 +569,8 @@ static int intel_setup_irq_remapping(struct intel_iommu *iommu)
 					    fn, &intel_ir_domain_ops,
 					    iommu);
 	if (!iommu->ir_domain) {
-		irq_domain_free_fwnode(fn);
 		pr_err("IR%d: failed to allocate irqdomain\n", iommu->seq_id);
-		goto out_free_bitmap;
+		goto out_free_fwnode;
 	}
 	iommu->ir_msi_domain =
 		arch_create_remap_msi_irq_domain(iommu->ir_domain,

@@ -595,7 +594,7 @@ static int intel_setup_irq_remapping(struct intel_iommu *iommu)
 
 		if (dmar_enable_qi(iommu)) {
 			pr_err("Failed to enable queued invalidation\n");
-			goto out_free_bitmap;
+			goto out_free_ir_domain;
 		}
 	}
 

@@ -619,6 +618,14 @@ static int intel_setup_irq_remapping(struct intel_iommu *iommu)
 
 	return 0;
 
+out_free_ir_domain:
+	if (iommu->ir_msi_domain)
+		irq_domain_remove(iommu->ir_msi_domain);
+	iommu->ir_msi_domain = NULL;
+	irq_domain_remove(iommu->ir_domain);
+	iommu->ir_domain = NULL;
+out_free_fwnode:
+	irq_domain_free_fwnode(fn);
 out_free_bitmap:
 	bitmap_free(bitmap);
 out_free_pages:

2.25.1

_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Applied this one, thanks.

_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu