[PATCH] PCI: vmd: Keep fwnode allocated through VMD irqdomain life

[PATCH] PCI: vmd: Keep fwnode allocated through VMD irqdomain life

[Jon Derrick]

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

The VMD domain does not subscribe to ACPI, and so does not operate on
it's irqdomain fwnode. It was freeing the handle after allocation of the
domain. As of 181e9d4efaf6a (irqdomain: Make __irq_domain_add() less
OF-dependent), the fwnode is put during irq_domain_remove causing a page
fault. This patch keeps VMD's fwnode allocated through the lifetime of
the VMD irqdomain.

Fixes: ae904cafd59d ("PCI/vmd: Create named irq domain")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Co-developed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
---
Hi Lorenzo, Bjorn,

Please take this patch for v5.8 fixes. It fixes an issue during VMD
unload.

Thanks
Jon

 drivers/pci/controller/vmd.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index e386d4eac407..ebec0a6e77ed 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -546,9 +546,10 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
 
 	vmd->irq_domain = pci_msi_create_irq_domain(fn, &vmd_msi_domain_info,
 						    x86_vector_domain);
-	irq_domain_free_fwnode(fn);
-	if (!vmd->irq_domain)
+	if (!vmd->irq_domain) {
+		irq_domain_free_fwnode(fn);
 		return -ENODEV;
+	}
 
 	pci_add_resource(&resources, &vmd->resources[0]);
 	pci_add_resource_offset(&resources, &vmd->resources[1], offset[0]);
@@ -559,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;
 	}
 
@@ -672,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);
@@ -679,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
-- 
2.18.1

[PATCH] PCI: vmd: Keep fwnode allocated through VMD irqdomain life

[Jon Derrick]

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

The VMD domain does not subscribe to ACPI, and so does not operate on
it's irqdomain fwnode. It was freeing the handle after allocation of the
domain. As of 181e9d4efaf6a (irqdomain: Make __irq_domain_add() less
OF-dependent), the fwnode is put during irq_domain_remove causing a page
fault. This patch keeps VMD's fwnode allocated through the lifetime of
the VMD irqdomain.

Fixes: ae904cafd59d ("PCI/vmd: Create named irq domain")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Co-developed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
---
Hi Lorenzo, Bjorn,

Please take this patch for v5.8 fixes. It fixes an issue during VMD
unload.

Thanks
Jon

 drivers/pci/controller/vmd.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index e386d4eac407..ebec0a6e77ed 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -546,9 +546,10 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
 
 	vmd->irq_domain = pci_msi_create_irq_domain(fn, &vmd_msi_domain_info,
 						    x86_vector_domain);
-	irq_domain_free_fwnode(fn);
-	if (!vmd->irq_domain)
+	if (!vmd->irq_domain) {
+		irq_domain_free_fwnode(fn);
 		return -ENODEV;
+	}
 
 	pci_add_resource(&resources, &vmd->resources[0]);
 	pci_add_resource_offset(&resources, &vmd->resources[1], offset[0]);
@@ -559,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;
 	}
 
@@ -672,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);
@@ -679,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
-- 
2.18.1

Re: [PATCH] PCI: vmd: Keep fwnode allocated through VMD irqdomain life

[Bjorn Helgaas]

[+cc Thomas]

On Thu, Jun 25, 2020 at 12:24:49PM -0400, Jon Derrick wrote:
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> The VMD domain does not subscribe to ACPI, and so does not operate on
> it's irqdomain fwnode. It was freeing the handle after allocation of the
> domain. As of 181e9d4efaf6a (irqdomain: Make __irq_domain_add() less
> OF-dependent), the fwnode is put during irq_domain_remove causing a page
> fault. This patch keeps VMD's fwnode allocated through the lifetime of
> the VMD irqdomain.
> 
> Fixes: ae904cafd59d ("PCI/vmd: Create named irq domain")
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Co-developed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
> ---
> Hi Lorenzo, Bjorn,
> 
> Please take this patch for v5.8 fixes. It fixes an issue during VMD
> unload.

I tentatively applied this to for-linus for v5.8.

But I would like to clarify the commit log.  It says this fixes
Thomas' ae904cafd59d ("PCI/vmd: Create named irq domain").  That
appeared in v4.13, which suggests that this patch should be backported
to v4.13 and later.

But it's not clear to me that ae904cafd59d is actually broken, since
the log also says the problem appeared after 181e9d4efaf6 ("irqdomain:
Make __irq_domain_add() less OF-dependent"), which we just merged for
v5.8-rc1.

And obviously, freeing the fwnode doesn't *cause* a page fault.  A
use-after-free might cause a fault, but it's not clear where that
happens.  I guess fwnode is used in the interval between:

  vmd_enable_domain
    pci_msi_create_irq_domain

  ...        <-- fwnode used here somewhere

  vmd_remove
    vmd_cleanup_srcu
      irq_domain_free_fwnode

But I can't tell how 181e9d4efaf6a and/or ae904cafd59d are related to
that.

>  drivers/pci/controller/vmd.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index e386d4eac407..ebec0a6e77ed 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
> @@ -546,9 +546,10 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
>  
>  	vmd->irq_domain = pci_msi_create_irq_domain(fn, &vmd_msi_domain_info,
>  						    x86_vector_domain);
> -	irq_domain_free_fwnode(fn);
> -	if (!vmd->irq_domain)
> +	if (!vmd->irq_domain) {
> +		irq_domain_free_fwnode(fn);
>  		return -ENODEV;
> +	}
>  
>  	pci_add_resource(&resources, &vmd->resources[0]);
>  	pci_add_resource_offset(&resources, &vmd->resources[1], offset[0]);
> @@ -559,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;
>  	}
>  
> @@ -672,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);
> @@ -679,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
> -- 
> 2.18.1
> 

Re: [PATCH] PCI: vmd: Keep fwnode allocated through VMD irqdomain life

[Derrick, Jonathan]

On Thu, 2020-06-25 at 14:58 -0500, Bjorn Helgaas wrote:
> [+cc Thomas]
> 
> On Thu, Jun 25, 2020 at 12:24:49PM -0400, Jon Derrick wrote:
> > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > 
> > The VMD domain does not subscribe to ACPI, and so does not operate on
> > it's irqdomain fwnode. It was freeing the handle after allocation of the
> > domain. As of 181e9d4efaf6a (irqdomain: Make __irq_domain_add() less
> > OF-dependent), the fwnode is put during irq_domain_remove causing a page
> > fault. This patch keeps VMD's fwnode allocated through the lifetime of
> > the VMD irqdomain.
> > 
> > Fixes: ae904cafd59d ("PCI/vmd: Create named irq domain")
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Co-developed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
> > ---
> > Hi Lorenzo, Bjorn,
> > 
> > Please take this patch for v5.8 fixes. It fixes an issue during VMD
> > unload.
> 
> I tentatively applied this to for-linus for v5.8.
> 
> But I would like to clarify the commit log.  It says this fixes
> Thomas' ae904cafd59d ("PCI/vmd: Create named irq domain").  That
> appeared in v4.13, which suggests that this patch should be backported
> to v4.13 and later.
> 
> But it's not clear to me that ae904cafd59d is actually broken, since
> the log also says the problem appeared after 181e9d4efaf6 ("irqdomain:
> Make __irq_domain_add() less OF-dependent"), which we just merged for
> v5.8-rc1.
> 
> And obviously, freeing the fwnode doesn't *cause* a page fault.  A
> use-after-free might cause a fault, but it's not clear where that
> happens.  I guess fwnode is used in the interval between:
> 
>   vmd_enable_domain
>     pci_msi_create_irq_domain
> 
>   ...        <-- fwnode used here somewhere
> 
>   vmd_remove
>     vmd_cleanup_srcu
>       irq_domain_free_fwnode
> 
> But I can't tell how 181e9d4efaf6a and/or ae904cafd59d are related to
> that.

The actual issue is that domain->fwnode was freed (and not set NULL),
leading to page fault here:
void irq_domain_remove(struct irq_domain *domain)
{
...
        fwnode_handle_put(domain->fwnode);

But it's not obvious to me that VMD has a reason to free fwnode if
there's other deps that rely on it existing

> 
> >  drivers/pci/controller/vmd.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> > index e386d4eac407..ebec0a6e77ed 100644
> > --- a/drivers/pci/controller/vmd.c
> > +++ b/drivers/pci/controller/vmd.c
> > @@ -546,9 +546,10 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
> >  
> >  	vmd->irq_domain = pci_msi_create_irq_domain(fn, &vmd_msi_domain_info,
> >  						    x86_vector_domain);
> > -	irq_domain_free_fwnode(fn);
> > -	if (!vmd->irq_domain)
> > +	if (!vmd->irq_domain) {
> > +		irq_domain_free_fwnode(fn);
> >  		return -ENODEV;
> > +	}
> >  
> >  	pci_add_resource(&resources, &vmd->resources[0]);
> >  	pci_add_resource_offset(&resources, &vmd->resources[1], offset[0]);
> > @@ -559,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;
> >  	}
> >  
> > @@ -672,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);
> > @@ -679,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
> > -- 
> > 2.18.1
> > 

Re: [PATCH] PCI: vmd: Keep fwnode allocated through VMD irqdomain life

[Bjorn Helgaas]

