[PATCH 0/2] iommu/vt-d: Fixes for v5.17-rc2

[PATCH 0/2] iommu/vt-d: Fixes for v5.17-rc2

[Lu Baolu]

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
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 1/2] iommu/vt-d: Fix PCI bus rescan device hot add

[Lu Baolu]

From: Jacob Pan <jacob.jun.pan@linux.intel.com>

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()

Fixes: 59ce0515cdaf ("iommu/vt-d: Update DRHD/RMRR/ATSR device scope")
Reported-by: Zhang, Bernice <bernice.zhang@intel.com>
Suggested-by: Lu Baolu <baolu.lu@linux.intel.com>
Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
Link: https://lore.kernel.org/r/1642148470-11949-1-git-send-email-jacob.jun.pan@linux.intel.com
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 include/linux/iommu.h      |  1 +
 drivers/iommu/intel/dmar.c | 71 ++++++++++++++++++++++++++++----------
 drivers/iommu/iommu.c      |  1 +
 3 files changed, 54 insertions(+), 19 deletions(-)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index de0c57a567c8..8e13c69980be 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -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 */
diff --git a/drivers/iommu/intel/dmar.c b/drivers/iommu/intel/dmar.c
index 915bff76fe96..fb1b2fa36e27 100644
--- a/drivers/iommu/intel/dmar.c
+++ b/drivers/iommu/intel/dmar.c
@@ -340,15 +340,19 @@ static inline void vf_inherit_msi_domain(struct pci_dev *pdev)
 	dev_set_msi_domain(&pdev->dev, dev_get_msi_domain(&physfn->dev));
 }
 
