linux-kernel - Re: [External] Re: [PATCH] perf/dwc_pcie: fix duplicate pci

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <31e00081-8695-4ca6-a1ef-af5a007a7565@linux.alibaba.com>
Date: Fri, 24 Jan 2025 14:48:30 +0800
From: Shuai Xue <xueshuai@...ux.alibaba.com>
To: yunhui cui <cuiyunhui@...edance.com>
Cc: renyu.zj@...ux.alibaba.com, will@...nel.org, mark.rutland@....com,
 linux-arm-kernel@...ts.infradead.org, linux-perf-users@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [External] Re: [PATCH] perf/dwc_pcie: fix duplicate pci_dev
 devices



在 2025/1/24 10:56, yunhui cui 写道:
> Hi Shuai,
> 
> On Thu, Jan 23, 2025 at 5:50 PM Shuai Xue <xueshuai@...ux.alibaba.com> wrote:
>>
>>
>>
>> 在 2025/1/23 15:48, Yunhui Cui 写道:
>>> During platform_device_register, wrongly using struct device
>>> pci_dev as platform_data caused a kmemdup copy of pci_dev. Worse
>>> still, accessing the duplicated device leads to list corruption as its
>>> mutex content (e.g., list, magic) remains the same as the original.
>>>
>>> Signed-off-by: Yunhui Cui <cuiyunhui@...edance.com>
>>> ---
>>>    drivers/perf/dwc_pcie_pmu.c | 25 ++++++++++++++++++-------
>>>    1 file changed, 18 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/perf/dwc_pcie_pmu.c b/drivers/perf/dwc_pcie_pmu.c
>>> index cccecae9823f..8b208f287a1f 100644
>>> --- a/drivers/perf/dwc_pcie_pmu.c
>>> +++ b/drivers/perf/dwc_pcie_pmu.c
>>> @@ -565,9 +565,7 @@ static int dwc_pcie_register_dev(struct pci_dev *pdev)
>>>        u32 sbdf;
>>>
>>>        sbdf = (pci_domain_nr(pdev->bus) << 16) | PCI_DEVID(pdev->bus->number, pdev->devfn);
>>> -     plat_dev = platform_device_register_data(NULL, "dwc_pcie_pmu", sbdf,
>>> -                                              pdev, sizeof(*pdev));
>>> -
>>> +     plat_dev = platform_device_register_simple("platform_dwc_pcie", sbdf, NULL, 0);
>>>        if (IS_ERR(plat_dev))
>>>                return PTR_ERR(plat_dev);
>>>
>>> @@ -614,19 +612,32 @@ static struct notifier_block dwc_pcie_pmu_nb = {
>>>
>>>    static int dwc_pcie_pmu_probe(struct platform_device *plat_dev)
>>>    {
>>> -     struct pci_dev *pdev = plat_dev->dev.platform_data;
>>> +     struct pci_dev *pdev = NULL;
>>>        struct dwc_pcie_pmu *pcie_pmu;
>>> +     bool found = false;
>>>        char *name;
>>>        u32 sbdf;
>>>        u16 vsec;
>>>        int ret;
>>>
>>> +     for_each_pci_dev(pdev) {
>>> +             sbdf = (pci_domain_nr(pdev->bus) << 16) | PCI_DEVID(pdev->bus->number, pdev->devfn);
>>> +             if (sbdf == plat_dev->id) {
>>> +                     found = true;
>>> +                     break;
>>> +             }
>>> +     }
>>> +     if (!found) {
>>> +             pr_err("The plat_dev->id does not match the sbdf");
>>> +             return -ENODEV;
>>> +     }
>>> +
>>
>> How about using pci_get_domain_bus_and_slot() to find pci_dev?
> It's not necessary. pci_get_domain_bus_and_slot also invokes
> for_each_pci_dev, and it further requires splitting the sbdf.

Compose sbdf from domain, bus number, and devfn is almost essentially
corresponding opposite operation. And I think we should grab a reference
count of pdev and handle it properly.

Personally speaking, I prefer pci_get_domain_bus_and_slot() because it is
simple and robust.

> 
>>
>>>        vsec = dwc_pcie_des_cap(pdev);
>>>        if (!vsec)
>>>                return -ENODEV;
>>>
>>>        sbdf = plat_dev->id;
>>> -     name = devm_kasprintf(&plat_dev->dev, GFP_KERNEL, "dwc_rootport_%x", sbdf);
>>> +     name = devm_kasprintf(&plat_dev->dev, GFP_KERNEL, "dwc_rootport_%d_pmu", sbdf);
>>
>> A new name will break previous user tools.
> This name isn't suitable. It can't clearly show which is the PMU
> device. Userspace tools don't have binding relationships, like perf.
> Tools must traverse PMU devices before use.

The device is under /sys/bus/event_source/ which indates it is PMU device.
As far as I know, most of PMU devices do not endup with a '_pmu' prefix.

Thanks.

Shuai