On Thu, Jun 25, 2020 at 02:58:23PM -0500, Bjorn Helgaas wrote:
> [+cc Thomas]
> 
> On Thu, Jun 25, 2020 at 12:24:49PM -0400, Jon Derrick wrote:
> > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > 
> > The VMD domain does not subscribe to ACPI, and so does not operate on
> > it's irqdomain fwnode. It was freeing the handle after allocation of the
> > domain. As of 181e9d4efaf6a (irqdomain: Make __irq_domain_add() less
> > OF-dependent), the fwnode is put during irq_domain_remove causing a page
> > fault. This patch keeps VMD's fwnode allocated through the lifetime of
> > the VMD irqdomain.
> > 
> > Fixes: ae904cafd59d ("PCI/vmd: Create named irq domain")
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Co-developed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
> > ---
> > Hi Lorenzo, Bjorn,
> > 
> > Please take this patch for v5.8 fixes. It fixes an issue during VMD
> > unload.
> 
> I tentatively applied this to for-linus for v5.8.
> 
> But I would like to clarify the commit log.  It says this fixes
> Thomas' ae904cafd59d ("PCI/vmd: Create named irq domain").  That
> appeared in v4.13, which suggests that this patch should be backported
> to v4.13 and later.

I didn't word this correctly.  What I meant was "I will consider
applying this after the commit log is clarified."  Just FYI that this
isn't resolved yet.

Since this is proposed for v5.8, I really want to understand this
better before asking Linus to pull it as a fix.

> But it's not clear to me that ae904cafd59d is actually broken, since
> the log also says the problem appeared after 181e9d4efaf6 ("irqdomain:
> Make __irq_domain_add() less OF-dependent"), which we just merged for
> v5.8-rc1.
> 
> And obviously, freeing the fwnode doesn't *cause* a page fault.  A
> use-after-free might cause a fault, but it's not clear where that
> happens.  I guess fwnode is used in the interval between:
> 
>   vmd_enable_domain
>     pci_msi_create_irq_domain
> 
>   ...        <-- fwnode used here somewhere
> 
>   vmd_remove
>     vmd_cleanup_srcu
>       irq_domain_free_fwnode
> 
> But I can't tell how 181e9d4efaf6a and/or ae904cafd59d are related to
> that.
> 
> >  drivers/pci/controller/vmd.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> > index e386d4eac407..ebec0a6e77ed 100644
> > --- a/drivers/pci/controller/vmd.c
> > +++ b/drivers/pci/controller/vmd.c
> > @@ -546,9 +546,10 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
> >  
> >  	vmd->irq_domain = pci_msi_create_irq_domain(fn, &vmd_msi_domain_info,
> >  						    x86_vector_domain);
> > -	irq_domain_free_fwnode(fn);
> > -	if (!vmd->irq_domain)
> > +	if (!vmd->irq_domain) {
> > +		irq_domain_free_fwnode(fn);
> >  		return -ENODEV;
> > +	}
> >  
> >  	pci_add_resource(&resources, &vmd->resources[0]);
> >  	pci_add_resource_offset(&resources, &vmd->resources[1], offset[0]);
> > @@ -559,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;
> >  	}
> >  
> > @@ -672,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);
> > @@ -679,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
> > -- 
> > 2.18.1
> > 

Re: [PATCH] PCI: vmd: Keep fwnode allocated through VMD irqdomain life

[Andy Shevchenko]

On Mon, Jun 29, 2020 at 06:20:11PM -0500, Bjorn Helgaas wrote:
> On Thu, Jun 25, 2020 at 02:58:23PM -0500, Bjorn Helgaas wrote:
> > [+cc Thomas]
> > 
> > On Thu, Jun 25, 2020 at 12:24:49PM -0400, Jon Derrick wrote:
> > > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > 
> > > The VMD domain does not subscribe to ACPI, and so does not operate on
> > > it's irqdomain fwnode. It was freeing the handle after allocation of the
> > > domain. As of 181e9d4efaf6a (irqdomain: Make __irq_domain_add() less
> > > OF-dependent), the fwnode is put during irq_domain_remove causing a page
> > > fault. This patch keeps VMD's fwnode allocated through the lifetime of
> > > the VMD irqdomain.
> > > 
> > > Fixes: ae904cafd59d ("PCI/vmd: Create named irq domain")
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > Co-developed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
> > > ---
> > > Hi Lorenzo, Bjorn,
> > > 
> > > Please take this patch for v5.8 fixes. It fixes an issue during VMD
> > > unload.
> > 
> > I tentatively applied this to for-linus for v5.8.
> > 
> > But I would like to clarify the commit log.  It says this fixes
> > Thomas' ae904cafd59d ("PCI/vmd: Create named irq domain").  That
> > appeared in v4.13, which suggests that this patch should be backported
> > to v4.13 and later.
> 
> I didn't word this correctly.  What I meant was "I will consider
> applying this after the commit log is clarified."  Just FYI that this
> isn't resolved yet.
> 
> Since this is proposed for v5.8, I really want to understand this
> better before asking Linus to pull it as a fix.

The problem here is in the original patch which relies on the knowledge that
fwnode is (was) not used anyhow specifically for ACPI case. That said, it makes
fwnode a dangling pointer which I personally consider as a mine left for
others. That's why the Fixes refers to the initial commit. The latter just
has been blasted on that mine.

If you think that's fine to have such trick, feel free to update Fixes tag.

> > But it's not clear to me that ae904cafd59d is actually broken, since
> > the log also says the problem appeared after 181e9d4efaf6 ("irqdomain:
> > Make __irq_domain_add() less OF-dependent"), which we just merged for
> > v5.8-rc1.
> > 
> > And obviously, freeing the fwnode doesn't *cause* a page fault.  A
> > use-after-free might cause a fault, but it's not clear where that
> > happens.  I guess fwnode is used in the interval between:
> > 
> >   vmd_enable_domain
> >     pci_msi_create_irq_domain
> > 
> >   ...        <-- fwnode used here somewhere
> > 
> >   vmd_remove
> >     vmd_cleanup_srcu
> >       irq_domain_free_fwnode
> > 
> > But I can't tell how 181e9d4efaf6a and/or ae904cafd59d are related to
> > that.
> > 
> > >  drivers/pci/controller/vmd.c | 8 ++++++--
> > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> > > index e386d4eac407..ebec0a6e77ed 100644
> > > --- a/drivers/pci/controller/vmd.c
> > > +++ b/drivers/pci/controller/vmd.c
> > > @@ -546,9 +546,10 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
> > >  
> > >  	vmd->irq_domain = pci_msi_create_irq_domain(fn, &vmd_msi_domain_info,
> > >  						    x86_vector_domain);
> > > -	irq_domain_free_fwnode(fn);
> > > -	if (!vmd->irq_domain)
> > > +	if (!vmd->irq_domain) {
> > > +		irq_domain_free_fwnode(fn);
> > >  		return -ENODEV;
> > > +	}
> > >  
> > >  	pci_add_resource(&resources, &vmd->resources[0]);
> > >  	pci_add_resource_offset(&resources, &vmd->resources[1], offset[0]);
> > > @@ -559,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;
> > >  	}
> > >  
> > > @@ -672,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);
> > > @@ -679,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
> > > -- 
> > > 2.18.1
> > > 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] PCI: vmd: Keep fwnode allocated through VMD irqdomain life

[Bjorn Helgaas]

On Tue, Jun 30, 2020 at 12:39:08PM +0300, Andy Shevchenko wrote:
> On Mon, Jun 29, 2020 at 06:20:11PM -0500, Bjorn Helgaas wrote:
> > On Thu, Jun 25, 2020 at 02:58:23PM -0500, Bjorn Helgaas wrote:
> > > [+cc Thomas]
> > > 
> > > On Thu, Jun 25, 2020 at 12:24:49PM -0400, Jon Derrick wrote:
> > > > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > 
> > > > The VMD domain does not subscribe to ACPI, and so does not operate on
> > > > it's irqdomain fwnode. It was freeing the handle after allocation of the
> > > > domain. As of 181e9d4efaf6a (irqdomain: Make __irq_domain_add() less
> > > > OF-dependent), the fwnode is put during irq_domain_remove causing a page
> > > > fault. This patch keeps VMD's fwnode allocated through the lifetime of
> > > > the VMD irqdomain.
> > > > 
> > > > Fixes: ae904cafd59d ("PCI/vmd: Create named irq domain")
> > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > Co-developed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
> > > > ---
> > > > Hi Lorenzo, Bjorn,
> > > > 
> > > > Please take this patch for v5.8 fixes. It fixes an issue during VMD
> > > > unload.
> > > 
> > > I tentatively applied this to for-linus for v5.8.
> > > 
> > > But I would like to clarify the commit log.  It says this fixes
> > > Thomas' ae904cafd59d ("PCI/vmd: Create named irq domain").  That
> > > appeared in v4.13, which suggests that this patch should be backported
> > > to v4.13 and later.
> > 
> > I didn't word this correctly.  What I meant was "I will consider
> > applying this after the commit log is clarified."  Just FYI that this
> > isn't resolved yet.
> > 
> > Since this is proposed for v5.8, I really want to understand this
> > better before asking Linus to pull it as a fix.
> 
> The problem here is in the original patch which relies on the
> knowledge that fwnode is (was) not used anyhow specifically for ACPI
> case. That said, it makes fwnode a dangling pointer which I
> personally consider as a mine left for others. That's why the Fixes
> refers to the initial commit. The latter just has been blasted on
> that mine.

