[PATCH v4] PCI/proc: Fix race between pci_proc_init() and pci_bus_add_device()

[PATCH v4] PCI/proc: Fix race between pci_proc_init() and pci_bus_add_device()

[Krzysztof Wilczyński]

pci_proc_attach_device() creates procfs entries for PCI devices and is
called from pci_bus_add_device().  It lazily creates the per-bus procfs
directory (bus->procdir) via proc_mkdir() on first use, and returns
early if proc_initialized is not yet set.

On x86 with ACPI, PCI enumeration occurs at subsys_initcall, before
pci_proc_init() sets proc_initialized at device_initcall.  The
for_each_pci_dev() loop in pci_proc_init() then creates procfs entries
for these already-enumerated devices, but runs without holding
pci_rescan_remove_lock.

On ARM64 with devicetree, PCI host bridges probe at device_initcall.
With async probing enabled, pci_bus_add_device() can run concurrently
with pci_proc_init(), and both may call pci_proc_attach_device() for
the same device or for different devices on the same bus.  As
pci_host_probe() holds pci_rescan_remove_lock while pci_proc_init()
does not, there is no serialisation between the two paths.

When two threads concurrently call pci_proc_attach_device() for
devices on the same bus, both observe bus->procdir as NULL and both
call proc_mkdir().  The proc filesystem serialises directory creation
internally, so only one caller succeeds.  The other receives NULL
(duplicate entry) and unconditionally stores it to bus->procdir,
corrupting the valid pointer set by the first caller.

Thus, serialise access to proc_initialized, proc_bus_pci_dir,
bus->procdir and dev->procent with a new mutex local to the
drivers/pci/proc.c file, and store the created entries to
bus->procdir and dev->procent only on success, so a failed
creation can never overwrite a valid pointer.

Additionally, wrap the for_each_pci_dev() loop in pci_proc_init() with
pci_lock_rescan_remove() to serialise against concurrent PCI bus
operations, add an early return in pci_proc_attach_device() when
dev->procent is already set to make the function idempotent, and clear
bus->procdir in pci_proc_detach_bus() to prevent use of a dangling
pointer after proc_remove().

Reported-by: Shuan He <heshuan@bytedance.com>
Closes: https://lore.kernel.org/linux-pci/20250702155112.40124-2-heshuan@bytedance.com/
Signed-off-by: Krzysztof Wilczyński <kwilczynski@kernel.org>
---
Changes in v4:
  https://lore.kernel.org/linux-pci/20260606203022.743558-1-kwilczynski@kernel.org/
 
  - Dropped the pci_proc_attach_bus() calls from pci_register_host_bridge()
    and pci_alloc_child_bus(), returning to creating the per-bus procfs
    directory lazily on first device attach.  Creating the directory at
    bus creation time left directories behind for buses that never
    receive any devices, changing the visible contents of /proc/bus/pci.
  - Serialised pci_proc_attach_device(), pci_proc_detach_device() and
    pci_proc_detach_bus() with a new mutex local to drivers/pci/proc.c.
    The pci_rescan_remove_lock cannot serialise every caller, as for
    example pci_enable_sriov() reaches pci_bus_add_device() both with
    the lock held (a driver probed during boot enumeration) and without
    it (the same driver probed from a later modprobe), so the procfs
    attach path can neither take the lock itself nor expect all callers
    to hold it.
  - Set proc_initialized in pci_proc_init() with the new mutex held, so
    that a concurrent caller can no longer observe it set while
    proc_bus_pci_dir is not yet visible.
  - Added the missing Reported-by tag to credit Shuan He properly.

Changes in v3:
  https://lore.kernel.org/linux-pci/20260606170146.673140-1-kwilczynski@kernel.org/

  - Used local variables in pci_proc_attach_bus() and pci_proc_attach_device()
    to ensure that bus->procdir and dev->procent are only assigned on success.
    If either the proc_mkdir() or proc_create_data() returns NULL, the struct
    field will not see a new assignment, preventing a valid pointer from being
    overwritten.  This was reported by Sashiko, see:
    https://sashiko.dev/#/patchset/20260606170146.673140-1-kwilczynski%40kernel.org?part=1
   
Changes in v2:
  https://lore.kernel.org/linux-pci/20260430003542.455198-1-kwilczynski@kernel.org/

  - Extracted bus->procdir creation into new pci_proc_attach_bus()
    function, addressing the suggestion to move proc_mkdir() to a bus
    creation path, as per Bjorn Helgaas' feedback.
  - Added bus->procdir = NULL in pci_proc_detach_bus() for symmetry
    and to prevent dangling pointer after proc_remove().
 drivers/pci/proc.c | 79 ++++++++++++++++++++++++++++++++--------------
 1 file changed, 56 insertions(+), 23 deletions(-)

diff --git a/drivers/pci/proc.c b/drivers/pci/proc.c
index ce36e35681e8..71ad289fcb8e 100644
--- a/drivers/pci/proc.c
+++ b/drivers/pci/proc.c
@@ -18,6 +18,7 @@
 #include "pci.h"
 
 static int proc_initialized;	/* = 0 */