-static int dmar_pci_bus_notifier(struct notifier_block *nb,
-				 unsigned long action, void *data)
+static int dmar_pci_bus_add_notifier(struct notifier_block *nb,
+				     unsigned long action, void *data)
 {
 	struct pci_dev *pdev = to_pci_dev(data);
 	struct dmar_pci_notify_info *info;
 
-	/* Only care about add/remove events for physical functions.
+	if (action != BUS_NOTIFY_ADD_DEVICE)
+		return NOTIFY_DONE;
+
+	/*
 	 * For VFs we actually do the lookup based on the corresponding
-	 * PF in device_to_iommu() anyway. */
+	 * PF in device_to_iommu() anyway.
+	 */
 	if (pdev->is_virtfn) {
 		/*
 		 * Ensure that the VF device inherits the irq domain of the
@@ -358,13 +362,34 @@ static int dmar_pci_bus_notifier(struct notifier_block *nb,
 		 * from the PF device, but that's yet another x86'sism to
 		 * inflict on everybody else.
 		 */
-		if (action == BUS_NOTIFY_ADD_DEVICE)
-			vf_inherit_msi_domain(pdev);
+		vf_inherit_msi_domain(pdev);
 		return NOTIFY_DONE;
 	}
 
-	if (action != BUS_NOTIFY_ADD_DEVICE &&
-	    action != BUS_NOTIFY_REMOVED_DEVICE)
+	info = dmar_alloc_pci_notify_info(pdev, action);
+	if (!info)
+		return NOTIFY_DONE;
+
+	down_write(&dmar_global_lock);
+	dmar_pci_bus_add_dev(info);
+	up_write(&dmar_global_lock);
+	dmar_free_pci_notify_info(info);
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block dmar_pci_bus_add_nb = {
+	.notifier_call = dmar_pci_bus_add_notifier,
+	.priority = IOMMU_BUS_NOTIFY_PRIORITY + 1,
+};
+
+static int dmar_pci_bus_remove_notifier(struct notifier_block *nb,
+					unsigned long action, void *data)
+{
+	struct pci_dev *pdev = to_pci_dev(data);
+	struct dmar_pci_notify_info *info;
+
+	if (pdev->is_virtfn || action != BUS_NOTIFY_REMOVED_DEVICE)
 		return NOTIFY_DONE;
 
 	info = dmar_alloc_pci_notify_info(pdev, action);
@@ -372,10 +397,7 @@ static int dmar_pci_bus_notifier(struct notifier_block *nb,
 		return NOTIFY_DONE;
 
 	down_write(&dmar_global_lock);
-	if (action == BUS_NOTIFY_ADD_DEVICE)
-		dmar_pci_bus_add_dev(info);
-	else if (action == BUS_NOTIFY_REMOVED_DEVICE)
-		dmar_pci_bus_del_dev(info);
+	dmar_pci_bus_del_dev(info);
 	up_write(&dmar_global_lock);
 
 	dmar_free_pci_notify_info(info);
@@ -383,9 +405,9 @@ static int dmar_pci_bus_notifier(struct notifier_block *nb,
 	return NOTIFY_OK;
 }
 
-static struct notifier_block dmar_pci_bus_nb = {
-	.notifier_call = dmar_pci_bus_notifier,
-	.priority = INT_MIN,
+static struct notifier_block dmar_pci_bus_remove_nb = {
+	.notifier_call = dmar_pci_bus_remove_notifier,
+	.priority = IOMMU_BUS_NOTIFY_PRIORITY - 1,
 };
 
 static struct dmar_drhd_unit *
@@ -835,10 +857,19 @@ int __init dmar_dev_scope_init(void)
 
 void __init dmar_register_bus_notifier(void)
 {
-	bus_register_notifier(&pci_bus_type, &dmar_pci_bus_nb);
+	/*
+	 * We need two notifiers in that we need to make sure the ordering
+	 * is enforced as the following:
+	 * 1. dmar_pci_bus_add_dev()
+	 * 2. iommu_probe_device()
+	 * 3. iommu_release_device()
+	 * 4. dmar_pci_bus_remove_dev()
+	 * Notifier block priority is used to enforce the order
+	 */
+	bus_register_notifier(&pci_bus_type, &dmar_pci_bus_add_nb);
+	bus_register_notifier(&pci_bus_type, &dmar_pci_bus_remove_nb);
 }
 
-
 int __init dmar_table_init(void)
 {
 	static int dmar_table_initialized;
@@ -2151,8 +2182,10 @@ static int __init dmar_free_unused_resources(void)
 	if (dmar_in_use())
 		return 0;
 
-	if (dmar_dev_scope_status != 1 && !list_empty(&dmar_drhd_units))
-		bus_unregister_notifier(&pci_bus_type, &dmar_pci_bus_nb);
+	if (dmar_dev_scope_status != 1 && !list_empty(&dmar_drhd_units)) {
+		bus_unregister_notifier(&pci_bus_type, &dmar_pci_bus_add_nb);
+		bus_unregister_notifier(&pci_bus_type, &dmar_pci_bus_remove_nb);
+	}
 
 	down_write(&dmar_global_lock);
 	list_for_each_entry_safe(dmaru, dmaru_n, &dmar_drhd_units, list) {
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 8b86406b7162..6103bcde1f65 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -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
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 1/2] iommu/vt-d: Fix PCI bus rescan device hot add

[Joerg Roedel]

Hi Jacob, Baolu,

On Fri, Jan 28, 2022 at 11:10:01AM +0800, Lu Baolu wrote:
> 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.

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
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 1/2] iommu/vt-d: Fix PCI bus rescan device hot add

[Robin Murphy]

On 2022-01-30 07:43, Joerg Roedel wrote:
> Hi Jacob, Baolu,
> 
> On Fri, Jan 28, 2022 at 11:10:01AM +0800, Lu Baolu wrote:
>> 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.
> 
> 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?

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
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 1/2] iommu/vt-d: Fix PCI bus rescan device hot add

[Joerg Roedel]

On Mon, Jan 31, 2022 at 01:53:06PM +0000, Robin Murphy wrote:
> 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 :(

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
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 1/2] iommu/vt-d: Fix PCI bus rescan device hot add

[Jacob Pan]

Hi Joerg,

On Mon, 31 Jan 2022 16:52:10 +0100, Joerg Roedel <joro@8bytes.org> wrote:

> On Mon, Jan 31, 2022 at 01:53:06PM +0000, Robin Murphy wrote:
> > 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 :(  
> 
> 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.
> 
Do you mean having an IRQ remapping notifier regardless IOMMU-API is
enabled, right?
It make sense, I will give it a try.

> Regards,
> 
> Joerg
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu


Thanks,

Jacob
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 1/2] iommu/vt-d: Fix PCI bus rescan device hot add

[Joerg Roedel]

On Tue, Feb 01, 2022 at 11:19:18AM -0800, Jacob Pan wrote:
> Do you mean having an IRQ remapping notifier regardless IOMMU-API is
> enabled, right?
> It make sense, I will give it a try.

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
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 1/2] iommu/vt-d: Fix PCI bus rescan device hot add

[Jacob Pan]

Hi Joerg,

On Thu, 3 Feb 2022 09:25:58 +0100, Joerg Roedel <joro@8bytes.org> wrote:

> On Tue, Feb 01, 2022 at 11:19:18AM -0800, Jacob Pan wrote:
> > Do you mean having an IRQ remapping notifier regardless IOMMU-API is
> > enabled, right?
> > It make sense, I will give it a try.  
> 
> Yeah, that would be best. I really don't like to use two notifiers just
> to work around ordering issues.
> 
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.

> Regards,
> 
> 	Joerg


Thanks,

Jacob
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 1/2] iommu/vt-d: Fix PCI bus rescan device hot add

[Joerg Roedel]

Hi Jacob,

On Wed, Feb 09, 2022 at 01:52:49PM -0800, Jacob Pan wrote:
> 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.

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

Regards,

	Joerg

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 1/2] iommu/vt-d: Fix PCI bus rescan device hot add

