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: <6131694.LvFx2qVVIh@steina-w>
Date:   Thu, 16 Mar 2023 10:33:38 +0100
From:   Alexander Stein <alexander.stein@...tq-group.com>
To:     Bjorn Helgaas <bhelgaas@...gle.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, 10:23:54 CET schrieb Oliver Neukum:
> On 16.03.23 10:15, Alexander Stein wrote:
> > From: Korneliusz Osmenda <korneliuszo@...il.com>
> > 
> > On Gateworks Ventana there is a number of PCI devices and:
> >    - imx6_pcie_probe takes longer than start of late init
> >    - pci_sysfs_init sets up flag sysfs_initialized
> >    - pci_sysfs_init initializes already found devices
> >    - imx6_pcie_probe tries to reinitialize device
> > 
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=215515
> > 
> > Signed-off-by: Korneliusz Osmenda <korneliuszo@...il.com>
> > Signed-off-by: Alexander Stein <alexander.stein@...tq-group.com>
> > ---
> > 
> >   drivers/pci/pci-sysfs.c | 6 ++++++
> >   include/linux/pci.h     | 2 ++
> >   2 files changed, 8 insertions(+)
> > 
> > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > index dd0d9d9bc509..998e44716b6f 100644
> > --- a/drivers/pci/pci-sysfs.c
> > +++ b/drivers/pci/pci-sysfs.c
> > @@ -1497,6 +1497,9 @@ int __must_check pci_create_sysfs_dev_files(struct
> > pci_dev *pdev)> 
> >   	if (!sysfs_initialized)
> >   	
> >   		return -EACCES;
> > 
> > +	if (atomic_cmpxchg(&pdev->sysfs_init_cnt, 0, 1) == 1)
> > +		return 0;		/* already added */
> > +
> > 
> >   	return pci_create_resource_files(pdev);
> 
> This is very likely a bug. You are returning an error in the error
> case. Yet the flag stays.

Ah, you are right. This is something needed to address.

> And simply resetting it in the error case
> would be a race. There is something fishy in that design.

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?

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/