+static DEFINE_MUTEX(pci_proc_lock);
 
 static loff_t proc_bus_pci_lseek(struct file *file, loff_t off, int whence)
 {
@@ -416,40 +417,64 @@ static const struct seq_operations proc_bus_pci_devices_op = {
 
 static struct proc_dir_entry *proc_bus_pci_dir;
 
-int pci_proc_attach_device(struct pci_dev *dev)
+static int __pci_proc_attach_bus(struct pci_bus *bus)
 {
-	struct pci_bus *bus = dev->bus;
-	struct proc_dir_entry *e;
+	struct proc_dir_entry *dir;
 	char name[16];
 
+	lockdep_assert_held(&pci_proc_lock);
+
 	if (!proc_initialized)
 		return -EACCES;
 
-	if (!bus->procdir) {
-		if (pci_proc_domain(bus)) {
-			sprintf(name, "%04x:%02x", pci_domain_nr(bus),
-					bus->number);
-		} else {
-			sprintf(name, "%02x", bus->number);
-		}
-		bus->procdir = proc_mkdir(name, proc_bus_pci_dir);
-		if (!bus->procdir)
-			return -ENOMEM;
-	}
+	if (bus->procdir)
+		return 0;
+
+	if (pci_proc_domain(bus))
+		sprintf(name, "%04x:%02x", pci_domain_nr(bus), bus->number);
+	else
+		sprintf(name, "%02x", bus->number);
+
+	dir = proc_mkdir(name, proc_bus_pci_dir);
+	if (!dir)
+		return -ENOMEM;
+
+	bus->procdir = dir;
+
+	return 0;
+}
+
+int pci_proc_attach_device(struct pci_dev *dev)
+{
+	struct pci_bus *bus = dev->bus;
+	struct proc_dir_entry *entry;
+	char name[16];
+	int ret;
+
+	guard(mutex)(&pci_proc_lock);
+
+	if (dev->procent)
+		return 0;
+
+	ret = __pci_proc_attach_bus(bus);
+	if (ret)
+		return ret;
 
 	sprintf(name, "%02x.%x", PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn));
-	e = proc_create_data(name, S_IFREG | S_IRUGO | S_IWUSR, bus->procdir,
-			     &proc_bus_pci_ops, dev);
-	if (!e)
+	entry = proc_create_data(name, S_IFREG | S_IRUGO | S_IWUSR,
+				 bus->procdir, &proc_bus_pci_ops, dev);
+	if (!entry)
 		return -ENOMEM;
-	proc_set_size(e, dev->cfg_size);
-	dev->procent = e;
+
+	proc_set_size(entry, dev->cfg_size);
+	dev->procent = entry;
 
 	return 0;
 }
 
 int pci_proc_detach_device(struct pci_dev *dev)
 {
+	guard(mutex)(&pci_proc_lock);
 	proc_remove(dev->procent);
 	dev->procent = NULL;
 	return 0;
@@ -457,19 +482,27 @@ int pci_proc_detach_device(struct pci_dev *dev)
 
 int pci_proc_detach_bus(struct pci_bus *bus)
 {
+	guard(mutex)(&pci_proc_lock);
 	proc_remove(bus->procdir);
+	bus->procdir = NULL;
 	return 0;
 }
 
 static int __init pci_proc_init(void)
 {
 	struct pci_dev *dev = NULL;
-	proc_bus_pci_dir = proc_mkdir("bus/pci", NULL);
-	proc_create_seq("devices", 0, proc_bus_pci_dir,
-		    &proc_bus_pci_devices_op);
-	proc_initialized = 1;
+
+	scoped_guard(mutex, &pci_proc_lock) {
+		proc_bus_pci_dir = proc_mkdir("bus/pci", NULL);
+		proc_create_seq("devices", 0, proc_bus_pci_dir,
+				&proc_bus_pci_devices_op);
+		proc_initialized = 1;
+	}
+
+	pci_lock_rescan_remove();
 	for_each_pci_dev(dev)
 		pci_proc_attach_device(dev);
+	pci_unlock_rescan_remove();
 
 	return 0;
 }
-- 
2.54.0

Re: [PATCH v4] PCI/proc: Fix race between pci_proc_init() and pci_bus_add_device()

[Krzysztof Wilczyński]

Hello,

[...]
> Thus, serialise access to proc_initialized, proc_bus_pci_dir,
> bus->procdir and dev->procent with a new mutex local to the
> drivers/pci/proc.c file, and store the created entries to
> bus->procdir and dev->procent only on success, so a failed
> creation can never overwrite a valid pointer.
> 
> Additionally, wrap the for_each_pci_dev() loop in pci_proc_init() with
> pci_lock_rescan_remove() to serialise against concurrent PCI bus
> operations, add an early return in pci_proc_attach_device() when
> dev->procent is already set to make the function idempotent, and clear
> bus->procdir in pci_proc_detach_bus() to prevent use of a dangling
> pointer after proc_remove().
> 
> Reported-by: Shuan He <heshuan@bytedance.com>
> Closes: https://lore.kernel.org/linux-pci/20250702155112.40124-2-heshuan@bytedance.com/

Applied to procfs branch.

Thank you,

	Krzysztof

Re: [PATCH v4] PCI/proc: Fix race between pci_proc_init() and pci_bus_add_device()

[Krzysztof Wilczyński]

Hello,

Related fix:

  https://lore.kernel.org/linux-fsdevel/20260612153031.536525-1-kwilczynski@kernel.org/

Found while working on this patch.

Thank you,

	Krzysztof