Re: [PATCH v2 3/3] perf/dwc_pcie: Convert to faux device interface

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Lockless modifications to `dwc_pcie_dev_info_head` corrupt the global list during concurrent PCI hotplug events.
- [High] Returning NOTIFY_BAD halts the notifier chain and breaks subsystems.
- [High] Premature CPU hotplug registration crashes the kernel via a NULL pointer dereference.
--

commit 699b6522ae4c9d70c5bdeb799d79993149c02386
Author: Yicong Yang <yang.yicong@picoheart.com>

perf/dwc_pcie: Convert to faux device interface

The DWC PCIe PMU makes use of the platform device interface but
is not the real device, it's actually the RAS DES capability of
the root port. It's more appropriate to use the lightweight
faux framework to abstract this, it'll be more simple and no
need for the complete device and drivers model. So convert to
the faux device interface.

> diff --git a/drivers/perf/dwc_pcie_pmu.c b/drivers/perf/dwc_pcie_pmu.c
> index 7ec8302d4090b..3a9965b5abf4b 100644
> --- a/drivers/perf/dwc_pcie_pmu.c
> +++ b/drivers/perf/dwc_pcie_pmu.c
> @@ -684,31 +691,41 @@ static u16 dwc_pcie_des_cap(struct pci_dev *pdev)
>  
>  static void dwc_pcie_unregister_dev(struct dwc_pcie_dev_info *dev_info)
>  {
> -	platform_device_unregister(dev_info->plat_dev);
> +	faux_device_destroy(dev_info->fdev);
>  	list_del(&dev_info->dev_node);
> +	kfree(dev_info->name);
>  	kfree(dev_info);
>  }
>  
>  static int dwc_pcie_register_dev(struct pci_dev *pdev)
>  {
> -	struct platform_device *plat_dev;
>  	struct dwc_pcie_dev_info *dev_info;
> +	struct faux_device *fdev;
> +	char *name;
>  	u32 sbdf;
>  
>  	sbdf = (pci_domain_nr(pdev->bus) << 16) | PCI_DEVID(pdev->bus->number, pdev->devfn);
> -	plat_dev = platform_device_register_simple("dwc_pcie_pmu", sbdf, NULL, 0);
> -	if (IS_ERR(plat_dev))
> -		return PTR_ERR(plat_dev);
> +	name = kasprintf(GFP_KERNEL, "dwc_pcie_pmu_%x", sbdf);
> +	if (!name)
> +		return -ENOMEM;
> +
> +	fdev = faux_device_create(name, &pdev->dev, &dwc_pcie_faux_ops);
> +	if (!fdev) {
> +		kfree(name);
> +		return -ENODEV;
> +	}
>  
>  	dev_info = kzalloc_obj(*dev_info);
>  	if (!dev_info) {
> -		platform_device_unregister(plat_dev);
> +		faux_device_destroy(fdev);
> +		kfree(name);
>  		return -ENOMEM;
>  	}
>  
> -	/* Cache platform device to handle pci device hotplug */
> -	dev_info->plat_dev = plat_dev;
> +	/* Cache faux device to handle pci device hotplug */
> +	dev_info->fdev = fdev;
>  	dev_info->pdev = pdev;
> +	dev_info->name = name;
>  	list_add(&dev_info->dev_node, &dwc_pcie_dev_info_head);

[Severity: High]
This isn't a bug introduced by this patch, but does this list modification
need synchronization?

Concurrent PCI hotplug events can invoke the bus notifier, which calls
dwc_pcie_register_dev() or dwc_pcie_unregister_dev(). This results in
list_add() or list_del() on dwc_pcie_dev_info_head without any locking,
which can cause list corruption and lead to an infinite loop or kernel panic.

[Severity: High]
This is a pre-existing issue, but if dwc_pcie_register_dev() fails
during the BUS_NOTIFY_ADD_DEVICE event:

drivers/perf/dwc_pcie_pmu.c:dwc_pcie_pmu_notifier() {
    ...
    case BUS_NOTIFY_ADD_DEVICE:
        if (dwc_pcie_register_dev(pdev))
            return NOTIFY_BAD;
    ...
}

Does returning NOTIFY_BAD here halt the notifier chain?

If an allocation fails in dwc_pcie_register_dev(), returning NOTIFY_BAD
aborts the bus notifier chain. This can silently break other subsystems
like the IOMMU that rely on BUS_NOTIFY_ADD_DEVICE.

[ ... ]

> @@ -811,7 +820,7 @@ static int dwc_pcie_pmu_probe(struct platform_device *plat_dev)
>  	}
>  
>  	/* Unwind when platform driver removes */
> -	ret = devm_add_action_or_reset(&plat_dev->dev,
> +	ret = devm_add_action_or_reset(&fdev->dev,
>  				       dwc_pcie_pmu_remove_cpuhp_instance,
>  				       &pcie_pmu->cpuhp_node);
>  	if (ret)

[Severity: High]
This is a pre-existing issue, but can this cause a NULL pointer dereference
if a CPU offline event happens during probe?

In dwc_pcie_pmu_probe(), cpuhp_state_add_instance() is called right before
this point, and perf_pmu_register() is called afterward. If a CPU offline
event occurs in that window:

drivers/perf/dwc_pcie_pmu.c:dwc_pcie_pmu_offline_cpu() {
    ...
    perf_pmu_migrate_context(&pcie_pmu->pmu, cpu, target);
    ...
}

Because perf_pmu_register() hasn't been called yet, pmu->cpu_pmu_context is
not yet allocated. Does this cause perf_pmu_migrate_context() to dereference
a NULL cpu_pmu_context?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260629092717.74946-1-yang.yicong@picoheart.com?part=3