| Message ID | 1346168638-32724-6-git-send-email-jiang.liu@huawei.com |
|---|---|
| State | Accepted |
| Headers | show |
On Tue, Aug 28, 2012 at 11:43:58PM +0800, Jiang Liu wrote: > Following code has a race window between pci_find_bus() and pci_get_slot() > if PCI hotplug operation happens between them which removes the pci_bus. > So use PCI hotplug safe interface pci_get_domain_bus_and_slot() instead, > which also reduces code complexity. Has this happend in practice? Is this something one can reproduce by manipulating SysFS and at the same time unplugging the PCI devices? > > struct pci_bus *pci_bus = pci_find_bus(domain, busno); > struct pci_dev *pci_dev = pci_get_slot(pci_bus, devfn); > > Signed-off-by: Jiang Liu <jiang.liu@huawei.com> > --- > drivers/pci/xen-pcifront.c | 10 ++-------- > 1 file changed, 2 insertions(+), 8 deletions(-) > > diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c > index d6cc62c..def8d0b 100644 > --- a/drivers/pci/xen-pcifront.c > +++ b/drivers/pci/xen-pcifront.c > @@ -982,7 +982,6 @@ static int pcifront_detach_devices(struct pcifront_device *pdev) > int err = 0; > int i, num_devs; > unsigned int domain, bus, slot, func; > - struct pci_bus *pci_bus; > struct pci_dev *pci_dev; > char str[64]; > > @@ -1032,13 +1031,8 @@ static int pcifront_detach_devices(struct pcifront_device *pdev) > goto out; > } > > - pci_bus = pci_find_bus(domain, bus); > - if (!pci_bus) { > - dev_dbg(&pdev->xdev->dev, "Cannot get bus %04x:%02x\n", > - domain, bus); > - continue; > - } > - pci_dev = pci_get_slot(pci_bus, PCI_DEVFN(slot, func)); > + pci_dev = pci_get_domain_bus_and_slot(domain, bus, > + PCI_DEVFN(slot, func)); > if (!pci_dev) { > dev_dbg(&pdev->xdev->dev, > "Cannot get PCI device %04x:%02x:%02x.%d\n", > -- > 1.7.9.5 > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/29/2012 12:59 AM, Konrad Rzeszutek Wilk wrote: > On Tue, Aug 28, 2012 at 11:43:58PM +0800, Jiang Liu wrote: >> Following code has a race window between pci_find_bus() and pci_get_slot() >> if PCI hotplug operation happens between them which removes the pci_bus. >> So use PCI hotplug safe interface pci_get_domain_bus_and_slot() instead, >> which also reduces code complexity. > > Has this happend in practice? Is this something one can reproduce > by manipulating SysFS and at the same time unplugging the PCI > devices? Hi Konrad, We just noticed this issue by code inspection when improving PCI hotplug implementation. I guess we could trigger such issue by adding some delay between pci_find_bus() and pci_get_slot(). Regards! Gerry >> >> struct pci_bus *pci_bus = pci_find_bus(domain, busno); >> struct pci_dev *pci_dev = pci_get_slot(pci_bus, devfn); >> >> Signed-off-by: Jiang Liu <jiang.liu@huawei.com> >> --- >> drivers/pci/xen-pcifront.c | 10 ++-------- >> 1 file changed, 2 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c >> index d6cc62c..def8d0b 100644 >> --- a/drivers/pci/xen-pcifront.c >> +++ b/drivers/pci/xen-pcifront.c >> @@ -982,7 +982,6 @@ static int pcifront_detach_devices(struct pcifront_device *pdev) >> int err = 0; >> int i, num_devs; >> unsigned int domain, bus, slot, func; >> - struct pci_bus *pci_bus; >> struct pci_dev *pci_dev; >> char str[64]; >> >> @@ -1032,13 +1031,8 @@ static int pcifront_detach_devices(struct pcifront_device *pdev) >> goto out; >> } >> >> - pci_bus = pci_find_bus(domain, bus); >> - if (!pci_bus) { >> - dev_dbg(&pdev->xdev->dev, "Cannot get bus %04x:%02x\n", >> - domain, bus); >> - continue; >> - } >> - pci_dev = pci_get_slot(pci_bus, PCI_DEVFN(slot, func)); >> + pci_dev = pci_get_domain_bus_and_slot(domain, bus, >> + PCI_DEVFN(slot, func)); >> if (!pci_dev) { >> dev_dbg(&pdev->xdev->dev, >> "Cannot get PCI device %04x:%02x:%02x.%d\n", >> -- >> 1.7.9.5 >> -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 29, 2012 at 07:56:43AM +0800, Jiang Liu wrote: > On 08/29/2012 12:59 AM, Konrad Rzeszutek Wilk wrote: > > On Tue, Aug 28, 2012 at 11:43:58PM +0800, Jiang Liu wrote: > >> Following code has a race window between pci_find_bus() and pci_get_slot() > >> if PCI hotplug operation happens between them which removes the pci_bus. > >> So use PCI hotplug safe interface pci_get_domain_bus_and_slot() instead, > >> which also reduces code complexity. > > > > Has this happend in practice? Is this something one can reproduce > > by manipulating SysFS and at the same time unplugging the PCI > > devices? > Hi Konrad, > We just noticed this issue by code inspection when improving PCI > hotplug implementation. I guess we could trigger such issue by adding > some delay between pci_find_bus() and pci_get_slot(). > Regards! OK. Let me test it out. > Gerry > > >> > >> struct pci_bus *pci_bus = pci_find_bus(domain, busno); > >> struct pci_dev *pci_dev = pci_get_slot(pci_bus, devfn); > >> > >> Signed-off-by: Jiang Liu <jiang.liu@huawei.com> > >> --- > >> drivers/pci/xen-pcifront.c | 10 ++-------- > >> 1 file changed, 2 insertions(+), 8 deletions(-) > >> > >> diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c > >> index d6cc62c..def8d0b 100644 > >> --- a/drivers/pci/xen-pcifront.c > >> +++ b/drivers/pci/xen-pcifront.c > >> @@ -982,7 +982,6 @@ static int pcifront_detach_devices(struct pcifront_device *pdev) > >> int err = 0; > >> int i, num_devs; > >> unsigned int domain, bus, slot, func; > >> - struct pci_bus *pci_bus; > >> struct pci_dev *pci_dev; > >> char str[64]; > >> > >> @@ -1032,13 +1031,8 @@ static int pcifront_detach_devices(struct pcifront_device *pdev) > >> goto out; > >> } > >> > >> - pci_bus = pci_find_bus(domain, bus); > >> - if (!pci_bus) { > >> - dev_dbg(&pdev->xdev->dev, "Cannot get bus %04x:%02x\n", > >> - domain, bus); > >> - continue; > >> - } > >> - pci_dev = pci_get_slot(pci_bus, PCI_DEVFN(slot, func)); > >> + pci_dev = pci_get_domain_bus_and_slot(domain, bus, > >> + PCI_DEVFN(slot, func)); > >> if (!pci_dev) { > >> dev_dbg(&pdev->xdev->dev, > >> "Cannot get PCI device %04x:%02x:%02x.%d\n", > >> -- > >> 1.7.9.5 > >> -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Aug 28, 2012 at 11:43:58PM +0800, Jiang Liu wrote: > Following code has a race window between pci_find_bus() and pci_get_slot() > if PCI hotplug operation happens between them which removes the pci_bus. > So use PCI hotplug safe interface pci_get_domain_bus_and_slot() instead, > which also reduces code complexity. > > struct pci_bus *pci_bus = pci_find_bus(domain, busno); > struct pci_dev *pci_dev = pci_get_slot(pci_bus, devfn); > > Signed-off-by: Jiang Liu <jiang.liu@huawei.com> So: Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > --- > drivers/pci/xen-pcifront.c | 10 ++-------- > 1 file changed, 2 insertions(+), 8 deletions(-) > > diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c > index d6cc62c..def8d0b 100644 > --- a/drivers/pci/xen-pcifront.c > +++ b/drivers/pci/xen-pcifront.c > @@ -982,7 +982,6 @@ static int pcifront_detach_devices(struct pcifront_device *pdev) > int err = 0; > int i, num_devs; > unsigned int domain, bus, slot, func; > - struct pci_bus *pci_bus; > struct pci_dev *pci_dev; > char str[64]; > > @@ -1032,13 +1031,8 @@ static int pcifront_detach_devices(struct pcifront_device *pdev) > goto out; > } > > - pci_bus = pci_find_bus(domain, bus); > - if (!pci_bus) { > - dev_dbg(&pdev->xdev->dev, "Cannot get bus %04x:%02x\n", > - domain, bus); > - continue; > - } > - pci_dev = pci_get_slot(pci_bus, PCI_DEVFN(slot, func)); > + pci_dev = pci_get_domain_bus_and_slot(domain, bus, > + PCI_DEVFN(slot, func)); > if (!pci_dev) { > dev_dbg(&pdev->xdev->dev, > "Cannot get PCI device %04x:%02x:%02x.%d\n", > -- > 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c index d6cc62c..def8d0b 100644 --- a/drivers/pci/xen-pcifront.c +++ b/drivers/pci/xen-pcifront.c @@ -982,7 +982,6 @@ static int pcifront_detach_devices(struct pcifront_device *pdev) int err = 0; int i, num_devs; unsigned int domain, bus, slot, func; - struct pci_bus *pci_bus; struct pci_dev *pci_dev; char str[64]; @@ -1032,13 +1031,8 @@ static int pcifront_detach_devices(struct pcifront_device *pdev) goto out; } - pci_bus = pci_find_bus(domain, bus); - if (!pci_bus) { - dev_dbg(&pdev->xdev->dev, "Cannot get bus %04x:%02x\n", - domain, bus); - continue; - } - pci_dev = pci_get_slot(pci_bus, PCI_DEVFN(slot, func)); + pci_dev = pci_get_domain_bus_and_slot(domain, bus, + PCI_DEVFN(slot, func)); if (!pci_dev) { dev_dbg(&pdev->xdev->dev, "Cannot get PCI device %04x:%02x:%02x.%d\n",
Following code has a race window between pci_find_bus() and pci_get_slot() if PCI hotplug operation happens between them which removes the pci_bus. So use PCI hotplug safe interface pci_get_domain_bus_and_slot() instead, which also reduces code complexity. struct pci_bus *pci_bus = pci_find_bus(domain, busno); struct pci_dev *pci_dev = pci_get_slot(pci_bus, devfn); Signed-off-by: Jiang Liu <jiang.liu@huawei.com> --- drivers/pci/xen-pcifront.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)