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: <4888964.44csPzL39Z@steina-w>
Date:   Thu, 16 Mar 2023 12:58:53 +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, 12:17:32 CET schrieb Oliver Neukum:
> On 16.03.23 10:33, Alexander Stein wrote:
> > Hi Oliver,
> 
> Hi,
> 
> > Admittedly
> > I would like to get rid of these two pathes for creating sysfs files in
> > the
> > first place, but I do not know the pci subsystem very well.
> > IMHO for_each_pci_dev(pdev) in pci_sysfs_init is part of the problem as it
> > unconditionally iterates over the bus, without any locks, thus creating
> > sysfs files for each device added to the bus.
> > Any ideas?
> 
> First of all, this existing code is a mess.
> 
> If I understand you have the issue that your driver adds a bridge
> in dw_pcie_host_init() and the generic code in pci_create_sysfs_dev_files()
> populates the directory before or while your driver does so and
> the devices are effectively discovered twice.

Yep, that's my observation as well.

> 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?
See my different approach eliminating this race at all.

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

This async call is scheduled in __driver_attach, from this callstack:
> __driver_attach from bus_for_each_dev+0x74/0xc8
> bus_for_each_dev from bus_add_driver+0xf0/0x1f4
> bus_add_driver from driver_register+0x7c/0x118
> driver_register from do_one_initcall+0x4c/0x180
> do_one_initcall from do_initcalls+0xe0/0x114
> do_initcalls from kernel_init_freeable+0xd8/0x100
> kernel_init_freeable from kernel_init+0x18/0x12c
> kernel_init from ret_from_fork+0x14/0x2c

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/