| Message ID | 20200625162450.5419-1-jonathan.derrick@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: vmd: Keep fwnode allocated through VMD irqdomain life | expand |
[+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 >
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 > >
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 > >
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 > > >
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 > >
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 > > > >
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 > > > > > >
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
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.
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]);
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]);
"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
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
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