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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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