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@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.
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
