Re: [PATCH] PCI: take the rescan lock when adding devices during host probe

[Bartosz Golaszewski]

On Tue, Oct 1, 2024 at 11:11 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
>
> On Thu, Sep 26, 2024 at 03:09:23PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> >
> > Since adding the PCI power control code, we may end up with a race
> > between the pwrctl platform device rescanning the bus and the host
> > controller probe function. The latter needs to take the rescan lock when
> > adding devices or may crash.
> >
> > Reported-by: Konrad Dybcio <konradybcio@xxxxxxxxxx>
> > Fixes: 4565d2652a37 ("PCI/pwrctl: Add PCI power control core code")
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> > ---
> >  drivers/pci/probe.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index 4f68414c3086..f1615805f5b0 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -3105,7 +3105,9 @@ int pci_host_probe(struct pci_host_bridge *bridge)
> >       list_for_each_entry(child, &bus->children, node)
> >               pcie_bus_configure_settings(child);
> >
> > +     pci_lock_rescan_remove();
> >       pci_bus_add_devices(bus);
> > +     pci_unlock_rescan_remove();
>
> Seems like we do need locking here, but don't we need a more
> comprehensive change?  There are many other callers of
> pci_bus_add_devices(), and most of them look similarly unprotected.
>

>From a quick glance it looks like the majority of users are specific
drivers (controller, hotplug, etc.). The calls inside pci_rescan_bus()
and pci_rescan_bus_bridge_resize() are already protected from what I
can tell. I'm not saying that the driver calls shouldn't be fixed but
there's no immediate danger. This however fixes an issue we hit with
PCI core so I'd send it upstream now and then we can think about the
other use-cases.

Bart