[PATCH] irqdomain/treewide: Free firmware node after domain removal

[PATCH] irqdomain/treewide: Free firmware node after domain removal

[Jon Derrick]

Change 711419e504eb ("irqdomain: Add the missing assignment of
domain->fwnode for named fwnode") unintentionally caused a dangling
pointer page fault issue on firmware nodes that were freed after IRQ
domain allocation. Change e3beca48a45b fixed that dangling pointer issue
by only freeing the firmware node after an IRQ domain allocation
failure. That fix no longer frees the firmware node immediately, but
leaves the firmware node allocated after the domain is removed.

We need to keep the firmware node through irq_domain_remove, but should
free it afterwards. This patch saves the handle and adds the freeing of
firmware node after domain removal where appropriate.

Fixes: e3beca48a45b ("irqdomain/treewide: Keep firmware node unconditionally allocated")
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
Cc: stable@vger.kernel.org
---
 arch/mips/pci/pci-xtalk-bridge.c    | 3 +++
 arch/x86/kernel/apic/io_apic.c      | 5 +++++
 drivers/iommu/intel/irq_remapping.c | 8 ++++++++
 drivers/mfd/ioc3.c                  | 6 ++++++
 drivers/pci/controller/vmd.c        | 3 +++
 5 files changed, 25 insertions(+)

diff --git a/arch/mips/pci/pci-xtalk-bridge.c b/arch/mips/pci/pci-xtalk-bridge.c
index 5958217..9b3cc77 100644
--- a/arch/mips/pci/pci-xtalk-bridge.c
+++ b/arch/mips/pci/pci-xtalk-bridge.c
@@ -728,6 +728,7 @@ static int bridge_probe(struct platform_device *pdev)
 	pci_free_resource_list(&host->windows);
 err_remove_domain:
 	irq_domain_remove(domain);
+	irq_domain_free_fwnode(fn);
 	return err;
 }
 
