pci_remove_resource_files() frees the bin_attribute pointed at by
pdev->res_attr[i] / pdev->res_attr_wc[i] but does not clear the slot
after kfree().  If pci_remove_sysfs_dev_files() is ever invoked twice
against the same pdev, the second pass dereferences a freed
bin_attribute and faults inside strlen() called from
kernfs_remove_by_name_ns().

To the best of my knowledge no in-tree caller hits this today -- the
primary path (pci_stop_dev() -> pci_remove_sysfs_dev_files()) is
expected to fire once per device -- so this is being submitted as
defence-in-depth rather than as a fix for a reproducer in the upstream
tree.  The motivation is that a freed-but-not-cleared pointer in a
device-lifetime table is a sharp edge: any future caller (in-tree or
out-of-tree) that re-enters the teardown path turns it into a UAF
with no warning.  Other allocator-managed pointer tables in the
kernel NULL the slot after kfree() for exactly this reason, and the
two-line change makes pci_remove_resource_files() idempotent at
essentially zero cost.

For full disclosure: the way I encountered this was via an
out-of-tree PCIe hotplug driver on an AMD Ryzen Embedded V3000
platform.  The driver re-enters pci_stop_and_remove_bus_device()
against a pdev that the standard pci_destroy_dev() path has already
torn down, and with slub_debug=FZPU the second entry faults with the
classic POISON_FREE signature:

  BUG: unable to handle page fault for address: 6b6b6b6b6b6b6b6b
  RIP: strlen+0x4
  Call Trace:
    kernfs_find_ns
    kernfs_remove_by_name_ns
    sysfs_remove_bin_file
    pci_remove_resource_files
    pci_remove_sysfs_dev_files
    pci_stop_bus_device
    pci_stop_and_remove_bus_device
    <out-of-tree hotplug driver remove path>

I am aware that "out-of-tree driver re-enters teardown" is not by
itself a reason to take a change upstream, and I would understand a
NACK on that basis.  The reason I am still sending it is that the
fix is local, mechanical, matches an idiom already used elsewhere in
the kernel, and removes a class of bug rather than papering over the
specific caller that surfaced it.

Signed-off-by: Gianfranco Dutka <gianfranco.dutka@arista.com>
---
 drivers/pci/pci-sysfs.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -1228,11 +1228,13 @@ static void pci_remove_resource_files(struct pci_dev *pdev)
 		if (res_attr) {
 			sysfs_remove_bin_file(&pdev->dev.kobj, res_attr);
 			kfree(res_attr);
+			pdev->res_attr[i] = NULL;
 		}

 		res_attr = pdev->res_attr_wc[i];
 		if (res_attr) {
 			sysfs_remove_bin_file(&pdev->dev.kobj, res_attr);
 			kfree(res_attr);
+			pdev->res_attr_wc[i] = NULL;
 		}
 	}
 }
--
2.43.0

Hello,

> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -1228,11 +1228,13 @@ static void pci_remove_resource_files(struct pci_dev *pdev)
>  		if (res_attr) {
>  			sysfs_remove_bin_file(&pdev->dev.kobj, res_attr);
>  			kfree(res_attr);
> +			pdev->res_attr[i] = NULL;
>  		}
> 
>  		res_attr = pdev->res_attr_wc[i];
>  		if (res_attr) {
>  			sysfs_remove_bin_file(&pdev->dev.kobj, res_attr);
>  			kfree(res_attr);
> +			pdev->res_attr_wc[i] = NULL;
>  		}
>  	}
>  }

Similar such patches (for reference):

  - https://lore.kernel.org/linux-pci/20251010093023.2569-1-guojinhui.liam@bytedance.com/
  - https://lore.kernel.org/linux-pci/20250629030547.1073425-1-zhangjian496@huawei.com/
  - https://lore.kernel.org/linux-pci/20210507102706.7658-1-danijel.slivka@amd.com/
  - https://lore.kernel.org/linux-pci/20191024172157.878735-2-s.miroshnichenko@yadro.com/

Thank you!

	Krzysztof

On Wed, 20 May 2026, Gianfranco Dutka wrote:

