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: <CAEEQ3wmDej9bbPUmkx-ZrD4FG9Z7gTHMeCrVszx64g90O0RyiQ@mail.gmail.com>
Date: Fri, 24 Jan 2025 10:56:31 +0800
From: yunhui cui <cuiyunhui@...edance.com>
To: Shuai Xue <xueshuai@...ux.alibaba.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

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.

>
> >       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.

>
> >       if (!name)
> >               return -ENOMEM;
> >
> > @@ -640,7 +651,7 @@ static int dwc_pcie_pmu_probe(struct platform_device *plat_dev)
> >       pcie_pmu->on_cpu = -1;
> >       pcie_pmu->pmu = (struct pmu){
> >               .name           = name,
> > -             .parent         = &pdev->dev,
> > +             .parent         = &plat_dev->dev,
> >               .module         = THIS_MODULE,
> >               .attr_groups    = dwc_pcie_attr_groups,
> >               .capabilities   = PERF_PMU_CAP_NO_EXCLUDE,
> > @@ -727,7 +738,7 @@ static int dwc_pcie_pmu_offline_cpu(unsigned int cpu, struct hlist_node *cpuhp_n
> >
> >   static struct platform_driver dwc_pcie_pmu_driver = {
> >       .probe = dwc_pcie_pmu_probe,
> > -     .driver = {.name = "dwc_pcie_pmu",},
> > +     .driver = {.name = "platform_dwc_pcie",},
> >   };
> >
> >   static int __init dwc_pcie_pmu_init(void)
>
> Thanks.
>
> Best Regards,
> Shuai
>

Thanks,
Yunhui