linux-kernel - Re: [PATCH v2 1/1] Guard pci_create_sysfs_dev

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3607385.usQuhbGJ8B@steina-w>
Date:   Thu, 16 Mar 2023 14:16:13 +0100
From:   Alexander Stein <alexander.stein@...tq-group.com>
To:     Bjorn Helgaas <bhelgaas@...gle.com>,
        Oliver Neukum <oneukum@...e.com>,
        Oliver Neukum <oneukum@...e.com>
Cc:     Korneliusz Osmenda <korneliuszo@...il.com>,
        linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/1] Guard pci_create_sysfs_dev_files with atomic value

Hi Oliver,

Am Donnerstag, 16. März 2023, 13:23:25 CET schrieb Oliver Neukum:
> On 16.03.23 12:58, Alexander Stein wrote:
> > Hi Oliver,
> > 
> > Am Donnerstag, 16. März 2023, 12:17:32 CET schrieb Oliver Neukum:
> >> It seems to me that you must not add a bridge before
> >> pci_create_sysfs_dev_files() has finished. Now you could add a wait_queue
> >> and a flag and wait for it to finish. But that is not very elegant.
> > 
> > Do we need the pci_sysfs_init initcall at all? Or to put it in other
> > words,
> > what does this initcall solve?
> 
> Fundamentally something has to discover the root bridge.
> Secondly your system has to boot. The device right behind
> the root bridge will already be up and running when the kernel
> takes control. IMHO treating such devices differently from
> other devices makes sense.

But isn't the root bridge discovered by the driver (pci-imx6 in this case) for 
that? And the driver probe path eventually calls into the sysfs file creation.
I compared the file creation to usb, as this is a discoverable bus as well. 
There is no special initialization regarding sysfs.

> > See my different approach eliminating this race at all.
> 
> Please elaborate

Currently the initcall pci_sysfs_init and the PCIe root bridge driver probe
paths are competing for file creation.
If, for some reason, the device enumeration for PCI bus during imx6_pcie_probe 
is delayed after pci_sysfs_init initcall, this initcall essentially does 
nothing, no devices or busses to iterate. Which means the complete pcie sysfs 
creation is done from bridge probe path. There is no reason to iterate over 
discovered PCIe devices/busses separately.

I assume this issue is not that prominent, if at all, as other platforms vary 
in speed a lot. I was not able to reproduce on i.MX8MP which uses the same 
PCIe bridge driver. Due to improved speed performance, I guess on this 
platform pci_sysfs_init finishes, without doing anything, before PCIe bridge 
is probed.

I might be missing something (ACPI systems, etc.), I do not know the details 
within pci subsystem, but from my point of view this initcall is superfluous.

For the record the patch is at [1]

[1] https://lore.kernel.org/linux-pci/20230316103036.1837869-1-alexander.stein@ew.tq-group.com/T/#u

> >>  From which initcall is your driver probed?
> > 
> > The callstack looks like this:
> >> imx6_pcie_probe from platform_probe+0x5c/0xb8
> >> platform_probe from call_driver_probe+0x24/0x118
> >> call_driver_probe from really_probe+0xc4/0x31c
> >> really_probe from __driver_probe_device+0x8c/0x120
> >> __driver_probe_device from driver_probe_device+0x30/0xc0
> >> driver_probe_device from __driver_attach_async_helper+0x50/0xd8
> >> __driver_attach_async_helper from async_run_entry_fn+0x30/0x144
> >> async_run_entry_fn from process_one_work+0x1c4/0x3d0
> >> process_one_work from worker_thread+0x50/0x41c
> >> worker_thread from kthread+0xec/0x104
> >> kthread from ret_from_fork+0x14/0x2c
> > 
> > So technically the device is not probed from within a initcall but a
> > kthread. It is set to be probed asynchronous in imx6_pcie_driver.
> 
> That may be the problem, respectively that system is incomplete
> You are registering a PCI bridge. The PCI subsystem should be
> done setting up when you run. That is just a simple dependency.

Is there such an dependency in the first place? I can't see anything, even the 
late_initcall to pci_resource_alignment_sysfs_init is a different matter.

Best regards,
Alexander
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/