IIUC, you're saying this pattern:

  fwnode = irq_domain_alloc_named_id_fwnode(...)
  irq_domain = pci_msi_create_irq_domain(fwnode, ...)
  irq_domain_free_fwnode(fwnode)

leaves a dangling fwnode pointer.  That does look suspicious because
__irq_domain_add() saves fwnode:

  irq_domain = pci_msi_create_irq_domain(fwnode, ...)
    msi_create_irq_domain(fwnode, ...)
      irq_domain_create_hierarchy(..., fwnode, ...)
	irq_domain_create_linear(fwnode, ...)
	  __irq_domain_add(fwnode, ...)
	    domain->fwnode = fwnode

and irq_domain_free_fwnode() frees it.  But I'm confused because there
are several other instances of this pattern:

  bridge_probe()                      # arch/mips/pci/pci-xtalk-bridge.c
  mp_irqdomain_create()
  arch_init_msi_domain()
  arch_create_remap_msi_irq_domain()
  dmar_get_irq_domain()
  hpet_create_irq_domain()
  ...

Are they all wrong?  I definitely think it's a bad idea to keep a copy
of a pointer after we free the data it points to.  But if they're all
wrong, I don't want to fix just one and leave all the others.

Thomas, can you enlighten us?

> If you think that's fine to have such trick, feel free to update Fixes tag.
> 
> > > But it's not clear to me that ae904cafd59d is actually broken, since
> > > the log also says the problem appeared after 181e9d4efaf6 ("irqdomain:
> > > Make __irq_domain_add() less OF-dependent"), which we just merged for
> > > v5.8-rc1.
> > > 
> > > And obviously, freeing the fwnode doesn't *cause* a page fault.  A
> > > use-after-free might cause a fault, but it's not clear where that
> > > happens.  I guess fwnode is used in the interval between:
> > > 
> > >   vmd_enable_domain
> > >     pci_msi_create_irq_domain
> > > 
> > >   ...        <-- fwnode used here somewhere
> > > 
> > >   vmd_remove
> > >     vmd_cleanup_srcu
> > >       irq_domain_free_fwnode
> > > 
> > > But I can't tell how 181e9d4efaf6a and/or ae904cafd59d are related to
> > > that.
> > > 
> > > >  drivers/pci/controller/vmd.c | 8 ++++++--
> > > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> > > > index e386d4eac407..ebec0a6e77ed 100644
> > > > --- a/drivers/pci/controller/vmd.c
> > > > +++ b/drivers/pci/controller/vmd.c
> > > > @@ -546,9 +546,10 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
> > > >  
> > > >  	vmd->irq_domain = pci_msi_create_irq_domain(fn, &vmd_msi_domain_info,
> > > >  						    x86_vector_domain);
> > > > -	irq_domain_free_fwnode(fn);
> > > > -	if (!vmd->irq_domain)
> > > > +	if (!vmd->irq_domain) {
> > > > +		irq_domain_free_fwnode(fn);
> > > >  		return -ENODEV;
> > > > +	}
> > > >  
> > > >  	pci_add_resource(&resources, &vmd->resources[0]);
> > > >  	pci_add_resource_offset(&resources, &vmd->resources[1], offset[0]);
> > > > @@ -559,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;
> > > >  	}
> > > >  
> > > > @@ -672,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);
> > > > @@ -679,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
> > > > -- 
> > > > 2.18.1
> > > > 
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH] PCI: vmd: Keep fwnode allocated through VMD irqdomain life

[Derrick, Jonathan]

On Tue, 2020-06-30 at 11:33 -0500, Bjorn Helgaas wrote:
> On Tue, Jun 30, 2020 at 12:39:08PM +0300, Andy Shevchenko wrote:
> > On Mon, Jun 29, 2020 at 06:20:11PM -0500, Bjorn Helgaas wrote:
> > > On Thu, Jun 25, 2020 at 02:58:23PM -0500, Bjorn Helgaas wrote:
> > > > [+cc Thomas]
> > > > 
> > > > On Thu, Jun 25, 2020 at 12:24:49PM -0400, Jon Derrick wrote:
> > > > > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > > 
> > > > > The VMD domain does not subscribe to ACPI, and so does not operate on
> > > > > it's irqdomain fwnode. It was freeing the handle after allocation of the
> > > > > domain. As of 181e9d4efaf6a (irqdomain: Make __irq_domain_add() less
> > > > > OF-dependent), the fwnode is put during irq_domain_remove causing a page
> > > > > fault. This patch keeps VMD's fwnode allocated through the lifetime of
> > > > > the VMD irqdomain.
> > > > > 
> > > > > Fixes: ae904cafd59d ("PCI/vmd: Create named irq domain")
> > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > > Co-developed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > > Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
> > > > > ---
> > > > > Hi Lorenzo, Bjorn,
> > > > > 
> > > > > Please take this patch for v5.8 fixes. It fixes an issue during VMD
> > > > > unload.
> > > > 
> > > > I tentatively applied this to for-linus for v5.8.
> > > > 
> > > > But I would like to clarify the commit log.  It says this fixes
> > > > Thomas' ae904cafd59d ("PCI/vmd: Create named irq domain").  That
> > > > appeared in v4.13, which suggests that this patch should be backported
> > > > to v4.13 and later.
> > > 
> > > I didn't word this correctly.  What I meant was "I will consider
> > > applying this after the commit log is clarified."  Just FYI that this
> > > isn't resolved yet.
> > > 
> > > Since this is proposed for v5.8, I really want to understand this
> > > better before asking Linus to pull it as a fix.
> > 
> > The problem here is in the original patch which relies on the
> > knowledge that fwnode is (was) not used anyhow specifically for ACPI
> > case. That said, it makes fwnode a dangling pointer which I
> > personally consider as a mine left for others. That's why the Fixes
> > refers to the initial commit. The latter just has been blasted on
> > that mine.
> 
> IIUC, you're saying this pattern:
> 
>   fwnode = irq_domain_alloc_named_id_fwnode(...)
>   irq_domain = pci_msi_create_irq_domain(fwnode, ...)
>   irq_domain_free_fwnode(fwnode)
> 
> leaves a dangling fwnode pointer.  That does look suspicious because
> __irq_domain_add() saves fwnode:
> 
>   irq_domain = pci_msi_create_irq_domain(fwnode, ...)
>     msi_create_irq_domain(fwnode, ...)
>       irq_domain_create_hierarchy(..., fwnode, ...)
> 	irq_domain_create_linear(fwnode, ...)
> 	  __irq_domain_add(fwnode, ...)
> 	    domain->fwnode = fwnode
> 
> and irq_domain_free_fwnode() frees it.  But I'm confused because there
> are several other instances of this pattern:
> 
>   bridge_probe()                      # arch/mips/pci/pci-xtalk-bridge.c
>   mp_irqdomain_create()
>   arch_init_msi_domain()
>   arch_create_remap_msi_irq_domain()
>   dmar_get_irq_domain()
>   hpet_create_irq_domain()
>   ...
> 
> Are they all wrong?  I definitely think it's a bad idea to keep a copy
> of a pointer after we free the data it points to.  But if they're all
> wrong, I don't want to fix just one and leave all the others.
> 
> Thomas, can you enlighten us?
> 

I see that struct irqchip_fwid contains the actual fwnode structure and
when that is freed, it's causes the issue.

I'm noticing in each caller of irq_domain_free_fwnode, we have the
domain itself available. It seems like it should be up to the caller to
deal with the fwnode pointer, but we could just do:

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index a4c2c915511d..61f0070285ff 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -101,7 +101,7 @@ EXPORT_SYMBOL_GPL(__irq_domain_alloc_fwnode);
  *
  * Free a fwnode_handle allocated with irq_domain_alloc_fwnode.
  */
-void irq_domain_free_fwnode(struct fwnode_handle *fwnode)
+void irq_domain_free_fwnode(struct irq_domain *domain, struct
fwnode_handle *fwnode)
 {
        struct irqchip_fwid *fwid;
 
@@ -109,6 +109,7 @@ void irq_domain_free_fwnode(struct fwnode_handle
*fwnode)
                return;
 
        fwid = container_of(fwnode, struct irqchip_fwid, fwnode);
+       domain->fwnode = NULL;
        kfree(fwid->name);
        kfree(fwid);
 }