[Jacob Pan]

Hi Joerg,

On Sun, 30 Jan 2022 08:43:19 +0100, Joerg Roedel <joro@8bytes.org> wrote:

> Hi Jacob, Baolu,
> 
> On Fri, Jan 28, 2022 at 11:10:01AM +0800, Lu Baolu wrote:
> > 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.  
> 
> 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?
> 
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
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 2/2] iommu/vt-d: Fix potential memory leak in intel_setup_irq_remapping()

[Lu Baolu]

From: Guoqing Jiang <guoqing.jiang@linux.dev>

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.

Fixes: e3beca48a45b ("irqdomain/treewide: Keep firmware node unconditionally allocated")
Suggested-by: Lu Baolu <baolu.lu@linux.intel.com>
Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
Link: https://lore.kernel.org/r/20220119063640.16864-1-guoqing.jiang@linux.dev
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/intel/irq_remapping.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/intel/irq_remapping.c b/drivers/iommu/intel/irq_remapping.c
index f912fe45bea2..a67319597884 100644
--- a/drivers/iommu/intel/irq_remapping.c
+++ b/drivers/iommu/intel/irq_remapping.c
@@ -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
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 2/2] iommu/vt-d: Fix potential memory leak in intel_setup_irq_remapping()

[Joerg Roedel]

On Fri, Jan 28, 2022 at 11:10:02AM +0800, Lu Baolu wrote:
> From: Guoqing Jiang <guoqing.jiang@linux.dev>
> 
> 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.
> 
> Fixes: e3beca48a45b ("irqdomain/treewide: Keep firmware node unconditionally allocated")
> Suggested-by: Lu Baolu <baolu.lu@linux.intel.com>
> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
> Link: https://lore.kernel.org/r/20220119063640.16864-1-guoqing.jiang@linux.dev
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/intel/irq_remapping.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)

Applied this one, thanks.

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu