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

[Bjorn Helgaas <helgaas@kernel.org>]

On Wed, Oct 02, 2024 at 10:31:46PM +0200, Konrad Dybcio wrote:
> On 2.10.2024 10:36 AM, Bartosz Golaszewski wrote:
> > On Tue, Oct 1, 2024 at 11:11 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> >>
> >> On Thu, Sep 26, 2024 at 03:09:23PM +0200, Bartosz Golaszewski wrote:
> >>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >>>
> >>> 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@kernel.org>
> >>> Fixes: 4565d2652a37 ("PCI/pwrctl: Add PCI power control core code")
> >>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >>> ---
> >>>  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.

Agreed that all current callers of pci_rescan_bus() and
pci_rescan_bus_bridge_resize() already do their own locking.  Most of
the hotplug drivers that use pci_bus_add_devices() do their own
locking as well.

pci_host_probe() is used by many controller drivers, but my guess is
that a dozen or so controller drivers that use pci_bus_add_devices()
directly without locking are still at risk.  It's sort of an
unfinished project to convert drivers like this over to using
pci_host_probe().

In the meantime, I wish we had a safer interface that could enforce
the locking internally.

> Probably worth showing an example of how this can manifest:
> 
> removed a device through sysfs and called bus rescan:

Thanks for this; I was about to ask for it!  I don't think we need
*all* the details, but something like the following might help people
recognize if we trip over another instance:

> Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
> Internal error: Oops: 0000000096000004 [#1] SMP
> Call trace:
>  __pi_strlen+0x14/0x150
>  kernfs_find_ns+0x80/0x13c
>  kernfs_remove_by_name_ns+0x54/0xf0
>  sysfs_remove_bin_file+0x24/0x34
>  pci_remove_resource_files+0x3c/0x84
>  pci_remove_sysfs_dev_files+0x28/0x38
>  pci_stop_bus_device+0x8c/0xd8
>  pci_stop_bus_device+0x40/0xd8
>  pci_stop_and_remove_bus_device_locked+0x28/0x48
>  remove_store+0x70/0xb0
>  dev_attr_store+0x20/0x38
>  sysfs_kf_write+0x58/0x78
>  kernfs_fop_write_iter+0xe8/0x184
>  vfs_write+0x2dc/0x308

Bjorn