[PATCH] fix: x86/events/intel/uncore: discover_upi_topology: inner loop leaks PCI device references from pci_get_domain_bus_and_slot

[PATCH] fix: x86/events/intel/uncore: discover_upi_topology: inner loop leaks PCI device references from pci_get_domain_bus_and_slot

[WenTao Liang]

In the inner for loop, dev is repeatedly overwritten by
  pci_get_domain_bus_and_slot() without first releasing the previous dev 
  via pci_dev_put(). The err label only releases the last ubox and dev
  references, while the references from earlier loop iterations are
  permanently leaked. Fix by adding pci_dev_put(dev) before the overwriting
  assignment.

Cc: stable@vger.kernel.org
Fixes: fdd041028f22 ("perf/x86/intel/uncore: Factor out topology_gidnid_map()")
Signed-off-by: WenTao Liang <vulab@iscas.ac.cn>
---
 arch/x86/events/intel/uncore_snbep.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/events/intel/uncore_snbep.c b/arch/x86/events/intel/uncore_snbep.c
index 215d33e260ed..cecc1ce0a248 100644
--- a/arch/x86/events/intel/uncore_snbep.c
+++ b/arch/x86/events/intel/uncore_snbep.c
@@ -5494,6 +5494,7 @@ static int discover_upi_topology(struct intel_uncore_type *type, int ubox_did, i
 		for (idx = 0; idx < type->num_boxes; idx++) {
 			upi = &type->topology[lgc_pkg][idx];
 			devfn = PCI_DEVFN(dev_link0 + idx, ICX_UPI_REGS_ADDR_FUNCTION);
+			pci_dev_put(dev);
 			dev = pci_get_domain_bus_and_slot(pci_domain_nr(ubox->bus),
 							  ubox->bus->number,
 							  devfn);
-- 
2.39.5 (Apple Git-154)

Re: [PATCH] fix: x86/events/intel/uncore: discover_upi_topology: inner loop leaks PCI device references from pci_get_domain_bus_and_slot

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] The patch fixes a PCI device reference leak in `discover_upi_topology()`, but misses an identical leak in `skx_upi_topology_cb()` within the same file.
- [High] `uncore_pci_pmus_register()` leaks PCI device references acquired via `pci_get_domain_bus_and_slot()`.
- [High] Uninitialized variable `ret` is returned from `skx_upi_topology_cb()` if no PCI devices are found.
--

commit 353e07e70f653f5204fbf8fbdb47ac7e0d92375b
Author: WenTao Liang <vulab@iscas.ac.cn>

fix: x86/events/intel/uncore: discover_upi_topology: inner loop leaks PCI device references from pci_get_domain_bus_and_slot
    
In the inner for loop, dev is repeatedly overwritten by
pci_get_domain_bus_and_slot() without first releasing the previous dev
via pci_dev_put(). The err label only releases the last ubox and dev
references, while the references from earlier loop iterations are
permanently leaked. Fix by adding pci_dev_put(dev) before the overwriting
assignment.

> diff --git a/arch/x86/events/intel/uncore_snbep.c b/arch/x86/events/intel/uncore_snbep.c
> index e1f370b8d065f..9eacc0b3d1212 100644
> --- a/arch/x86/events/intel/uncore_snbep.c
> +++ b/arch/x86/events/intel/uncore_snbep.c
> @@ -5600,6 +5600,7 @@ static int discover_upi_topology(struct intel_uncore_type *type, int ubox_did, i
>  		for (idx = 0; idx < type->num_boxes; idx++) {
>  			upi = &type->topology[lgc_pkg][idx];
>  			devfn = PCI_DEVFN(dev_link0 + idx, ICX_UPI_REGS_ADDR_FUNCTION);
> +			pci_dev_put(dev);
>  			dev = pci_get_domain_bus_and_slot(pci_domain_nr(ubox->bus),
>  							  ubox->bus->number,
>  							  devfn);

[Severity: High]
This is a pre-existing issue, but while you are fixing this leak here, I
noticed an identical leak in skx_upi_topology_cb() in the same file.

In arch/x86/events/intel/uncore_snbep.c:skx_upi_topology_cb():

	for (idx = 0; idx < type->num_boxes; idx++) {
		upi = &type->topology[die][idx];
		devfn = PCI_DEVFN(SKX_UPI_REGS_ADDR_DEVICE_LINK0 + idx,
				  SKX_UPI_REGS_ADDR_FUNCTION);
		dev = pci_get_domain_bus_and_slot(segment, bus, devfn);

If type->num_boxes > 1, dev is repeatedly overwritten without calling
pci_dev_put(dev) on the previous reference. Should skx_upi_topology_cb() be
updated to include the same fix?


[Severity: High]
This is another pre-existing issue, but looking closely at
skx_upi_topology_cb() in arch/x86/events/intel/uncore_snbep.c, the ret
variable is declared without initialization:

	int idx, ret;
	struct intel_uncore_topology *upi;
	unsigned int devfn;
	struct pci_dev *dev = NULL;
...
	for (idx = 0; idx < type->num_boxes; idx++) {
...
		dev = pci_get_domain_bus_and_slot(segment, bus, devfn);
		if (dev) {
			ret = upi_fill_topology(dev, upi, idx);
			if (ret)
				break;
		}
	}

	pci_dev_put(dev);
	return ret;

If pci_get_domain_bus_and_slot() returns NULL for all iterations, the loop
completes without executing the if (dev) block, and the function returns the
uninitialized ret. Could this result in returning stack garbage and treating
a failed discovery as successful?


[Severity: High]
This is also a pre-existing issue, but I noticed a broader leak involving
pci_get_domain_bus_and_slot() during uncore PMU registration.

In arch/x86/events/intel/uncore.c:uncore_pci_pmus_register():

			pdev = pci_get_domain_bus_and_slot(UNCORE_DISCOVERY_PCI_DOMAIN(unit->addr),
							   UNCORE_DISCOVERY_PCI_BUS(unit->addr),
							   UNCORE_DISCOVERY_PCI_DEVFN(unit->addr));

			if (!pdev)
				continue;
			pmu = &type->pmus[unit->pmu_idx];
			uncore_pci_pmu_register(pdev, type, pmu, unit->die);

It seems the reference-counted pdev acquired here is stored but never released
during module teardown (such as in uncore_free_boxes() and
uncore_pci_pmu_unregister()), which would leak the PCI device reference on
module unload. Does this teardown path need to call pci_dev_put(box->pci_dev)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260626100031.31494-1-vulab@iscas.ac.cn?part=1