> > If you think that's fine to have such trick, feel free to update Fixes tag.
> > 
> > > > But it's not clear to me that ae904cafd59d is actually broken, since
> > > > the log also says the problem appeared after 181e9d4efaf6 ("irqdomain:
> > > > Make __irq_domain_add() less OF-dependent"), which we just merged for
> > > > v5.8-rc1.
> > > > 
> > > > And obviously, freeing the fwnode doesn't *cause* a page fault.  A
> > > > use-after-free might cause a fault, but it's not clear where that
> > > > happens.  I guess fwnode is used in the interval between:
> > > > 
> > > >   vmd_enable_domain
> > > >     pci_msi_create_irq_domain
> > > > 
> > > >   ...        <-- fwnode used here somewhere
> > > > 
> > > >   vmd_remove
> > > >     vmd_cleanup_srcu
> > > >       irq_domain_free_fwnode
> > > > 
> > > > But I can't tell how 181e9d4efaf6a and/or ae904cafd59d are related to
> > > > that.
> > > > 
> > > > >  drivers/pci/controller/vmd.c | 8 ++++++--
> > > > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> > > > > index e386d4eac407..ebec0a6e77ed 100644
> > > > > --- a/drivers/pci/controller/vmd.c
> > > > > +++ b/drivers/pci/controller/vmd.c
> > > > > @@ -546,9 +546,10 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
> > > > >  
> > > > >  	vmd->irq_domain = pci_msi_create_irq_domain(fn, &vmd_msi_domain_info,
> > > > >  						    x86_vector_domain);
> > > > > -	irq_domain_free_fwnode(fn);
> > > > > -	if (!vmd->irq_domain)
> > > > > +	if (!vmd->irq_domain) {
> > > > > +		irq_domain_free_fwnode(fn);
> > > > >  		return -ENODEV;
> > > > > +	}
> > > > >  
> > > > >  	pci_add_resource(&resources, &vmd->resources[0]);
> > > > >  	pci_add_resource_offset(&resources, &vmd->resources[1], offset[0]);
> > > > > @@ -559,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;
> > > > >  	}
> > > > >  
> > > > > @@ -672,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);
> > > > > @@ -679,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
> > > > > -- 
> > > > > 2.18.1
> > > > > 
> > 
> > -- 
> > With Best Regards,
> > Andy Shevchenko
> > 
> > 

Re: [PATCH] PCI: vmd: Keep fwnode allocated through VMD irqdomain life

[andriy.shevchenko]

On Sat, Jul 04, 2020 at 01:44:59AM +0000, Derrick, Jonathan wrote:
> On Tue, 2020-06-30 at 11:33 -0500, Bjorn Helgaas wrote:
> > On Tue, Jun 30, 2020 at 12:39:08PM +0300, Andy Shevchenko wrote:
> > > On Mon, Jun 29, 2020 at 06:20:11PM -0500, Bjorn Helgaas wrote:
> > > > On Thu, Jun 25, 2020 at 02:58:23PM -0500, Bjorn Helgaas wrote:
> > > > > [+cc Thomas]
> > > > > 
> > > > > On Thu, Jun 25, 2020 at 12:24:49PM -0400, Jon Derrick wrote:
> > > > > > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > > > 
> > > > > > The VMD domain does not subscribe to ACPI, and so does not operate on
> > > > > > it's irqdomain fwnode. It was freeing the handle after allocation of the
> > > > > > domain. As of 181e9d4efaf6a (irqdomain: Make __irq_domain_add() less
> > > > > > OF-dependent), the fwnode is put during irq_domain_remove causing a page
> > > > > > fault. This patch keeps VMD's fwnode allocated through the lifetime of
> > > > > > the VMD irqdomain.
> > > > > > 
> > > > > > Fixes: ae904cafd59d ("PCI/vmd: Create named irq domain")
> > > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > > > Co-developed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > > > Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
> > > > > > ---
> > > > > > Hi Lorenzo, Bjorn,
> > > > > > 
> > > > > > Please take this patch for v5.8 fixes. It fixes an issue during VMD
> > > > > > unload.
> > > > > 
> > > > > I tentatively applied this to for-linus for v5.8.
> > > > > 
> > > > > But I would like to clarify the commit log.  It says this fixes
> > > > > Thomas' ae904cafd59d ("PCI/vmd: Create named irq domain").  That
> > > > > appeared in v4.13, which suggests that this patch should be backported
> > > > > to v4.13 and later.
> > > > 
> > > > I didn't word this correctly.  What I meant was "I will consider
> > > > applying this after the commit log is clarified."  Just FYI that this
> > > > isn't resolved yet.
> > > > 
> > > > Since this is proposed for v5.8, I really want to understand this
> > > > better before asking Linus to pull it as a fix.
> > > 
> > > The problem here is in the original patch which relies on the
> > > knowledge that fwnode is (was) not used anyhow specifically for ACPI
> > > case. That said, it makes fwnode a dangling pointer which I
> > > personally consider as a mine left for others. That's why the Fixes
> > > refers to the initial commit. The latter just has been blasted on
> > > that mine.
> > 
> > IIUC, you're saying this pattern:
> > 
> >   fwnode = irq_domain_alloc_named_id_fwnode(...)
> >   irq_domain = pci_msi_create_irq_domain(fwnode, ...)
> >   irq_domain_free_fwnode(fwnode)
> > 
> > leaves a dangling fwnode pointer.  That does look suspicious because
> > __irq_domain_add() saves fwnode:
> > 
> >   irq_domain = pci_msi_create_irq_domain(fwnode, ...)
> >     msi_create_irq_domain(fwnode, ...)
> >       irq_domain_create_hierarchy(..., fwnode, ...)
> > 	irq_domain_create_linear(fwnode, ...)
> > 	  __irq_domain_add(fwnode, ...)
> > 	    domain->fwnode = fwnode
> > 
> > and irq_domain_free_fwnode() frees it.  But I'm confused because there
> > are several other instances of this pattern:
> > 
> >   bridge_probe()                      # arch/mips/pci/pci-xtalk-bridge.c
> >   mp_irqdomain_create()
> >   arch_init_msi_domain()
> >   arch_create_remap_msi_irq_domain()
> >   dmar_get_irq_domain()
> >   hpet_create_irq_domain()
> >   ...
> > 
> > Are they all wrong?  I definitely think it's a bad idea to keep a copy
> > of a pointer after we free the data it points to.  But if they're all
> > wrong, I don't want to fix just one and leave all the others.
> > 
> > Thomas, can you enlighten us?
> > 
> 
> I see that struct irqchip_fwid contains the actual fwnode structure and
> when that is freed, it's causes the issue.
> 
> I'm noticing in each caller of irq_domain_free_fwnode, we have the
> domain itself available. It seems like it should be up to the caller to
> deal with the fwnode pointer, but we could just do:

It might work as well, but also needs a good documentation.

> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index a4c2c915511d..61f0070285ff 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -101,7 +101,7 @@ EXPORT_SYMBOL_GPL(__irq_domain_alloc_fwnode);
>   *
>   * Free a fwnode_handle allocated with irq_domain_alloc_fwnode.
>   */

> -void irq_domain_free_fwnode(struct fwnode_handle *fwnode)
> +void irq_domain_free_fwnode(struct irq_domain *domain, struct
> fwnode_handle *fwnode)

Isn't fwnode == domain->fwnode here and second parameter won't be necessary?

>  {
>         struct irqchip_fwid *fwid;
>  
> @@ -109,6 +109,7 @@ void irq_domain_free_fwnode(struct fwnode_handle
> *fwnode)
>                 return;
>  
>         fwid = container_of(fwnode, struct irqchip_fwid, fwnode);
> +       domain->fwnode = NULL;
>         kfree(fwid->name);
>         kfree(fwid);
>  }
> 
> 
> 
> > > If you think that's fine to have such trick, feel free to update Fixes tag.
> > > 
> > > > > But it's not clear to me that ae904cafd59d is actually broken, since
> > > > > the log also says the problem appeared after 181e9d4efaf6 ("irqdomain:
> > > > > Make __irq_domain_add() less OF-dependent"), which we just merged for
> > > > > v5.8-rc1.
> > > > > 
> > > > > And obviously, freeing the fwnode doesn't *cause* a page fault.  A
> > > > > use-after-free might cause a fault, but it's not clear where that
> > > > > happens.  I guess fwnode is used in the interval between:
> > > > > 
> > > > >   vmd_enable_domain
> > > > >     pci_msi_create_irq_domain
> > > > > 
> > > > >   ...        <-- fwnode used here somewhere
> > > > > 
> > > > >   vmd_remove
> > > > >     vmd_cleanup_srcu
> > > > >       irq_domain_free_fwnode
> > > > > 
> > > > > But I can't tell how 181e9d4efaf6a and/or ae904cafd59d are related to
> > > > > that.
> > > > > 
> > > > > >  drivers/pci/controller/vmd.c | 8 ++++++--
> > > > > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> > > > > > index e386d4eac407..ebec0a6e77ed 100644
> > > > > > --- a/drivers/pci/controller/vmd.c
> > > > > > +++ b/drivers/pci/controller/vmd.c
> > > > > > @@ -546,9 +546,10 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
> > > > > >  
> > > > > >  	vmd->irq_domain = pci_msi_create_irq_domain(fn, &vmd_msi_domain_info,
> > > > > >  						    x86_vector_domain);
> > > > > > -	irq_domain_free_fwnode(fn);
> > > > > > -	if (!vmd->irq_domain)
> > > > > > +	if (!vmd->irq_domain) {
> > > > > > +		irq_domain_free_fwnode(fn);
> > > > > >  		return -ENODEV;
> > > > > > +	}
> > > > > >  
> > > > > >  	pci_add_resource(&resources, &vmd->resources[0]);
> > > > > >  	pci_add_resource_offset(&resources, &vmd->resources[1], offset[0]);
> > > > > > @@ -559,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;
> > > > > >  	}
> > > > > >  
> > > > > > @@ -672,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);
> > > > > > @@ -679,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
> > > > > > -- 
> > > > > > 2.18.1
> > > > > > 
> > > 
> > > -- 
> > > With Best Regards,
> > > Andy Shevchenko
> > > 
> > > 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] PCI: vmd: Keep fwnode allocated through VMD irqdomain life