> pci_remove_resource_files() frees the bin_attribute pointed at by
> pdev->res_attr[i] / pdev->res_attr_wc[i] but does not clear the slot
> after kfree().  If pci_remove_sysfs_dev_files() is ever invoked twice
> against the same pdev, the second pass dereferences a freed
> bin_attribute and faults inside strlen() called from
> kernfs_remove_by_name_ns().
> 
> To the best of my knowledge no in-tree caller hits this today -- the
> primary path (pci_stop_dev() -> pci_remove_sysfs_dev_files()) is
> expected to fire once per device -- so this is being submitted as
> defence-in-depth rather than as a fix for a reproducer in the upstream
> tree.  The motivation is that a freed-but-not-cleared pointer in a
> device-lifetime table is a sharp edge: any future caller (in-tree or
> out-of-tree) that re-enters the teardown path turns it into a UAF
> with no warning.  Other allocator-managed pointer tables in the
> kernel NULL the slot after kfree() for exactly this reason, and the
> two-line change makes pci_remove_resource_files() idempotent at
> essentially zero cost.
> 
> For full disclosure: the way I encountered this was via an
> out-of-tree PCIe hotplug driver on an AMD Ryzen Embedded V3000
> platform.  The driver re-enters pci_stop_and_remove_bus_device()
> against a pdev that the standard pci_destroy_dev() path has already
> torn down, and with slub_debug=FZPU the second entry faults with the
> classic POISON_FREE signature:
> 
>   BUG: unable to handle page fault for address: 6b6b6b6b6b6b6b6b
>   RIP: strlen+0x4
>   Call Trace:
>     kernfs_find_ns
>     kernfs_remove_by_name_ns
>     sysfs_remove_bin_file
>     pci_remove_resource_files
>     pci_remove_sysfs_dev_files
>     pci_stop_bus_device
>     pci_stop_and_remove_bus_device
>     <out-of-tree hotplug driver remove path>
> 
> I am aware that "out-of-tree driver re-enters teardown" is not by
> itself a reason to take a change upstream, and I would understand a
> NACK on that basis.  The reason I am still sending it is that the
> fix is local, mechanical, matches an idiom already used elsewhere in
> the kernel, and removes a class of bug rather than papering over the
> specific caller that surfaced it.

Hi,

To me it looks more like pci_remove_resource_files() is expected to be 
entered only once.

If this change is accepted, the 2nd entry is "silently" allowed whereas 
better course of action would feel to catch that in 
pci_remove_resource_files() with:

	if (WARN_ON_ONCE(something))
		return;

That way, developers may become aware the code has some lifetime issue.

(I'm not strictly against your change but I think it would be much better 
to have sane lifetimes and catch offenders.)

> Signed-off-by: Gianfranco Dutka <gianfranco.dutka@arista.com>
> ---
>  drivers/pci/pci-sysfs.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -1228,11 +1228,13 @@ static void pci_remove_resource_files(struct pci_dev *pdev)
>  		if (res_attr) {
>  			sysfs_remove_bin_file(&pdev->dev.kobj, res_attr);
>  			kfree(res_attr);
> +			pdev->res_attr[i] = NULL;
>  		}
> 
>  		res_attr = pdev->res_attr_wc[i];
>  		if (res_attr) {
>  			sysfs_remove_bin_file(&pdev->dev.kobj, res_attr);
>  			kfree(res_attr);
> +			pdev->res_attr_wc[i] = NULL;
>  		}
>  	}
>  }
> --
> 2.43.0
> 

-- 
 i.

> To me it looks more like pci_remove_resource_files() is expected to be 
> entered only once.
> 
> If this change is accepted, the 2nd entry is "silently" allowed whereas 
> better course of action would feel to catch that in 
> pci_remove_resource_files() with:
> 
> 	if (WARN_ON_ONCE(something))
> 		return;
> 
> That way, developers may become aware the code has some lifetime issue.
> 
> (I'm not strictly against your change but I think it would be much better 
> to have sane lifetimes and catch offenders.)

The upcoming sysfs changes would remove pci_remove_resource_files()
completely, so there would not be anything needed to be done here,
hopefully.

Thank you!

	Krzysztof

On Thu, 21 May 2026, Krzysztof Wilczyński wrote:

> > To me it looks more like pci_remove_resource_files() is expected to be 
> > entered only once.
> > 
> > If this change is accepted, the 2nd entry is "silently" allowed whereas 
> > better course of action would feel to catch that in 
> > pci_remove_resource_files() with:
> > 
> > 	if (WARN_ON_ONCE(something))
> > 		return;
> > 
> > That way, developers may become aware the code has some lifetime issue.
> > 
> > (I'm not strictly against your change but I think it would be much better 
> > to have sane lifetimes and catch offenders.)
> 
> The upcoming sysfs changes would remove pci_remove_resource_files()
> completely, so there would not be anything needed to be done here,
> hopefully.

Yeah, I should have looked at that branch first instead of checking just 
code in mainline.

-- 
 i.

Thanks for the review.

Both findings concern code paths I have not independently
debugged or reproduced, so I do not want to confirm or refute
them as bugs from the sender side of this patch -- that is for
the PCI maintainers and, in the alpha case, the alpha
maintainers to judge.

What I can say about this patch: it is intentionally
defence-in-depth, not a fix for the concurrency window or for
the alpha allocation pattern.  The read-then-NULL it adds is
not atomic, so if pci_remove_resource_files() is genuinely
reachable from two threads concurrently then this patch does
not close that window.  And the patch makes no change to how
the two pointers are freed, so if those pointers do alias the
same allocation on alpha then this patch does not change the
underlying free behaviour either.

For this patch itself I am happy with any of:

  - merge as-is for defence-in-depth, and let the issues raised
    in review be addressed (or not) in separate work,
  - hold it pending a proper fix for whichever of those issues
    you consider blocking,
  - NACK if you would rather not carry a NULL-after-free that
    does not address the underlying concerns.

Whichever you prefer.

Thanks,
Gianfranco

Hello,

> Whichever you prefer.

If you have a moment, consider changes from the following series:

  https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=sysfs

Test, and let us know if this fixes the problem for you.

I would appreciate if you could test this for us.

Thank you!

	Krzysztof