Message ID | 20210211182435.47968-1-tyreld@linux.ibm.com (mailing list archive) |
---|---|
State | Accepted |
Headers | show |
Series | powerpc/pseries: extract host bridge from pci_bus prior to bus removal | expand |
Context | Check | Description |
---|---|---|
snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (626a6c3d2e20da80aaa710104f34ea6037b28b33) |
snowpatch_ozlabs/build-ppc64le | success | Build succeeded |
snowpatch_ozlabs/build-ppc64be | success | Build succeeded |
snowpatch_ozlabs/build-ppc64e | success | Build succeeded |
snowpatch_ozlabs/build-pmac32 | success | Build succeeded |
snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 16 lines checked |
snowpatch_ozlabs/needsstable | warning | Please consider tagging this patch for stable! |
On 2/11/21 10:24 AM, Tyrel Datwyler wrote: > The pci_bus->bridge reference may no longer be valid after > pci_bus_remove() resulting in passing a bad value to device_unregister() > for the associated bridge device. > > Store the host_bridge reference in a separate variable prior to > pci_bus_remove(). > > Fixes: 7340056567e3 ("powerpc/pci: Reorder pci bus/bridge unregistration during PHB removal") > Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com> Ping?
Hi Tyrel, > The pci_bus->bridge reference may no longer be valid after > pci_bus_remove() resulting in passing a bad value to device_unregister() > for the associated bridge device. > > Store the host_bridge reference in a separate variable prior to > pci_bus_remove(). > The patch certainly seems to do what you say. I'm not really up on the innards of PCI, so I'm struggling to figure out by what code path pci_bus_remove() might invalidate pci_bus->bridge? A quick look at pci_remove_bus was not very illuminating but I didn't chase down every call it made. Kind regards, Daniel > Fixes: 7340056567e3 ("powerpc/pci: Reorder pci bus/bridge unregistration during PHB removal") > Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com> > --- > arch/powerpc/platforms/pseries/pci_dlpar.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/arch/powerpc/platforms/pseries/pci_dlpar.c b/arch/powerpc/platforms/pseries/pci_dlpar.c > index f9ae17e8a0f4..a8f9140a24fa 100644 > --- a/arch/powerpc/platforms/pseries/pci_dlpar.c > +++ b/arch/powerpc/platforms/pseries/pci_dlpar.c > @@ -50,6 +50,7 @@ EXPORT_SYMBOL_GPL(init_phb_dynamic); > int remove_phb_dynamic(struct pci_controller *phb) > { > struct pci_bus *b = phb->bus; > + struct pci_host_bridge *host_bridge = to_pci_host_bridge(b->bridge); > struct resource *res; > int rc, i; > > @@ -76,7 +77,8 @@ int remove_phb_dynamic(struct pci_controller *phb) > /* Remove the PCI bus and unregister the bridge device from sysfs */ > phb->bus = NULL; > pci_remove_bus(b); > - device_unregister(b->bridge); > + host_bridge->bus = NULL; > + device_unregister(&host_bridge->dev); > > /* Now release the IO resource */ > if (res->flags & IORESOURCE_IO) > -- > 2.27.0
On 4/16/21 12:15 AM, Daniel Axtens wrote: > Hi Tyrel, > >> The pci_bus->bridge reference may no longer be valid after >> pci_bus_remove() resulting in passing a bad value to device_unregister() >> for the associated bridge device. >> >> Store the host_bridge reference in a separate variable prior to >> pci_bus_remove(). >> > The patch certainly seems to do what you say. I'm not really up on the > innards of PCI, so I'm struggling to figure out by what code path > pci_bus_remove() might invalidate pci_bus->bridge? A quick look at > pci_remove_bus was not very illuminating but I didn't chase down every > call it made. remove_phb_dynamic() |--> pci_remove_bus(bus) |--> device_unregister(&bus->dev) |--> put_device(dev) |--> device_release(kobj) |--> dev->class->dev_release(dev) == release_pci_bus(dev) |--> kfree(bus) We have the above call chain that takes place in the when put_device() triggers the kobject ref count to go zero. The kobject_release function in this case is device_release() which in turn calls dev->class->dev_release(dev). For a pci_bus the class is appropriately pcibus_class whose dev_release() callback points to release_pci_bus(). This in turn calls kfree() on the bus. Which means we can no longer safely dereference any fields of the pci_bus struct. -Tyrel > > Kind regards, > Daniel > >> Fixes: 7340056567e3 ("powerpc/pci: Reorder pci bus/bridge unregistration during PHB removal") >> Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com> >> --- >> arch/powerpc/platforms/pseries/pci_dlpar.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/arch/powerpc/platforms/pseries/pci_dlpar.c b/arch/powerpc/platforms/pseries/pci_dlpar.c >> index f9ae17e8a0f4..a8f9140a24fa 100644 >> --- a/arch/powerpc/platforms/pseries/pci_dlpar.c >> +++ b/arch/powerpc/platforms/pseries/pci_dlpar.c >> @@ -50,6 +50,7 @@ EXPORT_SYMBOL_GPL(init_phb_dynamic); >> int remove_phb_dynamic(struct pci_controller *phb) >> { >> struct pci_bus *b = phb->bus; >> + struct pci_host_bridge *host_bridge = to_pci_host_bridge(b->bridge); >> struct resource *res; >> int rc, i; >> >> @@ -76,7 +77,8 @@ int remove_phb_dynamic(struct pci_controller *phb) >> /* Remove the PCI bus and unregister the bridge device from sysfs */ >> phb->bus = NULL; >> pci_remove_bus(b); >> - device_unregister(b->bridge); >> + host_bridge->bus = NULL; >> + device_unregister(&host_bridge->dev); >> >> /* Now release the IO resource */ >> if (res->flags & IORESOURCE_IO) >> -- >> 2.27.0
On Thu, 11 Feb 2021 12:24:35 -0600, Tyrel Datwyler wrote: > The pci_bus->bridge reference may no longer be valid after > pci_bus_remove() resulting in passing a bad value to device_unregister() > for the associated bridge device. > > Store the host_bridge reference in a separate variable prior to > pci_bus_remove(). Applied to powerpc/next. [1/1] powerpc/pseries: extract host bridge from pci_bus prior to bus removal https://git.kernel.org/powerpc/c/38d0b1c9cec71e6d0f3bddef0bbce41d05a3e796 cheers
diff --git a/arch/powerpc/platforms/pseries/pci_dlpar.c b/arch/powerpc/platforms/pseries/pci_dlpar.c index f9ae17e8a0f4..a8f9140a24fa 100644 --- a/arch/powerpc/platforms/pseries/pci_dlpar.c +++ b/arch/powerpc/platforms/pseries/pci_dlpar.c @@ -50,6 +50,7 @@ EXPORT_SYMBOL_GPL(init_phb_dynamic); int remove_phb_dynamic(struct pci_controller *phb) { struct pci_bus *b = phb->bus; + struct pci_host_bridge *host_bridge = to_pci_host_bridge(b->bridge); struct resource *res; int rc, i; @@ -76,7 +77,8 @@ int remove_phb_dynamic(struct pci_controller *phb) /* Remove the PCI bus and unregister the bridge device from sysfs */ phb->bus = NULL; pci_remove_bus(b); - device_unregister(b->bridge); + host_bridge->bus = NULL; + device_unregister(&host_bridge->dev); /* Now release the IO resource */ if (res->flags & IORESOURCE_IO)
The pci_bus->bridge reference may no longer be valid after pci_bus_remove() resulting in passing a bad value to device_unregister() for the associated bridge device. Store the host_bridge reference in a separate variable prior to pci_bus_remove(). Fixes: 7340056567e3 ("powerpc/pci: Reorder pci bus/bridge unregistration during PHB removal") Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com> --- arch/powerpc/platforms/pseries/pci_dlpar.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)