[Thomas Gleixner]

"Derrick, Jonathan" <jonathan.derrick@intel.com> writes:
> On Tue, 2020-06-30 at 11:33 -0500, Bjorn Helgaas wrote:
> I see that struct irqchip_fwid contains the actual fwnode structure and
> when that is freed, it's causes the issue.
>
> I'm noticing in each caller of irq_domain_free_fwnode, we have the
> domain itself available. It seems like it should be up to the caller to
> deal with the fwnode pointer, but we could just do:

Why? The fwnode pointer handling is inconsistent for the various fwnode
types. We really don't want to go back to that state.

Thanks,

        tglx

Re: [PATCH] PCI: vmd: Keep fwnode allocated through VMD irqdomain life

[Derrick, Jonathan]

On Tue, 2020-07-14 at 17:40 +0200, Thomas Gleixner wrote:
> "Derrick, Jonathan" <jonathan.derrick@intel.com> writes:
> > On Tue, 2020-06-30 at 11:33 -0500, Bjorn Helgaas wrote:
> > I see that struct irqchip_fwid contains the actual fwnode structure and
> > when that is freed, it's causes the issue.
> > 
> > I'm noticing in each caller of irq_domain_free_fwnode, we have the
> > domain itself available. It seems like it should be up to the caller to
> > deal with the fwnode pointer, but we could just do:
> 
> Why? The fwnode pointer handling is inconsistent for the various fwnode
> types.
I see... That does explain a lot.

>  We really don't want to go back to that state.
> 
> Thanks,
> 
>         tglx

Thanks for the prompt fix

Jon

Re: [PATCH] PCI: vmd: Keep fwnode allocated through VMD irqdomain life

[Thomas Gleixner]

Bjorn Helgaas <helgaas@kernel.org> writes:
> On Tue, Jun 30, 2020 at 12:39:08PM +0300, Andy Shevchenko wrote:
>> The problem here is in the original patch which relies on the
>> knowledge that fwnode is (was) not used anyhow specifically for ACPI
>> case. That said, it makes fwnode a dangling pointer which I
>> personally consider as a mine left for others. That's why the Fixes
>> refers to the initial commit. The latter just has been blasted on
>> that mine.

No. The original patch did not create a dangling pointer because fwnode
was not stored for IRQCHIP_FWNODE_NAMED and IRQCHIP_FWNODE_NAMED_ID type
nodes.

The fail was introduced in:

711419e504eb ("irqdomain: Add the missing assignment of domain->fwnode for named fwnode")

> IIUC, you're saying this pattern:
>
>   fwnode = irq_domain_alloc_named_id_fwnode(...)
>   irq_domain = pci_msi_create_irq_domain(fwnode, ...)
>   irq_domain_free_fwnode(fwnode)
>
> leaves a dangling fwnode pointer.  That does look suspicious because
> __irq_domain_add() saves fwnode:
>
>   irq_domain = pci_msi_create_irq_domain(fwnode, ...)
>     msi_create_irq_domain(fwnode, ...)
>       irq_domain_create_hierarchy(..., fwnode, ...)
> 	irq_domain_create_linear(fwnode, ...)
> 	  __irq_domain_add(fwnode, ...)
> 	    domain->fwnode = fwnode
>
> and irq_domain_free_fwnode() frees it.  But I'm confused because there
> are several other instances of this pattern:
>
>   bridge_probe()                      # arch/mips/pci/pci-xtalk-bridge.c
>   mp_irqdomain_create()
>   arch_init_msi_domain()
>   arch_create_remap_msi_irq_domain()
>   dmar_get_irq_domain()
>   hpet_create_irq_domain()
>   ...
>
> Are they all wrong?  I definitely think it's a bad idea to keep a copy
> of a pointer after we free the data it points to.  But if they're all
> wrong, I don't want to fix just one and leave all the others.
>
> Thomas, can you enlighten us?

Did so. And yes, all instances which do alloc/create/free are busted
since that commit.

Thanks,

        tglx

Re: [PATCH] PCI: vmd: Keep fwnode allocated through VMD irqdomain life

[Andy Shevchenko]

On Mon, Jul 06, 2020 at 12:47:59PM +0200, Thomas Gleixner wrote:
> Bjorn Helgaas <helgaas@kernel.org> writes:
> > On Tue, Jun 30, 2020 at 12:39:08PM +0300, Andy Shevchenko wrote:
> >> The problem here is in the original patch which relies on the
> >> knowledge that fwnode is (was) not used anyhow specifically for ACPI
> >> case. That said, it makes fwnode a dangling pointer which I
> >> personally consider as a mine left for others. That's why the Fixes
> >> refers to the initial commit. The latter just has been blasted on
> >> that mine.
> 
> No. The original patch did not create a dangling pointer because fwnode
> was not stored for IRQCHIP_FWNODE_NAMED and IRQCHIP_FWNODE_NAMED_ID type
> nodes.
> 
> The fail was introduced in:
> 
> 711419e504eb ("irqdomain: Add the missing assignment of domain->fwnode for named fwnode")

Ah, sorry for missing that and thank you for pointing out.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] PCI: vmd: Keep fwnode allocated through VMD irqdomain life

[Thomas Gleixner]

Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> On Mon, Jul 06, 2020 at 12:47:59PM +0200, Thomas Gleixner wrote:
>> Bjorn Helgaas <helgaas@kernel.org> writes:
>> > On Tue, Jun 30, 2020 at 12:39:08PM +0300, Andy Shevchenko wrote:
>> >> The problem here is in the original patch which relies on the
>> >> knowledge that fwnode is (was) not used anyhow specifically for ACPI
>> >> case. That said, it makes fwnode a dangling pointer which I
>> >> personally consider as a mine left for others. That's why the Fixes
>> >> refers to the initial commit. The latter just has been blasted on
>> >> that mine.
>> 
>> No. The original patch did not create a dangling pointer because fwnode
>> was not stored for IRQCHIP_FWNODE_NAMED and IRQCHIP_FWNODE_NAMED_ID type
>> nodes.
>> 
>> The fail was introduced in:
>> 
>> 711419e504eb ("irqdomain: Add the missing assignment of domain->fwnode for named fwnode")
>
> Ah, sorry for missing that and thank you for pointing out.

So something like the below wants to be applied and marked for stable

Thanks,

        tglx
---
diff --git a/arch/mips/pci/pci-xtalk-bridge.c b/arch/mips/pci/pci-xtalk-bridge.c
index 3b2552fb7735..5958217861b8 100644
--- a/arch/mips/pci/pci-xtalk-bridge.c
+++ b/arch/mips/pci/pci-xtalk-bridge.c
@@ -627,9 +627,10 @@ static int bridge_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	domain = irq_domain_create_hierarchy(parent, 0, 8, fn,
 					     &bridge_domain_ops, NULL);
-	irq_domain_free_fwnode(fn);
-	if (!domain)
+	if (!domain) {
+		irq_domain_free_fwnode(fn);
 		return -ENOMEM;
+	}
 
 	pci_set_flags(PCI_PROBE_ONLY);
 
diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index ce61e3e7d399..81ffcfbfaef2 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -2316,12 +2316,12 @@ static int mp_irqdomain_create(int ioapic)
 	ip->irqdomain = irq_domain_create_linear(fn, hwirqs, cfg->ops,
 						 (void *)(long)ioapic);
 
-	/* Release fw handle if it was allocated above */
-	if (!cfg->dev)
-		irq_domain_free_fwnode(fn);
-
-	if (!ip->irqdomain)
+	if (!ip->irqdomain) {
+		/* Release fw handle if it was allocated above */
+		if (!cfg->dev)
+			irq_domain_free_fwnode(fn);
 		return -ENOMEM;
+	}
 
 	ip->irqdomain->parent = parent;
 
diff --git a/arch/x86/kernel/apic/msi.c b/arch/x86/kernel/apic/msi.c
index 5cbaca58af95..c2b2911feeef 100644
--- a/arch/x86/kernel/apic/msi.c
+++ b/arch/x86/kernel/apic/msi.c
@@ -263,12 +263,13 @@ void __init arch_init_msi_domain(struct irq_domain *parent)
 		msi_default_domain =
 			pci_msi_create_irq_domain(fn, &pci_msi_domain_info,
 						  parent);
-		irq_domain_free_fwnode(fn);
 	}
-	if (!msi_default_domain)
+	if (!msi_default_domain) {
+		irq_domain_free_fwnode(fn);
 		pr_warn("failed to initialize irqdomain for MSI/MSI-x.\n");
-	else
+	} else {
 		msi_default_domain->flags |= IRQ_DOMAIN_MSI_NOMASK_QUIRK;
+	}
 }
 
 #ifdef CONFIG_IRQ_REMAP
@@ -301,7 +302,8 @@ struct irq_domain *arch_create_remap_msi_irq_domain(struct irq_domain *parent,
 	if (!fn)
 		return NULL;
 	d = pci_msi_create_irq_domain(fn, &pci_msi_ir_domain_info, parent);
-	irq_domain_free_fwnode(fn);
+	if (!d)
+		irq_domain_free_fwnode(fn);
 	return d;
 }
 #endif
