RE: [PATCH 2/2] pci-hyperv: properly handle device eject

[Long Li]

> -----Original Message-----
> From: devel [mailto:driverdev-devel-bounces@xxxxxxxxxxxxxxxxxxxxxx] On
> Behalf Of Long Li
> Sent: Tuesday, September 13, 2016 10:33 AM
> To: Dexuan Cui <decui@xxxxxxxxxxxxx>; KY Srinivasan
> <kys@xxxxxxxxxxxxx>; Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>; Bjorn
> Helgaas <bhelgaas@xxxxxxxxxx>
> Cc: devel@xxxxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-
> pci@xxxxxxxxxxxxxxx
> Subject: RE: [PATCH 2/2] pci-hyperv: properly handle device eject
> 
> This sender failed our fraud detection checks and may not be who they
> appear to be. Learn about spoofing at http://aka.ms/LearnAboutSpoofing
> 
> > -----Original Message-----
> > From: Dexuan Cui
> > Sent: Tuesday, September 13, 2016 2:51 AM
> > To: Long Li <longli@xxxxxxxxxxxxx>; KY Srinivasan <kys@xxxxxxxxxxxxx>;
> > Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>; Bjorn Helgaas
> > <bhelgaas@xxxxxxxxxx>
> > Cc: devel@xxxxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-
> > pci@xxxxxxxxxxxxxxx
> > Subject: RE: [PATCH 2/2] pci-hyperv: properly handle device eject
> >
> > > From: devel [mailto:driverdev-devel-bounces@xxxxxxxxxxxxxxxxxxxxxx]
> > > On Behalf Of Long Li
> > > Sent: Tuesday, September 13, 2016 7:54 ...
> > > A PCI_EJECT message can arrive at the same time we are calling
> > > pci_scan_child_bus in the workqueue for the previous
> > PCI_BUS_RELATIONS
> > > message, in this case we could potentailly modify the bus from two
> places.
> > > Properly lock the bus access.
> > >
> > > --- a/drivers/pci/host/pci-hyperv.c
> > > +++ b/drivers/pci/host/pci-hyperv.c
> > > @@ -1587,7 +1587,7 @@ static void hv_eject_device_work(struct
> > > work_struct
> > > *work)
> > >         pdev =
> > > pci_get_domain_bus_and_slot(hpdev->hbus->sysdata.domain,
> > 0,
> > >                                            wslot);
> > >         if (pdev) {
> > > -               pci_stop_and_remove_bus_device(pdev);
> > > +               pci_stop_and_remove_bus_device_locked(pdev);
> > >                 pci_dev_put(pdev);
> > >         }
> >
> > The _locked version tries to get the mutex pci_rescan_remove_lock.
> >
> > But it looks pci_scan_child_bus() doesn't try to get the mutex(?), so
> > how can this patch make sure the 2 code paths are not running
> simultaneously?
> 
> Thanks for the review.
> 
> The lock is to protect the following call to pci_scan_child_bus() in
> pci_devices_present_work():
> 
>                 /*
>                  * Tell the core to rescan bus
>                  * because there may have been changes.
>                  */
>                 pci_lock_rescan_remove();
>                 pci_scan_child_bus(hbus->pci_bus);
>                 pci_unlock_rescan_remove();
> 
> This race condition has shown up in the tests.
> 
> You raised a valid concern in create_root_hv_pci_bus(). There might be
> another race condition there. I'll look into this.

I think this code is safe here. If we reach the code pci_stop_and_remove_bus_device_locked, create_root_hv_pci_bus() is already called.

> 
> >
> > Thanks,
> > -- Dexuan
> _______________________________________________
> devel mailing list
> devel@xxxxxxxxxxxxxxxxxxxxxx
> https://na01.safelinks.protection.outlook.com/?url=http%3a%2f%2fdriverde
> v.linuxdriverproject.org%2fmailman%2flistinfo%2fdriverdev-
> devel&data=02%7c01%7clongli%40microsoft.com%7c3d12ee6d87c140eb5114
> 08d3dbfc1713%7c72f988bf86f141af91ab2d7cd011db47%7c1%7c0%7c6360938
> 48185348266&sdata=a2GYqIBsQAFxszkKg3fl1nqqPgvZHh%2bAY2255RgrvUU
> %3d