Re: [PATCH 4/7] perf/x86/intel/uncore: Factor out box setup code

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Fatal NULL pointer dereference in uncore_bus_notify() on removal of unbound PCI devices.
--

commit 6a9ba5cde2aa109e895cd057ed9fdc789f5590c7
Author: Zide Chen <zide.chen@intel.com>

perf/x86/intel/uncore: Factor out box setup code

This commit factors the lazy registration management of the PCI uncore PMU
path into a shared helper function. This allows the same code to be reused
by the MSR and MMIO paths in future changes.

> diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
> index 65e9d0c64f8f9..617a68c1d76c3 100644
> --- a/arch/x86/events/intel/uncore.c
> +++ b/arch/x86/events/intel/uncore.c

[ ... ]

> @@ -1154,19 +1177,13 @@ static int uncore_pci_pmu_register(struct pci_dev *pdev,
>  	box->dieid = die;
>  	box->pci_dev = pdev;
>  	box->pmu = pmu;
> -	uncore_box_init(box);
> -
> -	pmu->boxes[die] = box;
> -	if (atomic_inc_return(&pmu->die_refcnt) > 1)
> -		return 0;
>  
> -	/* First active box registers the pmu */
> -	ret = uncore_pmu_register(pmu);
> -	if (ret) {
> -		pmu->boxes[die] = NULL;
> -		uncore_box_exit(box);
> +	ret = uncore_box_setup(pmu, box);
> +	if (!ret)
> +		pmu->boxes[die] = box;
> +	else
>  		kfree(box);

This isn't a bug introduced by your patch, but can this cause a NULL pointer
dereference if the device is later removed?

If uncore_box_setup() fails, pmu->boxes[die] remains NULL while the device
remains on the PCI bus without a bound driver. If this device is later
removed, uncore_bus_notify() fires for BUS_NOTIFY_DEL_DEVICE:

uncore_bus_notify() {
	...
	if (action != BUS_NOTIFY_DEL_DEVICE)
		return NOTIFY_DONE;

	pmu = uncore_pci_find_dev_pmu(pdev, ids);
	...
	uncore_pci_pmu_unregister(pmu, die);
}

This identifies the PMU via PCI IDs and unconditionally calls
uncore_pci_pmu_unregister(), without checking if the driver actually
bound to the device.

uncore_pci_pmu_unregister() {
	struct intel_uncore_box *box = pmu->boxes[die];
	...
	uncore_box_exit(box);
	kfree(box);
}

Since box is NULL here, will uncore_box_exit() dereference the NULL pointer
when it calls test_and_clear_bit(UNCORE_BOX_FLAG_INITIALIZED, &box->flags)?

> +
>  	return ret;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512233048.9577-1-zide.chen@intel.com?part=4