@@ -364,7 +366,8 @@ static struct irq_domain *dmar_get_irq_domain(void)
 	if (fn) {
 		dmar_domain = msi_create_irq_domain(fn, &dmar_msi_domain_info,
 						    x86_vector_domain);
-		irq_domain_free_fwnode(fn);
+		if (!dmar_domain)
+			irq_domain_free_fwnode(fn);
 	}
 out:
 	mutex_unlock(&dmar_lock);
@@ -489,7 +492,10 @@ struct irq_domain *hpet_create_irq_domain(int hpet_id)
 	}
 
 	d = msi_create_irq_domain(fn, domain_info, parent);
-	irq_domain_free_fwnode(fn);
+	if (!d) {
+		irq_domain_free_fwnode(fn);
+		kfree(domain_info);
+	}
 	return d;
 }
 
diff --git a/arch/x86/kernel/apic/vector.c b/arch/x86/kernel/apic/vector.c
index c48be6e1f676..cc8b16f89dd4 100644
--- a/arch/x86/kernel/apic/vector.c
+++ b/arch/x86/kernel/apic/vector.c
@@ -709,7 +709,6 @@ int __init arch_early_irq_init(void)
 	x86_vector_domain = irq_domain_create_tree(fn, &x86_vector_domain_ops,
 						   NULL);
 	BUG_ON(x86_vector_domain == NULL);
-	irq_domain_free_fwnode(fn);
 	irq_set_default_host(x86_vector_domain);
 
 	arch_init_msi_domain(x86_vector_domain);
diff --git a/arch/x86/platform/uv/uv_irq.c b/arch/x86/platform/uv/uv_irq.c
index fc13cbbb2dce..abb6075397f0 100644
--- a/arch/x86/platform/uv/uv_irq.c
+++ b/arch/x86/platform/uv/uv_irq.c
@@ -167,9 +167,10 @@ static struct irq_domain *uv_get_irq_domain(void)
 		goto out;
 
 	uv_domain = irq_domain_create_tree(fn, &uv_domain_ops, NULL);
-	irq_domain_free_fwnode(fn);
 	if (uv_domain)
 		uv_domain->parent = x86_vector_domain;
+	else
+		irq_domain_free_fwnode(fn);
 out:
 	mutex_unlock(&uv_lock);
 
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 74cca1757172..2f22326ee4df 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -3985,9 +3985,10 @@ int amd_iommu_create_irq_domain(struct amd_iommu *iommu)
 	if (!fn)
 		return -ENOMEM;
 	iommu->ir_domain = irq_domain_create_tree(fn, &amd_ir_domain_ops, iommu);
-	irq_domain_free_fwnode(fn);
-	if (!iommu->ir_domain)
+	if (!iommu->ir_domain) {
+		irq_domain_free_fwnode(fn);
 		return -ENOMEM;
+	}
 
 	iommu->ir_domain->parent = arch_get_ir_parent_domain();
 	iommu->msi_domain = arch_create_remap_msi_irq_domain(iommu->ir_domain,
diff --git a/drivers/iommu/hyperv-iommu.c b/drivers/iommu/hyperv-iommu.c
index 3c0c67a99c7b..8919c1c70b68 100644
--- a/drivers/iommu/hyperv-iommu.c
+++ b/drivers/iommu/hyperv-iommu.c
@@ -155,7 +155,10 @@ static int __init hyperv_prepare_irq_remapping(void)
 				0, IOAPIC_REMAPPING_ENTRY, fn,
 				&hyperv_ir_domain_ops, NULL);
 
