Re: [PATCH RFC 2/3] PCI: Add per-device debugfs directories

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] PCI: system-wide stall during hotplug due to global pci_debugfs_lock held over debugfs removal
- [Medium] PCI: speculative resource leak due to unpoisoned debugfs pointer on teardown
--

commit 64859f6a500632327edcd41b730592212c9d3478
Author: Wei Huang <wei.huang2@amd.com>

PCI: Add per-device debugfs directories

This commit adds per-device debugfs directories owned by the PCI core,
using pci_dev_debugfs_dir() to create them on demand and pci_destroy_dev()
to recursively remove them.

> diff --git a/drivers/pci/debugfs.c b/drivers/pci/debugfs.c
> new file mode 100644
> index 0000000000000..682fb614d7cfb
> --- /dev/null
> +++ b/drivers/pci/debugfs.c

[ ... ]

> +void pci_dev_debugfs_remove(struct pci_dev *dev)
> +{
> +	guard(mutex)(&pci_debugfs_lock);
> +
> +	debugfs_remove_recursive(dev->debugfs);

[Severity: High]
Does holding the global pci_debugfs_lock during debugfs_remove_recursive()
risk a system-wide stall?

If a userspace process is actively reading a debugfs file, the removal will
block for SRCU synchronization. Because the global lock is held via
guard(mutex) for the entire function scope, any concurrent
pci_dev_debugfs_dir() or pci_dev_debugfs_remove() calls on unrelated PCI
devices will then block indefinitely during hotplug and removal operations.

Could the dentry be extracted under the lock and removed outside of it?

> +	dev->debugfs = NULL;

[Severity: Medium]
Can a concurrent runtime sysfs operation mistakenly recreate this debugfs
directory after removal?

This sets the pointer to NULL while the device is still exposed to the
system. As seen in pci_destroy_dev(), pci_dev_debugfs_remove() is called
prior to device_del(). If a concurrent operation calls pci_dev_debugfs_dir()
and sees NULL, it might recreate the directory and permanently leak it after
the device is finally deleted.

Should the pointer be explicitly poisoned with ERR_PTR(-ENODEV) instead
of set to NULL to prevent recreation?

> +}

> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
> index e711ac1d4e38a..e10139117d578 100644
> --- a/drivers/pci/remove.c
> +++ b/drivers/pci/remove.c

[ ... ]

> @@ -37,6 +37,7 @@ static void pci_destroy_dev(struct pci_dev *dev)
>  	platform_pci_remove_wake(dev);
>  	pci_doe_sysfs_teardown(dev);
>  	pci_npem_remove(dev);
> +	pci_dev_debugfs_remove(dev);
>  
>  	/*
>  	 * While device is in D0 drop the device from TSM link operations
>  	 * including unbind and disconnect (IDE + SPDM teardown).
>  	 */
>  	pci_tsm_destroy(dev);
>  
>  	device_del(&dev->dev);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001155551.4182899-1-wei.huang2@amd.com?part=2