@@ -735,8 +736,10 @@ static int bridge_remove(struct platform_device *pdev)
 {
 	struct pci_bus *bus = platform_get_drvdata(pdev);
 	struct bridge_controller *bc = BRIDGE_CONTROLLER(bus);
+	struct fwnode_handle *fn = bc->domain->fwnode;
 
 	irq_domain_remove(bc->domain);
+	irq_domain_free_fwnode(fn);
 	pci_lock_rescan_remove();
 	pci_stop_root_bus(bus);
 	pci_remove_root_bus(bus);
diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 81ffcfb..21325a4a 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -2335,8 +2335,13 @@ static int mp_irqdomain_create(int ioapic)
 
 static void ioapic_destroy_irqdomain(int idx)
 {
+	struct ioapic_domain_cfg *cfg = &ioapics[idx].irqdomain_cfg;
+	struct fwnode_handle *fn = ioapics[idx].irqdomain->fwnode;
+
 	if (ioapics[idx].irqdomain) {
 		irq_domain_remove(ioapics[idx].irqdomain);
+		if (!cfg->dev)
+			irq_domain_free_fwnode(fn);
 		ioapics[idx].irqdomain = NULL;
 	}
 }
diff --git a/drivers/iommu/intel/irq_remapping.c b/drivers/iommu/intel/irq_remapping.c
index 9564d23..aa096b3 100644
--- a/drivers/iommu/intel/irq_remapping.c
+++ b/drivers/iommu/intel/irq_remapping.c
@@ -628,13 +628,21 @@ static int intel_setup_irq_remapping(struct intel_iommu *iommu)
 
 static void intel_teardown_irq_remapping(struct intel_iommu *iommu)
 {
+	struct fwnode_handle *fn;
+
 	if (iommu && iommu->ir_table) {
 		if (iommu->ir_msi_domain) {
+			fn = iommu->ir_msi_domain->fwnode;
+
 			irq_domain_remove(iommu->ir_msi_domain);
+			irq_domain_free_fwnode(fn);
 			iommu->ir_msi_domain = NULL;
 		}
 		if (iommu->ir_domain) {
+			fn = iommu->ir_domain->fwnode;
+
 			irq_domain_remove(iommu->ir_domain);
+			irq_domain_free_fwnode(fn);
 			iommu->ir_domain = NULL;
 		}
 		free_pages((unsigned long)iommu->ir_table->base,
diff --git a/drivers/mfd/ioc3.c b/drivers/mfd/ioc3.c
index 74cee7c..d939ccc 100644
--- a/drivers/mfd/ioc3.c
+++ b/drivers/mfd/ioc3.c
@@ -616,7 +616,10 @@ static int ioc3_mfd_probe(struct pci_dev *pdev,
 		/* Remove all already added MFD devices */
 		mfd_remove_devices(&ipd->pdev->dev);
 		if (ipd->domain) {
+			struct fwnode_handle *fn = ipd->domain->fwnode;
+
 			irq_domain_remove(ipd->domain);
+			irq_domain_free_fwnode(fn);
 			free_irq(ipd->domain_irq, (void *)ipd);
 		}
 		pci_iounmap(pdev, regs);
@@ -643,7 +646,10 @@ static void ioc3_mfd_remove(struct pci_dev *pdev)
 	/* Release resources */
 	mfd_remove_devices(&ipd->pdev->dev);
 	if (ipd->domain) {
+		struct fwnode_handle *fn = ipd->domain->fwnode;
+
 		irq_domain_remove(ipd->domain);
+		irq_domain_free_fwnode(fn);
 		free_irq(ipd->domain_irq, (void *)ipd);
 	}
 	pci_iounmap(pdev, ipd->regs);
diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index f078114..91eb769 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -560,6 +560,7 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
 	if (!vmd->bus) {
 		pci_free_resource_list(&resources);
 		irq_domain_remove(vmd->irq_domain);
+		irq_domain_free_fwnode(fn);
 		return -ENODEV;
 	}
 
@@ -673,6 +674,7 @@ static void vmd_cleanup_srcu(struct vmd_dev *vmd)
 static void vmd_remove(struct pci_dev *dev)
 {
 	struct vmd_dev *vmd = pci_get_drvdata(dev);
+	struct fwnode_handle *fn = vmd->irq_domain->fwnode;
 
 	sysfs_remove_link(&vmd->dev->dev.kobj, "domain");
 	pci_stop_root_bus(vmd->bus);
@@ -680,6 +682,7 @@ static void vmd_remove(struct pci_dev *dev)
 	vmd_cleanup_srcu(vmd);
 	vmd_detach_resources(vmd);
 	irq_domain_remove(vmd->irq_domain);
+	irq_domain_free_fwnode(fn);
 }
 
 #ifdef CONFIG_PM_SLEEP
-- 
1.8.3.1

Re: [PATCH] irqdomain/treewide: Free firmware node after domain removal

[Bjorn Helgaas]

On Tue, Jul 21, 2020 at 02:26:09PM -0600, Jon Derrick wrote:
> Change 711419e504eb ("irqdomain: Add the missing assignment of
> domain->fwnode for named fwnode") unintentionally caused a dangling
> pointer page fault issue on firmware nodes that were freed after IRQ
> domain allocation. Change e3beca48a45b fixed that dangling pointer issue
> by only freeing the firmware node after an IRQ domain allocation
> failure. That fix no longer frees the firmware node immediately, but
> leaves the firmware node allocated after the domain is removed.
> 
> We need to keep the firmware node through irq_domain_remove, but should
> free it afterwards. This patch saves the handle and adds the freeing of
> firmware node after domain removal where appropriate.
> 
> Fixes: e3beca48a45b ("irqdomain/treewide: Keep firmware node unconditionally allocated")
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
> Cc: stable@vger.kernel.org

Acked-by: Bjorn Helgaas <bhelgaas@google.com>	# drivers/pci

> ---
>  arch/mips/pci/pci-xtalk-bridge.c    | 3 +++
>  arch/x86/kernel/apic/io_apic.c      | 5 +++++
>  drivers/iommu/intel/irq_remapping.c | 8 ++++++++
>  drivers/mfd/ioc3.c                  | 6 ++++++
>  drivers/pci/controller/vmd.c        | 3 +++
>  5 files changed, 25 insertions(+)
> 
> diff --git a/arch/mips/pci/pci-xtalk-bridge.c b/arch/mips/pci/pci-xtalk-bridge.c
> index 5958217..9b3cc77 100644
> --- a/arch/mips/pci/pci-xtalk-bridge.c
> +++ b/arch/mips/pci/pci-xtalk-bridge.c
> @@ -728,6 +728,7 @@ static int bridge_probe(struct platform_device *pdev)
>  	pci_free_resource_list(&host->windows);
>  err_remove_domain:
>  	irq_domain_remove(domain);
> +	irq_domain_free_fwnode(fn);
>  	return err;
>  }
>  
> @@ -735,8 +736,10 @@ static int bridge_remove(struct platform_device *pdev)
>  {
>  	struct pci_bus *bus = platform_get_drvdata(pdev);
>  	struct bridge_controller *bc = BRIDGE_CONTROLLER(bus);
> +	struct fwnode_handle *fn = bc->domain->fwnode;
>  
>  	irq_domain_remove(bc->domain);
> +	irq_domain_free_fwnode(fn);
>  	pci_lock_rescan_remove();
>  	pci_stop_root_bus(bus);
>  	pci_remove_root_bus(bus);
> diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
> index 81ffcfb..21325a4a 100644
> --- a/arch/x86/kernel/apic/io_apic.c
> +++ b/arch/x86/kernel/apic/io_apic.c
> @@ -2335,8 +2335,13 @@ static int mp_irqdomain_create(int ioapic)
>  
>  static void ioapic_destroy_irqdomain(int idx)
>  {
> +	struct ioapic_domain_cfg *cfg = &ioapics[idx].irqdomain_cfg;
> +	struct fwnode_handle *fn = ioapics[idx].irqdomain->fwnode;
> +
>  	if (ioapics[idx].irqdomain) {
>  		irq_domain_remove(ioapics[idx].irqdomain);
> +		if (!cfg->dev)
> +			irq_domain_free_fwnode(fn);
>  		ioapics[idx].irqdomain = NULL;
>  	}
>  }
> diff --git a/drivers/iommu/intel/irq_remapping.c b/drivers/iommu/intel/irq_remapping.c
> index 9564d23..aa096b3 100644
> --- a/drivers/iommu/intel/irq_remapping.c
> +++ b/drivers/iommu/intel/irq_remapping.c
> @@ -628,13 +628,21 @@ static int intel_setup_irq_remapping(struct intel_iommu *iommu)
>  
>  static void intel_teardown_irq_remapping(struct intel_iommu *iommu)
>  {
> +	struct fwnode_handle *fn;
> +
>  	if (iommu && iommu->ir_table) {
>  		if (iommu->ir_msi_domain) {
> +			fn = iommu->ir_msi_domain->fwnode;
> +
>  			irq_domain_remove(iommu->ir_msi_domain);
> +			irq_domain_free_fwnode(fn);
>  			iommu->ir_msi_domain = NULL;
>  		}
>  		if (iommu->ir_domain) {
> +			fn = iommu->ir_domain->fwnode;
> +
>  			irq_domain_remove(iommu->ir_domain);
> +			irq_domain_free_fwnode(fn);
>  			iommu->ir_domain = NULL;
>  		}
>  		free_pages((unsigned long)iommu->ir_table->base,
> diff --git a/drivers/mfd/ioc3.c b/drivers/mfd/ioc3.c
> index 74cee7c..d939ccc 100644
> --- a/drivers/mfd/ioc3.c
> +++ b/drivers/mfd/ioc3.c
> @@ -616,7 +616,10 @@ static int ioc3_mfd_probe(struct pci_dev *pdev,
>  		/* Remove all already added MFD devices */
>  		mfd_remove_devices(&ipd->pdev->dev);
>  		if (ipd->domain) {
> +			struct fwnode_handle *fn = ipd->domain->fwnode;
> +
>  			irq_domain_remove(ipd->domain);
> +			irq_domain_free_fwnode(fn);
>  			free_irq(ipd->domain_irq, (void *)ipd);
>  		}
>  		pci_iounmap(pdev, regs);
> @@ -643,7 +646,10 @@ static void ioc3_mfd_remove(struct pci_dev *pdev)
>  	/* Release resources */
>  	mfd_remove_devices(&ipd->pdev->dev);
>  	if (ipd->domain) {
> +		struct fwnode_handle *fn = ipd->domain->fwnode;
> +
>  		irq_domain_remove(ipd->domain);
> +		irq_domain_free_fwnode(fn);
>  		free_irq(ipd->domain_irq, (void *)ipd);
>  	}
>  	pci_iounmap(pdev, ipd->regs);
> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index f078114..91eb769 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
> @@ -560,6 +560,7 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
>  	if (!vmd->bus) {
>  		pci_free_resource_list(&resources);
>  		irq_domain_remove(vmd->irq_domain);
> +		irq_domain_free_fwnode(fn);
>  		return -ENODEV;
>  	}
>  
> @@ -673,6 +674,7 @@ static void vmd_cleanup_srcu(struct vmd_dev *vmd)
>  static void vmd_remove(struct pci_dev *dev)
>  {
>  	struct vmd_dev *vmd = pci_get_drvdata(dev);
> +	struct fwnode_handle *fn = vmd->irq_domain->fwnode;
>  
>  	sysfs_remove_link(&vmd->dev->dev.kobj, "domain");
>  	pci_stop_root_bus(vmd->bus);
> @@ -680,6 +682,7 @@ static void vmd_remove(struct pci_dev *dev)
>  	vmd_cleanup_srcu(vmd);
>  	vmd_detach_resources(vmd);
>  	irq_domain_remove(vmd->irq_domain);
> +	irq_domain_free_fwnode(fn);
>  }
>  
>  #ifdef CONFIG_PM_SLEEP
> -- 
> 1.8.3.1
> 

Re: [PATCH] irqdomain/treewide: Free firmware node after domain removal

[Thomas Gleixner]

Jon Derrick <jonathan.derrick@intel.com> writes:
> Change 711419e504eb ("irqdomain: Add the missing assignment of
> domain->fwnode for named fwnode") unintentionally caused a dangling
> pointer page fault issue on firmware nodes that were freed after IRQ
> domain allocation. Change e3beca48a45b fixed that dangling pointer issue
> by only freeing the firmware node after an IRQ domain allocation
> failure. That fix no longer frees the firmware node immediately, but
> leaves the firmware node allocated after the domain is removed.

Gah, I missed that under the assumption that these nodes are freed when
the domain is removed. What a mess.

> We need to keep the firmware node through irq_domain_remove, but should
> free it afterwards. This patch saves the handle and adds the freeing of
> firmware node after domain removal where appropriate.

Care to read Documentation/process/submitting-patches.rst and look for
'I/We' and 'This patch' next time?

Thanks,

        tglx

[tip: irq/urgent] irqdomain/treewide: Free firmware node after domain removal

[tip-bot2 for Jon Derrick]

The following commit has been merged into the irq/urgent branch of tip:

Commit-ID:     ec0160891e387f4771f953b888b1fe951398e5d9
Gitweb:        https://git.kernel.org/tip/ec0160891e387f4771f953b888b1fe951398e5d9
Author:        Jon Derrick <jonathan.derrick@intel.com>
AuthorDate:    Tue, 21 Jul 2020 14:26:09 -06:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Thu, 23 Jul 2020 00:08:52 +02:00

irqdomain/treewide: Free firmware node after domain removal

Commit 711419e504eb ("irqdomain: Add the missing assignment of
domain->fwnode for named fwnode") unintentionally caused a dangling pointer
page fault issue on firmware nodes that were freed after IRQ domain
allocation. Commit e3beca48a45b fixed that dangling pointer issue by only
freeing the firmware node after an IRQ domain allocation failure. That fix
no longer frees the firmware node immediately, but leaves the firmware node
allocated after the domain is removed.

The firmware node must be kept around through irq_domain_remove, but should be
freed it afterwards.

Add the missing free operations after domain removal where where appropriate.

Fixes: e3beca48a45b ("irqdomain/treewide: Keep firmware node unconditionally allocated")
Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Bjorn Helgaas <bhelgaas@google.com>	# drivers/pci
Cc: stable@vger.kernel.org
Link: https://lkml.kernel.org/r/1595363169-7157-1-git-send-email-jonathan.derrick@intel.com

---
 arch/mips/pci/pci-xtalk-bridge.c    | 3 +++
 arch/x86/kernel/apic/io_apic.c      | 5 +++++
 drivers/iommu/intel/irq_remapping.c | 8 ++++++++
 drivers/mfd/ioc3.c                  | 6 ++++++
 drivers/pci/controller/vmd.c        | 3 +++
 5 files changed, 25 insertions(+)

diff --git a/arch/mips/pci/pci-xtalk-bridge.c b/arch/mips/pci/pci-xtalk-bridge.c
index 5958217..9b3cc77 100644
--- a/arch/mips/pci/pci-xtalk-bridge.c
+++ b/arch/mips/pci/pci-xtalk-bridge.c
@@ -728,6 +728,7 @@ err_free_resource:
 	pci_free_resource_list(&host->windows);
 err_remove_domain:
 	irq_domain_remove(domain);
+	irq_domain_free_fwnode(fn);
 	return err;
 }
 
@@ -735,8 +736,10 @@ static int bridge_remove(struct platform_device *pdev)
 {
 	struct pci_bus *bus = platform_get_drvdata(pdev);
 	struct bridge_controller *bc = BRIDGE_CONTROLLER(bus);
+	struct fwnode_handle *fn = bc->domain->fwnode;
 
 	irq_domain_remove(bc->domain);
+	irq_domain_free_fwnode(fn);
 	pci_lock_rescan_remove();
 	pci_stop_root_bus(bus);
 	pci_remove_root_bus(bus);
diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 81ffcfb..21325a4 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -2335,8 +2335,13 @@ static int mp_irqdomain_create(int ioapic)
 
 static void ioapic_destroy_irqdomain(int idx)
 {
+	struct ioapic_domain_cfg *cfg = &ioapics[idx].irqdomain_cfg;
+	struct fwnode_handle *fn = ioapics[idx].irqdomain->fwnode;
+
 	if (ioapics[idx].irqdomain) {
 		irq_domain_remove(ioapics[idx].irqdomain);
+		if (!cfg->dev)
+			irq_domain_free_fwnode(fn);
 		ioapics[idx].irqdomain = NULL;
 	}
 }
diff --git a/drivers/iommu/intel/irq_remapping.c b/drivers/iommu/intel/irq_remapping.c
index 9564d23..aa096b3 100644
--- a/drivers/iommu/intel/irq_remapping.c
+++ b/drivers/iommu/intel/irq_remapping.c
@@ -628,13 +628,21 @@ out_free_table:
 
 static void intel_teardown_irq_remapping(struct intel_iommu *iommu)
 {
+	struct fwnode_handle *fn;
+
 	if (iommu && iommu->ir_table) {
 		if (iommu->ir_msi_domain) {
+			fn = iommu->ir_msi_domain->fwnode;
+
 			irq_domain_remove(iommu->ir_msi_domain);
+			irq_domain_free_fwnode(fn);
 			iommu->ir_msi_domain = NULL;
 		}
 		if (iommu->ir_domain) {
+			fn = iommu->ir_domain->fwnode;
+
 			irq_domain_remove(iommu->ir_domain);
+			irq_domain_free_fwnode(fn);
 			iommu->ir_domain = NULL;
 		}
 		free_pages((unsigned long)iommu->ir_table->base,
diff --git a/drivers/mfd/ioc3.c b/drivers/mfd/ioc3.c
index 74cee7c..d939ccc 100644
--- a/drivers/mfd/ioc3.c
+++ b/drivers/mfd/ioc3.c
@@ -616,7 +616,10 @@ static int ioc3_mfd_probe(struct pci_dev *pdev,
 		/* Remove all already added MFD devices */
 		mfd_remove_devices(&ipd->pdev->dev);
 		if (ipd->domain) {
+			struct fwnode_handle *fn = ipd->domain->fwnode;
+
 			irq_domain_remove(ipd->domain);
+			irq_domain_free_fwnode(fn);
 			free_irq(ipd->domain_irq, (void *)ipd);
 		}
 		pci_iounmap(pdev, regs);
@@ -643,7 +646,10 @@ static void ioc3_mfd_remove(struct pci_dev *pdev)
 	/* Release resources */
 	mfd_remove_devices(&ipd->pdev->dev);
 	if (ipd->domain) {
+		struct fwnode_handle *fn = ipd->domain->fwnode;
+
 		irq_domain_remove(ipd->domain);
+		irq_domain_free_fwnode(fn);
 		free_irq(ipd->domain_irq, (void *)ipd);
 	}
 	pci_iounmap(pdev, ipd->regs);
diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index 9a64cf9..ebec0a6 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -560,6 +560,7 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
 	if (!vmd->bus) {
 		pci_free_resource_list(&resources);
 		irq_domain_remove(vmd->irq_domain);
+		irq_domain_free_fwnode(fn);
 		return -ENODEV;
 	}
 
@@ -673,6 +674,7 @@ static void vmd_cleanup_srcu(struct vmd_dev *vmd)
 static void vmd_remove(struct pci_dev *dev)
 {
 	struct vmd_dev *vmd = pci_get_drvdata(dev);
+	struct fwnode_handle *fn = vmd->irq_domain->fwnode;
 
 	sysfs_remove_link(&vmd->dev->dev.kobj, "domain");
 	pci_stop_root_bus(vmd->bus);
@@ -680,6 +682,7 @@ static void vmd_remove(struct pci_dev *dev)
 	vmd_cleanup_srcu(vmd);
 	vmd_detach_resources(vmd);
 	irq_domain_remove(vmd->irq_domain);
+	irq_domain_free_fwnode(fn);
 }
 
 #ifdef CONFIG_PM_SLEEP