-	irq_domain_free_fwnode(fn);
+	if (!ioapic_ir_domain) {
+		irq_domain_free_fwnode(fn);
+		return -ENOMEM;
+	}
 
 	/*
 	 * Hyper-V doesn't provide irq remapping function for
diff --git a/drivers/iommu/intel/irq_remapping.c b/drivers/iommu/intel/irq_remapping.c
index 7f8769800815..9564d23d094f 100644
--- a/drivers/iommu/intel/irq_remapping.c
+++ b/drivers/iommu/intel/irq_remapping.c
@@ -563,8 +563,8 @@ static int intel_setup_irq_remapping(struct intel_iommu *iommu)
 					    0, INTR_REMAP_TABLE_ENTRIES,
 					    fn, &intel_ir_domain_ops,
 					    iommu);
-	irq_domain_free_fwnode(fn);
 	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;
 	}
diff --git a/drivers/mfd/ioc3.c b/drivers/mfd/ioc3.c
index 02998d4eb74b..74cee7cb0afc 100644
--- a/drivers/mfd/ioc3.c
+++ b/drivers/mfd/ioc3.c
@@ -142,10 +142,11 @@ static int ioc3_irq_domain_setup(struct ioc3_priv_data *ipd, int irq)
 		goto err;
 
 	domain = irq_domain_create_linear(fn, 24, &ioc3_irq_domain_ops, ipd);
-	if (!domain)
+	if (!domain) {
+		irq_domain_free_fwnode(fn);
 		goto err;
+	}
 
-	irq_domain_free_fwnode(fn);
 	ipd->domain = domain;
 
 	irq_set_chained_handler_and_data(irq, ioc3_irq_handler, domain);
diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index e386d4eac407..9a64cf90c291 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -546,9 +546,10 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
 
 	vmd->irq_domain = pci_msi_create_irq_domain(fn, &vmd_msi_domain_info,
 						    x86_vector_domain);
-	irq_domain_free_fwnode(fn);
-	if (!vmd->irq_domain)
+	if (!vmd->irq_domain) {
+		irq_domain_free_fwnode(fn);
 		return -ENODEV;
+	}
 
 	pci_add_resource(&resources, &vmd->resources[0]);
 	pci_add_resource_offset(&resources, &vmd->resources[1], offset[0]);

Re: [PATCH] PCI: vmd: Keep fwnode allocated through VMD irqdomain life

[Bjorn Helgaas]

On Mon, Jul 06, 2020 at 03:30:23PM +0200, Thomas Gleixner wrote:
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> > On Mon, Jul 06, 2020 at 12:47:59PM +0200, Thomas Gleixner wrote:
> >> Bjorn Helgaas <helgaas@kernel.org> writes:
> >> > On Tue, Jun 30, 2020 at 12:39:08PM +0300, Andy Shevchenko wrote:
> >> >> The problem here is in the original patch which relies on the
> >> >> knowledge that fwnode is (was) not used anyhow specifically for ACPI
> >> >> case. That said, it makes fwnode a dangling pointer which I
> >> >> personally consider as a mine left for others. That's why the Fixes
> >> >> refers to the initial commit. The latter just has been blasted on
> >> >> that mine.
> >> 
> >> No. The original patch did not create a dangling pointer because fwnode
> >> was not stored for IRQCHIP_FWNODE_NAMED and IRQCHIP_FWNODE_NAMED_ID type
> >> nodes.
> >> 
> >> The fail was introduced in:
> >> 
> >> 711419e504eb ("irqdomain: Add the missing assignment of domain->fwnode for named fwnode")
> >
> > Ah, sorry for missing that and thank you for pointing out.
> 
> So something like the below wants to be applied and marked for stable

Great, thanks for all your help, Thomas!

This looks more like a general IRQ domain thing than a PCI thing, so
maybe it will make the most sense if Marc takes care of it.

> ---
> diff --git a/arch/mips/pci/pci-xtalk-bridge.c b/arch/mips/pci/pci-xtalk-bridge.c
> index 3b2552fb7735..5958217861b8 100644
> --- a/arch/mips/pci/pci-xtalk-bridge.c
> +++ b/arch/mips/pci/pci-xtalk-bridge.c
> @@ -627,9 +627,10 @@ static int bridge_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  	domain = irq_domain_create_hierarchy(parent, 0, 8, fn,
>  					     &bridge_domain_ops, NULL);
> -	irq_domain_free_fwnode(fn);
> -	if (!domain)
> +	if (!domain) {
> +		irq_domain_free_fwnode(fn);
>  		return -ENOMEM;
> +	}
>  
>  	pci_set_flags(PCI_PROBE_ONLY);
>  
> diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
> index ce61e3e7d399..81ffcfbfaef2 100644
> --- a/arch/x86/kernel/apic/io_apic.c
> +++ b/arch/x86/kernel/apic/io_apic.c
> @@ -2316,12 +2316,12 @@ static int mp_irqdomain_create(int ioapic)
>  	ip->irqdomain = irq_domain_create_linear(fn, hwirqs, cfg->ops,
>  						 (void *)(long)ioapic);
>  
> -	/* Release fw handle if it was allocated above */
> -	if (!cfg->dev)
> -		irq_domain_free_fwnode(fn);
> -
> -	if (!ip->irqdomain)
> +	if (!ip->irqdomain) {
> +		/* Release fw handle if it was allocated above */
> +		if (!cfg->dev)
> +			irq_domain_free_fwnode(fn);
>  		return -ENOMEM;
> +	}
>  
>  	ip->irqdomain->parent = parent;
>  
> diff --git a/arch/x86/kernel/apic/msi.c b/arch/x86/kernel/apic/msi.c
> index 5cbaca58af95..c2b2911feeef 100644
> --- a/arch/x86/kernel/apic/msi.c
> +++ b/arch/x86/kernel/apic/msi.c
> @@ -263,12 +263,13 @@ void __init arch_init_msi_domain(struct irq_domain *parent)
>  		msi_default_domain =
>  			pci_msi_create_irq_domain(fn, &pci_msi_domain_info,
>  						  parent);
> -		irq_domain_free_fwnode(fn);
>  	}
> -	if (!msi_default_domain)
> +	if (!msi_default_domain) {
> +		irq_domain_free_fwnode(fn);
>  		pr_warn("failed to initialize irqdomain for MSI/MSI-x.\n");
> -	else
> +	} else {
>  		msi_default_domain->flags |= IRQ_DOMAIN_MSI_NOMASK_QUIRK;
> +	}
>  }
>  
>  #ifdef CONFIG_IRQ_REMAP
> @@ -301,7 +302,8 @@ struct irq_domain *arch_create_remap_msi_irq_domain(struct irq_domain *parent,
>  	if (!fn)
>  		return NULL;
>  	d = pci_msi_create_irq_domain(fn, &pci_msi_ir_domain_info, parent);
> -	irq_domain_free_fwnode(fn);
> +	if (!d)
> +		irq_domain_free_fwnode(fn);
>  	return d;
>  }
>  #endif
> @@ -364,7 +366,8 @@ static struct irq_domain *dmar_get_irq_domain(void)
>  	if (fn) {
>  		dmar_domain = msi_create_irq_domain(fn, &dmar_msi_domain_info,
>  						    x86_vector_domain);
> -		irq_domain_free_fwnode(fn);
> +		if (!dmar_domain)
> +			irq_domain_free_fwnode(fn);
>  	}
>  out:
>  	mutex_unlock(&dmar_lock);
> @@ -489,7 +492,10 @@ struct irq_domain *hpet_create_irq_domain(int hpet_id)
>  	}
>  
>  	d = msi_create_irq_domain(fn, domain_info, parent);
> -	irq_domain_free_fwnode(fn);
> +	if (!d) {
> +		irq_domain_free_fwnode(fn);
> +		kfree(domain_info);
> +	}
>  	return d;
>  }
>  
> diff --git a/arch/x86/kernel/apic/vector.c b/arch/x86/kernel/apic/vector.c
> index c48be6e1f676..cc8b16f89dd4 100644
> --- a/arch/x86/kernel/apic/vector.c
> +++ b/arch/x86/kernel/apic/vector.c
> @@ -709,7 +709,6 @@ int __init arch_early_irq_init(void)
>  	x86_vector_domain = irq_domain_create_tree(fn, &x86_vector_domain_ops,
>  						   NULL);
>  	BUG_ON(x86_vector_domain == NULL);
> -	irq_domain_free_fwnode(fn);
>  	irq_set_default_host(x86_vector_domain);
>  
>  	arch_init_msi_domain(x86_vector_domain);
> diff --git a/arch/x86/platform/uv/uv_irq.c b/arch/x86/platform/uv/uv_irq.c
> index fc13cbbb2dce..abb6075397f0 100644
> --- a/arch/x86/platform/uv/uv_irq.c
> +++ b/arch/x86/platform/uv/uv_irq.c
> @@ -167,9 +167,10 @@ static struct irq_domain *uv_get_irq_domain(void)
>  		goto out;
>  
>  	uv_domain = irq_domain_create_tree(fn, &uv_domain_ops, NULL);
> -	irq_domain_free_fwnode(fn);
>  	if (uv_domain)
>  		uv_domain->parent = x86_vector_domain;
> +	else
> +		irq_domain_free_fwnode(fn);
>  out:
>  	mutex_unlock(&uv_lock);
>  
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index 74cca1757172..2f22326ee4df 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -3985,9 +3985,10 @@ int amd_iommu_create_irq_domain(struct amd_iommu *iommu)
>  	if (!fn)
>  		return -ENOMEM;
>  	iommu->ir_domain = irq_domain_create_tree(fn, &amd_ir_domain_ops, iommu);
> -	irq_domain_free_fwnode(fn);
> -	if (!iommu->ir_domain)
> +	if (!iommu->ir_domain) {
> +		irq_domain_free_fwnode(fn);
>  		return -ENOMEM;
> +	}
>  
>  	iommu->ir_domain->parent = arch_get_ir_parent_domain();
>  	iommu->msi_domain = arch_create_remap_msi_irq_domain(iommu->ir_domain,
> diff --git a/drivers/iommu/hyperv-iommu.c b/drivers/iommu/hyperv-iommu.c
> index 3c0c67a99c7b..8919c1c70b68 100644
> --- a/drivers/iommu/hyperv-iommu.c
> +++ b/drivers/iommu/hyperv-iommu.c
> @@ -155,7 +155,10 @@ static int __init hyperv_prepare_irq_remapping(void)
>  				0, IOAPIC_REMAPPING_ENTRY, fn,
>  				&hyperv_ir_domain_ops, NULL);
>  
> -	irq_domain_free_fwnode(fn);
> +	if (!ioapic_ir_domain) {
> +		irq_domain_free_fwnode(fn);
> +		return -ENOMEM;
> +	}
>  
>  	/*
>  	 * Hyper-V doesn't provide irq remapping function for
> diff --git a/drivers/iommu/intel/irq_remapping.c b/drivers/iommu/intel/irq_remapping.c
> index 7f8769800815..9564d23d094f 100644
> --- a/drivers/iommu/intel/irq_remapping.c
> +++ b/drivers/iommu/intel/irq_remapping.c
> @@ -563,8 +563,8 @@ static int intel_setup_irq_remapping(struct intel_iommu *iommu)
>  					    0, INTR_REMAP_TABLE_ENTRIES,
>  					    fn, &intel_ir_domain_ops,
>  					    iommu);
> -	irq_domain_free_fwnode(fn);
>  	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;
>  	}
> diff --git a/drivers/mfd/ioc3.c b/drivers/mfd/ioc3.c
> index 02998d4eb74b..74cee7cb0afc 100644
> --- a/drivers/mfd/ioc3.c
> +++ b/drivers/mfd/ioc3.c
> @@ -142,10 +142,11 @@ static int ioc3_irq_domain_setup(struct ioc3_priv_data *ipd, int irq)
>  		goto err;
>  
>  	domain = irq_domain_create_linear(fn, 24, &ioc3_irq_domain_ops, ipd);
> -	if (!domain)
> +	if (!domain) {
> +		irq_domain_free_fwnode(fn);
>  		goto err;
> +	}
>  
> -	irq_domain_free_fwnode(fn);
>  	ipd->domain = domain;
>  
>  	irq_set_chained_handler_and_data(irq, ioc3_irq_handler, domain);
> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index e386d4eac407..9a64cf90c291 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
> @@ -546,9 +546,10 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
>  
>  	vmd->irq_domain = pci_msi_create_irq_domain(fn, &vmd_msi_domain_info,
>  						    x86_vector_domain);
> -	irq_domain_free_fwnode(fn);
> -	if (!vmd->irq_domain)
> +	if (!vmd->irq_domain) {
> +		irq_domain_free_fwnode(fn);
>  		return -ENODEV;
> +	}
>  
>  	pci_add_resource(&resources, &vmd->resources[0]);
>  	pci_add_resource_offset(&resources, &vmd->resources[1], offset[0]);

[PATCH] irqdomain/treewide: Keep firmware node unconditionally allocated

[Thomas Gleixner]

Quite some non OF/ACPI users of irqdomains allocate firmware nodes of type
IRQCHIP_FWNODE_NAMED or IRQCHIP_FWNODE_NAMED_ID and free them right after
creating the irqdomain. The only purpose of these FW nodes is to convey
name information. When this was introduced the core code did not store the
pointer to the node in the irqdomain. A recent change stored the firmware
node pointer in irqdomain for other reasons and missed to notice that the
usage sites which do the alloc_fwnode/create_domain/free_fwnode sequence
are broken by this. Storing a dangling pointer is dangerous itself, but in
case that the domain is destroyed later on this leads to a double free.

Remove the freeing of the firmware node after creating the irqdomain from
all affected call sites to cure this.

Fixes: 711419e504eb ("irqdomain: Add the missing assignment of domain->fwnode for named fwnode")
Reported-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: stable@vger.kernel.org

---
 arch/mips/pci/pci-xtalk-bridge.c    |    5 +++--
 arch/x86/kernel/apic/io_apic.c      |   10 +++++-----
 arch/x86/kernel/apic/msi.c          |   18 ++++++++++++------
 arch/x86/kernel/apic/vector.c       |    1 -
 arch/x86/platform/uv/uv_irq.c       |    3 ++-
 drivers/iommu/amd/iommu.c           |    5 +++--
 drivers/iommu/hyperv-iommu.c        |    5 ++++-
 drivers/iommu/intel/irq_remapping.c |    2 +-
 drivers/mfd/ioc3.c                  |    5 +++--
 drivers/pci/controller/vmd.c        |    5 +++--
 10 files changed, 36 insertions(+), 23 deletions(-)

--- a/arch/mips/pci/pci-xtalk-bridge.c
+++ b/arch/mips/pci/pci-xtalk-bridge.c
@@ -627,9 +627,10 @@ static int bridge_probe(struct platform_
 		return -ENOMEM;
 	domain = irq_domain_create_hierarchy(parent, 0, 8, fn,
 					     &bridge_domain_ops, NULL);
-	irq_domain_free_fwnode(fn);
-	if (!domain)
+	if (!domain) {
+		irq_domain_free_fwnode(fn);
 		return -ENOMEM;
+	}
 
 	pci_set_flags(PCI_PROBE_ONLY);
 
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -2316,12 +2316,12 @@ static int mp_irqdomain_create(int ioapi
 	ip->irqdomain = irq_domain_create_linear(fn, hwirqs, cfg->ops,
 						 (void *)(long)ioapic);
 
-	/* Release fw handle if it was allocated above */
-	if (!cfg->dev)
-		irq_domain_free_fwnode(fn);
-
-	if (!ip->irqdomain)
+	if (!ip->irqdomain) {
+		/* Release fw handle if it was allocated above */
+		if (!cfg->dev)
+			irq_domain_free_fwnode(fn);
 		return -ENOMEM;
+	}
 
 	ip->irqdomain->parent = parent;
 
