| Message ID | 20250331121536.2626552-1-andy.xu@hj-micro.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] perf: arm-ni: Fix list_add() corruption in arm_ni_probe() | expand |
On 2025-03-31 1:15 pm, HongBo Yao wrote: > From: Hongbo Yao <andy.xu@hj-micro.com> > > When a resource allocation fails in one clock domain of an NI device, > we need to properly roll back all previously registered perf PMUs in > other clock domains of the same device. > > Otherwise, it can lead to kernel panics. > > Calling arm_ni_init+0x0/0xff8 [arm_ni] @ 2374 > arm-ni ARMHCB70:00: Failed to request PMU region 0x1f3c13000 > arm-ni ARMHCB70:00: probe with driver arm-ni failed with error -16 > list_add corruption: next->prev should be prev (fffffd01e9698a18), > but was 0000000000000000. (next=ffff10001a0decc8). > pstate: 6340009 (nZCv daif +PAN -UAO +TCO +DIT -SSBS BTYPE=--) > pc : list_add_valid_or_report+0x7c/0xb8 > lr : list_add_valid_or_report+0x7c/0xb8 > Call trace: > __list_add_valid_or_report+0x7c/0xb8 > perf_pmu_register+0x22c/0x3a0 > arm_ni_probe+0x554/0x70c [arm_ni] > platform_probe+0x70/0xe8 > really_probe+0xc6/0x4d8 > driver_probe_device+0x48/0x170 > __driver_attach+0x8e/0x1c0 > bus_for_each_dev+0x64/0xf0 > driver_add+0x138/0x260 > bus_add_driver+0x68/0x138 > __platform_driver_register+0x2c/0x40 > arm_ni_init+0x14/0x2a [arm_ni] > do_init_module+0x36/0x298 > ---[ end trace 0000000000000000 ]--- > Kernel panic - not syncing: Oops - BUG: Fatal exception > SMP: stopping secondary CPUs This, and the subject, are really confusing, since apparent list corruption is just one of many possible symptoms, and the fact that it happens to be a second instance of arm_ni_probe() hitting it is pure coincidence. The bug is that probe failure can lead to PMUs being freed without being unregistered, which can obviously lead to all manner of use-after-free conditions in the perf core. > Fixes: 4d5a7680f2b4 ("perf: Add driver for Arm NI-700 interconnect PMU") > Signed-off-by: Hongbo Yao <andy.xu@hj-micro.com> > --- > drivers/perf/arm-ni.c | 44 +++++++++++++++++++++++++++++-------------- > 1 file changed, 30 insertions(+), 14 deletions(-) > > diff --git a/drivers/perf/arm-ni.c b/drivers/perf/arm-ni.c > index fd7a5e60e963..ee85577e86b9 100644 > --- a/drivers/perf/arm-ni.c > +++ b/drivers/perf/arm-ni.c > @@ -102,6 +102,7 @@ struct arm_ni_unit { > struct arm_ni_cd { > void __iomem *pmu_base; > u16 id; > + bool pmu_registered; > int num_units; > int irq; > int cpu; > @@ -571,10 +572,31 @@ static int arm_ni_init_cd(struct arm_ni *ni, struct arm_ni_node *node, u64 res_s > err = perf_pmu_register(&cd->pmu, name, -1); > if (err) > cpuhp_state_remove_instance_nocalls(arm_ni_hp_state, &cd->cpuhp_node); > + else > + cd->pmu_registered = true; > > return err; > } > > +static void arm_ni_remove_cds(struct arm_ni *ni) > +{ > + for (int i = 0; i < ni->num_cds; i++) { > + struct arm_ni_cd *cd = ni->cds + i; > + > + if (!cd->pmu_base) > + continue; > + > + if (!cd->pmu_registered) > + continue; It's clearly redundant to have both checks here - TBH I'd be inclined to just do the minimal fix with the existing pmu_base logic: @@ -656,8 +673,11 @@ static int arm_ni_probe(struct platform_device *pdev) reg = readl_relaxed(pd.base + NI_CHILD_PTR(c)); arm_ni_probe_domain(base + reg, &cd); ret = arm_ni_init_cd(ni, &cd, res->start); - if (ret) + if (ret) { + ni->cds[cd.id].pmu_base = NULL; + arm_ni_remove(pdev); return ret; + } } } } ...however I'm not against replacing that use of pmu_base with an explicit flag if you think that's nicer. As implied, though, I really don't see any point in splitting up arm_ni_remove() itself, we can just move the whole definition up and call it for this purpose too (with your other drvdata fix). Thanks, Robin.
在 2025/4/2 20:17, Robin Murphy 写道: > On 2025-03-31 1:15 pm, HongBo Yao wrote: >> From: Hongbo Yao <andy.xu@hj-micro.com> >> >> When a resource allocation fails in one clock domain of an NI device, >> we need to properly roll back all previously registered perf PMUs in >> other clock domains of the same device. >> >> Otherwise, it can lead to kernel panics. >> >> Calling arm_ni_init+0x0/0xff8 [arm_ni] @ 2374 >> arm-ni ARMHCB70:00: Failed to request PMU region 0x1f3c13000 >> arm-ni ARMHCB70:00: probe with driver arm-ni failed with error -16 >> list_add corruption: next->prev should be prev (fffffd01e9698a18), >> but was 0000000000000000. (next=ffff10001a0decc8). >> pstate: 6340009 (nZCv daif +PAN -UAO +TCO +DIT -SSBS BTYPE=--) >> pc : list_add_valid_or_report+0x7c/0xb8 >> lr : list_add_valid_or_report+0x7c/0xb8 >> Call trace: >> __list_add_valid_or_report+0x7c/0xb8 >> perf_pmu_register+0x22c/0x3a0 >> arm_ni_probe+0x554/0x70c [arm_ni] >> platform_probe+0x70/0xe8 >> really_probe+0xc6/0x4d8 >> driver_probe_device+0x48/0x170 >> __driver_attach+0x8e/0x1c0 >> bus_for_each_dev+0x64/0xf0 >> driver_add+0x138/0x260 >> bus_add_driver+0x68/0x138 >> __platform_driver_register+0x2c/0x40 >> arm_ni_init+0x14/0x2a [arm_ni] >> do_init_module+0x36/0x298 >> ---[ end trace 0000000000000000 ]--- >> Kernel panic - not syncing: Oops - BUG: Fatal exception >> SMP: stopping secondary CPUs > > This, and the subject, are really confusing, since apparent list > corruption is just one of many possible symptoms, and the fact that it > happens to be a second instance of arm_ni_probe() hitting it is pure > coincidence. The bug is that probe failure can lead to PMUs being freed > without being unregistered, which can obviously lead to all manner of > use-after-free conditions in the perf core. > Hi, robin. Thank you for the feedback. I'll revise the title as suggested. >> Fixes: 4d5a7680f2b4 ("perf: Add driver for Arm NI-700 interconnect PMU") >> Signed-off-by: Hongbo Yao <andy.xu@hj-micro.com> >> --- >> drivers/perf/arm-ni.c | 44 +++++++++++++++++++++++++++++-------------- >> 1 file changed, 30 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/perf/arm-ni.c b/drivers/perf/arm-ni.c >> index fd7a5e60e963..ee85577e86b9 100644 >> --- a/drivers/perf/arm-ni.c >> +++ b/drivers/perf/arm-ni.c >> @@ -102,6 +102,7 @@ struct arm_ni_unit { >> struct arm_ni_cd { >> void __iomem *pmu_base; >> u16 id; >> + bool pmu_registered; >> int num_units; >> int irq; >> int cpu; >> @@ -571,10 +572,31 @@ static int arm_ni_init_cd(struct arm_ni *ni, >> struct arm_ni_node *node, u64 res_s >> err = perf_pmu_register(&cd->pmu, name, -1); >> if (err) >> cpuhp_state_remove_instance_nocalls(arm_ni_hp_state, &cd- >> >cpuhp_node); >> + else >> + cd->pmu_registered = true; >> return err; >> } >> +static void arm_ni_remove_cds(struct arm_ni *ni) >> +{ >> + for (int i = 0; i < ni->num_cds; i++) { >> + struct arm_ni_cd *cd = ni->cds + i; >> + >> + if (!cd->pmu_base) >> + continue; >> + >> + if (!cd->pmu_registered) >> + continue; > > It's clearly redundant to have both checks here - TBH I'd be inclined to > just do the minimal fix with the existing pmu_base logic: > > @@ -656,8 +673,11 @@ static int arm_ni_probe(struct platform_device *pdev) > reg = readl_relaxed(pd.base + NI_CHILD_PTR(c)); > arm_ni_probe_domain(base + reg, &cd); > ret = arm_ni_init_cd(ni, &cd, res->start); > - if (ret) > + if (ret) { > + ni->cds[cd.id].pmu_base = NULL; > + arm_ni_remove(pdev); > return ret; > + } > } > } > } > > ...however I'm not against replacing that use of pmu_base with an > explicit flag if you think that's nicer. As implied, though, I really > don't see any point in splitting up arm_ni_remove() itself, we can just > move the whole definition up and call it for this purpose too (with your > other drvdata fix). > You're right, i'll simplify the fix by leveraging the exisiting pmu_base logic and move the arm_ni_remove() definition up in the next version. Appreciate the guidance. Best regards, Hongbo. > Thanks, > Robin. > >
diff --git a/drivers/perf/arm-ni.c b/drivers/perf/arm-ni.c index fd7a5e60e963..ee85577e86b9 100644 --- a/drivers/perf/arm-ni.c +++ b/drivers/perf/arm-ni.c @@ -102,6 +102,7 @@ struct arm_ni_unit { struct arm_ni_cd { void __iomem *pmu_base; u16 id; + bool pmu_registered; int num_units; int irq; int cpu; @@ -571,10 +572,31 @@ static int arm_ni_init_cd(struct arm_ni *ni, struct arm_ni_node *node, u64 res_s err = perf_pmu_register(&cd->pmu, name, -1); if (err) cpuhp_state_remove_instance_nocalls(arm_ni_hp_state, &cd->cpuhp_node); + else + cd->pmu_registered = true; return err; } +static void arm_ni_remove_cds(struct arm_ni *ni) +{ + for (int i = 0; i < ni->num_cds; i++) { + struct arm_ni_cd *cd = ni->cds + i; + + if (!cd->pmu_base) + continue; + + if (!cd->pmu_registered) + continue; + + writel_relaxed(0, cd->pmu_base + NI_PMCR); + writel_relaxed(U32_MAX, cd->pmu_base + NI_PMINTENCLR); + perf_pmu_unregister(&cd->pmu); + cpuhp_state_remove_instance_nocalls(arm_ni_hp_state, &cd->cpuhp_node); + } +} + + static void arm_ni_probe_domain(void __iomem *base, struct arm_ni_node *node) { u32 reg = readl_relaxed(base + NI_NODE_TYPE); @@ -593,6 +615,7 @@ static int arm_ni_probe(struct platform_device *pdev) void __iomem *base; static atomic_t id; int num_cds; + int ret; u32 reg, part; /* @@ -651,35 +674,28 @@ static int arm_ni_probe(struct platform_device *pdev) reg = readl_relaxed(vd.base + NI_CHILD_PTR(p)); arm_ni_probe_domain(base + reg, &pd); for (int c = 0; c < pd.num_components; c++) { - int ret; - reg = readl_relaxed(pd.base + NI_CHILD_PTR(c)); arm_ni_probe_domain(base + reg, &cd); ret = arm_ni_init_cd(ni, &cd, res->start); if (ret) - return ret; + goto init_cd_cleanup; } } } return 0; + +init_cd_cleanup: + arm_ni_remove_cds(ni); + + return ret; } static void arm_ni_remove(struct platform_device *pdev) { struct arm_ni *ni = platform_get_drvdata(pdev); - for (int i = 0; i < ni->num_cds; i++) { - struct arm_ni_cd *cd = ni->cds + i; - - if (!cd->pmu_base) - continue; - - writel_relaxed(0, cd->pmu_base + NI_PMCR); - writel_relaxed(U32_MAX, cd->pmu_base + NI_PMINTENCLR); - perf_pmu_unregister(&cd->pmu); - cpuhp_state_remove_instance_nocalls(arm_ni_hp_state, &cd->cpuhp_node); - } + arm_ni_remove_cds(ni); } #ifdef CONFIG_OF