diff mbox series

perf/dwc_pcie: fix duplicate pci_dev devices

Message ID 20250123074830.90923-1-cuiyunhui@bytedance.com (mailing list archive)
State New
Headers show
Series perf/dwc_pcie: fix duplicate pci_dev devices | expand

Commit Message

yunhui cui Jan. 23, 2025, 7:48 a.m. UTC 
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@bytedance.com>
---
 drivers/perf/dwc_pcie_pmu.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

Comments

Shuai Xue Jan. 23, 2025, 9:49 a.m. UTC | #1 
在 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@bytedance.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?

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

>   	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
yunhui cui Jan. 24, 2025, 2:56 a.m. UTC | #2 
Hi Shuai,

On Thu, Jan 23, 2025 at 5:50 PM Shuai Xue <xueshuai@linux.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@bytedance.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
diff mbox series

Patch

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;
+	}
+
 	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);
 	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)