--- a/arch/x86/kernel/apic/msi.c
+++ b/arch/x86/kernel/apic/msi.c
@@ -263,12 +263,13 @@ void __init arch_init_msi_domain(struct
 		msi_default_domain =
 			pci_msi_create_irq_domain(fn, &pci_msi_domain_info,
 						  parent);
-		irq_domain_free_fwnode(fn);
 	}
-	if (!msi_default_domain)
+	if (!msi_default_domain) {
+		irq_domain_free_fwnode(fn);
 		pr_warn("failed to initialize irqdomain for MSI/MSI-x.\n");
-	else
+	} else {
 		msi_default_domain->flags |= IRQ_DOMAIN_MSI_NOMASK_QUIRK;
+	}
 }
 
 #ifdef CONFIG_IRQ_REMAP
@@ -301,7 +302,8 @@ struct irq_domain *arch_create_remap_msi
 	if (!fn)
 		return NULL;
 	d = pci_msi_create_irq_domain(fn, &pci_msi_ir_domain_info, parent);
-	irq_domain_free_fwnode(fn);
+	if (!d)
+		irq_domain_free_fwnode(fn);
 	return d;
 }
 #endif
@@ -364,7 +366,8 @@ static struct irq_domain *dmar_get_irq_d
 	if (fn) {
 		dmar_domain = msi_create_irq_domain(fn, &dmar_msi_domain_info,
 						    x86_vector_domain);
-		irq_domain_free_fwnode(fn);
+		if (!dmar_domain)
+			irq_domain_free_fwnode(fn);
 	}
 out:
 	mutex_unlock(&dmar_lock);
@@ -489,7 +492,10 @@ struct irq_domain *hpet_create_irq_domai
 	}
 
 	d = msi_create_irq_domain(fn, domain_info, parent);
-	irq_domain_free_fwnode(fn);
+	if (!d) {
+		irq_domain_free_fwnode(fn);
+		kfree(domain_info);
+	}
 	return d;
 }
 
--- a/arch/x86/kernel/apic/vector.c
+++ b/arch/x86/kernel/apic/vector.c
@@ -709,7 +709,6 @@ int __init arch_early_irq_init(void)
 	x86_vector_domain = irq_domain_create_tree(fn, &x86_vector_domain_ops,
 						   NULL);
 	BUG_ON(x86_vector_domain == NULL);
-	irq_domain_free_fwnode(fn);
 	irq_set_default_host(x86_vector_domain);
 
 	arch_init_msi_domain(x86_vector_domain);
--- a/arch/x86/platform/uv/uv_irq.c
+++ b/arch/x86/platform/uv/uv_irq.c
@@ -167,9 +167,10 @@ static struct irq_domain *uv_get_irq_dom
 		goto out;
 
 	uv_domain = irq_domain_create_tree(fn, &uv_domain_ops, NULL);
-	irq_domain_free_fwnode(fn);
 	if (uv_domain)
 		uv_domain->parent = x86_vector_domain;
+	else
+		irq_domain_free_fwnode(fn);
 out:
 	mutex_unlock(&uv_lock);
 
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -3985,9 +3985,10 @@ int amd_iommu_create_irq_domain(struct a
 	if (!fn)
 		return -ENOMEM;
 	iommu->ir_domain = irq_domain_create_tree(fn, &amd_ir_domain_ops, iommu);
-	irq_domain_free_fwnode(fn);
-	if (!iommu->ir_domain)
+	if (!iommu->ir_domain) {
+		irq_domain_free_fwnode(fn);
 		return -ENOMEM;
+	}
 
 	iommu->ir_domain->parent = arch_get_ir_parent_domain();
 	iommu->msi_domain = arch_create_remap_msi_irq_domain(iommu->ir_domain,
--- a/drivers/iommu/hyperv-iommu.c
+++ b/drivers/iommu/hyperv-iommu.c
@@ -155,7 +155,10 @@ static int __init hyperv_prepare_irq_rem
 				0, IOAPIC_REMAPPING_ENTRY, fn,
 				&hyperv_ir_domain_ops, NULL);
 
-	irq_domain_free_fwnode(fn);
+	if (!ioapic_ir_domain) {
+		irq_domain_free_fwnode(fn);
+		return -ENOMEM;
+	}
 
 	/*
 	 * Hyper-V doesn't provide irq remapping function for
--- a/drivers/iommu/intel/irq_remapping.c
+++ b/drivers/iommu/intel/irq_remapping.c
@@ -563,8 +563,8 @@ static int intel_setup_irq_remapping(str
 					    0, INTR_REMAP_TABLE_ENTRIES,
 					    fn, &intel_ir_domain_ops,
 					    iommu);
-	irq_domain_free_fwnode(fn);
 	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;
 	}
--- a/drivers/mfd/ioc3.c
+++ b/drivers/mfd/ioc3.c
@@ -142,10 +142,11 @@ static int ioc3_irq_domain_setup(struct
 		goto err;
 
 	domain = irq_domain_create_linear(fn, 24, &ioc3_irq_domain_ops, ipd);
-	if (!domain)
+	if (!domain) {
+		irq_domain_free_fwnode(fn);
 		goto err;
+	}
 
-	irq_domain_free_fwnode(fn);
 	ipd->domain = domain;
 
 	irq_set_chained_handler_and_data(irq, ioc3_irq_handler, domain);
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -546,9 +546,10 @@ static int vmd_enable_domain(struct vmd_
 
 	vmd->irq_domain = pci_msi_create_irq_domain(fn, &vmd_msi_domain_info,
 						    x86_vector_domain);
-	irq_domain_free_fwnode(fn);
-	if (!vmd->irq_domain)
+	if (!vmd->irq_domain) {
+		irq_domain_free_fwnode(fn);
 		return -ENODEV;
+	}
 
 	pci_add_resource(&resources, &vmd->resources[0]);
 	pci_add_resource_offset(&resources, &vmd->resources[1], offset[0]);

Re: [PATCH] irqdomain/treewide: Keep firmware node unconditionally allocated

[Marc Zyngier]

Hi Thomas,

Catching up on email...

On 2020-07-09 10:53, Thomas Gleixner wrote:
> Quite some non OF/ACPI users of irqdomains allocate firmware nodes of 
> type
> IRQCHIP_FWNODE_NAMED or IRQCHIP_FWNODE_NAMED_ID and free them right 
> after
> creating the irqdomain. The only purpose of these FW nodes is to convey
> name information. When this was introduced the core code did not store 
> the
> pointer to the node in the irqdomain. A recent change stored the 
> firmware
> node pointer in irqdomain for other reasons and missed to notice that 
> the
> usage sites which do the alloc_fwnode/create_domain/free_fwnode 
> sequence
> are broken by this. Storing a dangling pointer is dangerous itself, but 
> in
> case that the domain is destroyed later on this leads to a double free.
> 
> Remove the freeing of the firmware node after creating the irqdomain 
> from
> all affected call sites to cure this.
> 
> Fixes: 711419e504eb ("irqdomain: Add the missing assignment of
> domain->fwnode for named fwnode")
> Reported-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: stable@vger.kernel.org

Urgh, that's pretty disastrous. My bad. Thanks a lot for having
put this patch together.

Acked-by: Marc Zyngier <maz@kernel.org>

If you can take it directly into Linus' tree, that'd be greatly
appreciated.

Thanks again,

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] irqdomain/treewide: Keep firmware node unconditionally allocated

[Bjorn Helgaas]

On Thu, Jul 09, 2020 at 11:53:06AM +0200, Thomas Gleixner wrote:
> Quite some non OF/ACPI users of irqdomains allocate firmware nodes of type
> IRQCHIP_FWNODE_NAMED or IRQCHIP_FWNODE_NAMED_ID and free them right after
> creating the irqdomain. The only purpose of these FW nodes is to convey
> name information. When this was introduced the core code did not store the
> pointer to the node in the irqdomain. A recent change stored the firmware
> node pointer in irqdomain for other reasons and missed to notice that the
> usage sites which do the alloc_fwnode/create_domain/free_fwnode sequence
> are broken by this. Storing a dangling pointer is dangerous itself, but in
> case that the domain is destroyed later on this leads to a double free.
> 
> Remove the freeing of the firmware node after creating the irqdomain from
> all affected call sites to cure this.
> 
> Fixes: 711419e504eb ("irqdomain: Add the missing assignment of domain->fwnode for named fwnode")
> Reported-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: stable@vger.kernel.org

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