| Message ID | 20200602025612.62707-1-aik@ozlabs.ru (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b460b512417ae9c8b51a3bdcc09020cd6c60ff69 |
| Headers | show |
| Series | [kernel] powerpc/perf: Stop crashing with generic_compat_pmu | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (00ec79b0b767994422c43792d73ff1327714a73f) |
| snowpatch_ozlabs/build-ppc64le | success | Build succeeded |
| snowpatch_ozlabs/build-ppc64be | success | Build succeeded |
| snowpatch_ozlabs/build-ppc64e | success | Build succeeded |
| snowpatch_ozlabs/build-pmac32 | success | Build succeeded |
| snowpatch_ozlabs/checkpatch | warning | total: 0 errors, 0 warnings, 2 checks, 37 lines checked |
| snowpatch_ozlabs/needsstable | success | Patch has no Fixes tags |
On 6/2/20 8:26 AM, Alexey Kardashevskiy wrote: > The bhrb_filter_map ("The Branch History Rolling Buffer") callback is > only defined in raw CPUs' power_pmu structs. The "architected" CPUs use > generic_compat_pmu which does not have this callback and crashed occur. > > This add a NULL pointer check for bhrb_filter_map() which behaves as if > the callback returned an error. > > This does not add the same check for config_bhrb() as the only caller > checks for cpuhw->bhrb_users which remains zero if bhrb_filter_map==0. Changes looks fine. Reviewed-by: Madhavan Srinivasan <maddy@linux.ibm.com> The commit be80e758d0c2e ('powerpc/perf: Add generic compat mode pmu driver') which introduced generic_compat_pmu was merged in v5.2. So we need to CC stable starting from 5.2 :( . My bad, sorry. Maddy > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > --- > arch/powerpc/perf/core-book3s.c | 19 ++++++++++++++----- > 1 file changed, 14 insertions(+), 5 deletions(-) > > diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c > index 3dcfecf858f3..36870569bf9c 100644 > --- a/arch/powerpc/perf/core-book3s.c > +++ b/arch/powerpc/perf/core-book3s.c > @@ -1515,9 +1515,16 @@ static int power_pmu_add(struct perf_event *event, int ef_flags) > ret = 0; > out: > if (has_branch_stack(event)) { > - power_pmu_bhrb_enable(event); > - cpuhw->bhrb_filter = ppmu->bhrb_filter_map( > - event->attr.branch_sample_type); > + u64 bhrb_filter = -1; > + > + if (ppmu->bhrb_filter_map) > + bhrb_filter = ppmu->bhrb_filter_map( > + event->attr.branch_sample_type); > + > + if (bhrb_filter != -1) { > + cpuhw->bhrb_filter = bhrb_filter; > + power_pmu_bhrb_enable(event); /* Does bhrb_users++ */ > + } > } > > perf_pmu_enable(event->pmu); > @@ -1839,7 +1846,6 @@ static int power_pmu_event_init(struct perf_event *event) > int n; > int err; > struct cpu_hw_events *cpuhw; > - u64 bhrb_filter; > > if (!ppmu) > return -ENOENT; > @@ -1945,7 +1951,10 @@ static int power_pmu_event_init(struct perf_event *event) > err = power_check_constraints(cpuhw, events, cflags, n + 1); > > if (has_branch_stack(event)) { > - bhrb_filter = ppmu->bhrb_filter_map( > + u64 bhrb_filter = -1; > + > + if (ppmu->bhrb_filter_map) > + bhrb_filter = ppmu->bhrb_filter_map( > event->attr.branch_sample_type); > > if (bhrb_filter == -1) {
On Tue, 2 Jun 2020 12:56:12 +1000, Alexey Kardashevskiy wrote: > The bhrb_filter_map ("The Branch History Rolling Buffer") callback is > only defined in raw CPUs' power_pmu structs. The "architected" CPUs use > generic_compat_pmu which does not have this callback and crashed occur. > > This add a NULL pointer check for bhrb_filter_map() which behaves as if > the callback returned an error. > > [...] Applied to powerpc/fixes. [1/1] powerpc/perf: Fix crashes with generic_compat_pmu & BHRB https://git.kernel.org/powerpc/c/b460b512417ae9c8b51a3bdcc09020cd6c60ff69 cheers
Hi Maddy, I just noticed that I still have "powerpc/perf: Add checks for reserved values" in my pile (pushed here https://github.com/aik/linux/commit/61e1bc3f2e19d450e2e2d39174d422160b21957b ), do we still need it? The lockups I saw were fixed by https://github.com/aik/linux/commit/17899eaf88d689 but it is hardly a replacement. Thanks, On 04/06/2020 02:34, Madhavan Srinivasan wrote: > > > On 6/2/20 8:26 AM, Alexey Kardashevskiy wrote: >> The bhrb_filter_map ("The Branch History Rolling Buffer") callback is >> only defined in raw CPUs' power_pmu structs. The "architected" CPUs use >> generic_compat_pmu which does not have this callback and crashed occur. >> >> This add a NULL pointer check for bhrb_filter_map() which behaves as if >> the callback returned an error. >> >> This does not add the same check for config_bhrb() as the only caller >> checks for cpuhw->bhrb_users which remains zero if bhrb_filter_map==0. > > Changes looks fine. > Reviewed-by: Madhavan Srinivasan <maddy@linux.ibm.com> > > The commit be80e758d0c2e ('powerpc/perf: Add generic compat mode pmu > driver') > which introduced generic_compat_pmu was merged in v5.2. So we need to > CC stable starting from 5.2 :( . My bad, sorry. > > Maddy > >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >> --- >> arch/powerpc/perf/core-book3s.c | 19 ++++++++++++++----- >> 1 file changed, 14 insertions(+), 5 deletions(-) >> >> diff --git a/arch/powerpc/perf/core-book3s.c >> b/arch/powerpc/perf/core-book3s.c >> index 3dcfecf858f3..36870569bf9c 100644 >> --- a/arch/powerpc/perf/core-book3s.c >> +++ b/arch/powerpc/perf/core-book3s.c >> @@ -1515,9 +1515,16 @@ static int power_pmu_add(struct perf_event >> *event, int ef_flags) >> ret = 0; >> out: >> if (has_branch_stack(event)) { >> - power_pmu_bhrb_enable(event); >> - cpuhw->bhrb_filter = ppmu->bhrb_filter_map( >> - event->attr.branch_sample_type); >> + u64 bhrb_filter = -1; >> + >> + if (ppmu->bhrb_filter_map) >> + bhrb_filter = ppmu->bhrb_filter_map( >> + event->attr.branch_sample_type); >> + >> + if (bhrb_filter != -1) { >> + cpuhw->bhrb_filter = bhrb_filter; >> + power_pmu_bhrb_enable(event); /* Does bhrb_users++ */ >> + } >> } >> >> perf_pmu_enable(event->pmu); >> @@ -1839,7 +1846,6 @@ static int power_pmu_event_init(struct >> perf_event *event) >> int n; >> int err; >> struct cpu_hw_events *cpuhw; >> - u64 bhrb_filter; >> >> if (!ppmu) >> return -ENOENT; >> @@ -1945,7 +1951,10 @@ static int power_pmu_event_init(struct >> perf_event *event) >> err = power_check_constraints(cpuhw, events, cflags, n + 1); >> >> if (has_branch_stack(event)) { >> - bhrb_filter = ppmu->bhrb_filter_map( >> + u64 bhrb_filter = -1; >> + >> + if (ppmu->bhrb_filter_map) >> + bhrb_filter = ppmu->bhrb_filter_map( >> event->attr.branch_sample_type); >> >> if (bhrb_filter == -1) { >
On 12/2/20 8:31 AM, Alexey Kardashevskiy wrote: > Hi Maddy, > > I just noticed that I still have "powerpc/perf: Add checks for > reserved values" in my pile (pushed here > https://github.com/aik/linux/commit/61e1bc3f2e19d450e2e2d39174d422160b21957b > ), do we still need it? The lockups I saw were fixed by > https://github.com/aik/linux/commit/17899eaf88d689 but it is hardly a > replacement. Thanks, sorry missed this. Will look at this again. Since we will need generation specific checks for the reserve field. Maddy > > > On 04/06/2020 02:34, Madhavan Srinivasan wrote: >> >> >> On 6/2/20 8:26 AM, Alexey Kardashevskiy wrote: >>> The bhrb_filter_map ("The Branch History Rolling Buffer") >>> callback is >>> only defined in raw CPUs' power_pmu structs. The "architected" CPUs use >>> generic_compat_pmu which does not have this callback and crashed occur. >>> >>> This add a NULL pointer check for bhrb_filter_map() which behaves as if >>> the callback returned an error. >>> >>> This does not add the same check for config_bhrb() as the only caller >>> checks for cpuhw->bhrb_users which remains zero if bhrb_filter_map==0. >> >> Changes looks fine. >> Reviewed-by: Madhavan Srinivasan <maddy@linux.ibm.com> >> >> The commit be80e758d0c2e ('powerpc/perf: Add generic compat mode pmu >> driver') >> which introduced generic_compat_pmu was merged in v5.2. So we need to >> CC stable starting from 5.2 :( . My bad, sorry. >> >> Maddy >> >>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >>> --- >>> arch/powerpc/perf/core-book3s.c | 19 ++++++++++++++----- >>> 1 file changed, 14 insertions(+), 5 deletions(-) >>> >>> diff --git a/arch/powerpc/perf/core-book3s.c >>> b/arch/powerpc/perf/core-book3s.c >>> index 3dcfecf858f3..36870569bf9c 100644 >>> --- a/arch/powerpc/perf/core-book3s.c >>> +++ b/arch/powerpc/perf/core-book3s.c >>> @@ -1515,9 +1515,16 @@ static int power_pmu_add(struct perf_event >>> *event, int ef_flags) >>> ret = 0; >>> out: >>> if (has_branch_stack(event)) { >>> - power_pmu_bhrb_enable(event); >>> - cpuhw->bhrb_filter = ppmu->bhrb_filter_map( >>> - event->attr.branch_sample_type); >>> + u64 bhrb_filter = -1; >>> + >>> + if (ppmu->bhrb_filter_map) >>> + bhrb_filter = ppmu->bhrb_filter_map( >>> + event->attr.branch_sample_type); >>> + >>> + if (bhrb_filter != -1) { >>> + cpuhw->bhrb_filter = bhrb_filter; >>> + power_pmu_bhrb_enable(event); /* Does bhrb_users++ */ >>> + } >>> } >>> >>> perf_pmu_enable(event->pmu); >>> @@ -1839,7 +1846,6 @@ static int power_pmu_event_init(struct >>> perf_event *event) >>> int n; >>> int err; >>> struct cpu_hw_events *cpuhw; >>> - u64 bhrb_filter; >>> >>> if (!ppmu) >>> return -ENOENT; >>> @@ -1945,7 +1951,10 @@ static int power_pmu_event_init(struct >>> perf_event *event) >>> err = power_check_constraints(cpuhw, events, cflags, n + 1); >>> >>> if (has_branch_stack(event)) { >>> - bhrb_filter = ppmu->bhrb_filter_map( >>> + u64 bhrb_filter = -1; >>> + >>> + if (ppmu->bhrb_filter_map) >>> + bhrb_filter = ppmu->bhrb_filter_map( >>> event->attr.branch_sample_type); >>> >>> if (bhrb_filter == -1) { >> >
On 03/12/2020 16:27, Madhavan Srinivasan wrote: > > On 12/2/20 8:31 AM, Alexey Kardashevskiy wrote: >> Hi Maddy, >> >> I just noticed that I still have "powerpc/perf: Add checks for >> reserved values" in my pile (pushed here >> https://github.com/aik/linux/commit/61e1bc3f2e19d450e2e2d39174d422160b21957b >> ), do we still need it? The lockups I saw were fixed by >> https://github.com/aik/linux/commit/17899eaf88d689 but it is hardly a >> replacement. Thanks, > > sorry missed this. Will look at this again. Since we will need > generation specific checks for the reserve field. So any luck with this? Cheers, > > Maddy > >> >> >> On 04/06/2020 02:34, Madhavan Srinivasan wrote: >>> >>> >>> On 6/2/20 8:26 AM, Alexey Kardashevskiy wrote: >>>> The bhrb_filter_map ("The Branch History Rolling Buffer") >>>> callback is >>>> only defined in raw CPUs' power_pmu structs. The "architected" CPUs use >>>> generic_compat_pmu which does not have this callback and crashed occur. >>>> >>>> This add a NULL pointer check for bhrb_filter_map() which behaves as if >>>> the callback returned an error. >>>> >>>> This does not add the same check for config_bhrb() as the only caller >>>> checks for cpuhw->bhrb_users which remains zero if bhrb_filter_map==0. >>> >>> Changes looks fine. >>> Reviewed-by: Madhavan Srinivasan <maddy@linux.ibm.com> >>> >>> The commit be80e758d0c2e ('powerpc/perf: Add generic compat mode pmu >>> driver') >>> which introduced generic_compat_pmu was merged in v5.2. So we need to >>> CC stable starting from 5.2 :( . My bad, sorry. >>> >>> Maddy >>> >>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >>>> --- >>>> arch/powerpc/perf/core-book3s.c | 19 ++++++++++++++----- >>>> 1 file changed, 14 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/arch/powerpc/perf/core-book3s.c >>>> b/arch/powerpc/perf/core-book3s.c >>>> index 3dcfecf858f3..36870569bf9c 100644 >>>> --- a/arch/powerpc/perf/core-book3s.c >>>> +++ b/arch/powerpc/perf/core-book3s.c >>>> @@ -1515,9 +1515,16 @@ static int power_pmu_add(struct perf_event >>>> *event, int ef_flags) >>>> ret = 0; >>>> out: >>>> if (has_branch_stack(event)) { >>>> - power_pmu_bhrb_enable(event); >>>> - cpuhw->bhrb_filter = ppmu->bhrb_filter_map( >>>> - event->attr.branch_sample_type); >>>> + u64 bhrb_filter = -1; >>>> + >>>> + if (ppmu->bhrb_filter_map) >>>> + bhrb_filter = ppmu->bhrb_filter_map( >>>> + event->attr.branch_sample_type); >>>> + >>>> + if (bhrb_filter != -1) { >>>> + cpuhw->bhrb_filter = bhrb_filter; >>>> + power_pmu_bhrb_enable(event); /* Does bhrb_users++ */ >>>> + } >>>> } >>>> >>>> perf_pmu_enable(event->pmu); >>>> @@ -1839,7 +1846,6 @@ static int power_pmu_event_init(struct >>>> perf_event *event) >>>> int n; >>>> int err; >>>> struct cpu_hw_events *cpuhw; >>>> - u64 bhrb_filter; >>>> >>>> if (!ppmu) >>>> return -ENOENT; >>>> @@ -1945,7 +1951,10 @@ static int power_pmu_event_init(struct >>>> perf_event *event) >>>> err = power_check_constraints(cpuhw, events, cflags, n + 1); >>>> >>>> if (has_branch_stack(event)) { >>>> - bhrb_filter = ppmu->bhrb_filter_map( >>>> + u64 bhrb_filter = -1; >>>> + >>>> + if (ppmu->bhrb_filter_map) >>>> + bhrb_filter = ppmu->bhrb_filter_map( >>>> event->attr.branch_sample_type); >>>> >>>> if (bhrb_filter == -1) { >>> >>
diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c index 3dcfecf858f3..36870569bf9c 100644 --- a/arch/powerpc/perf/core-book3s.c +++ b/arch/powerpc/perf/core-book3s.c @@ -1515,9 +1515,16 @@ static int power_pmu_add(struct perf_event *event, int ef_flags) ret = 0; out: if (has_branch_stack(event)) { - power_pmu_bhrb_enable(event); - cpuhw->bhrb_filter = ppmu->bhrb_filter_map( - event->attr.branch_sample_type); + u64 bhrb_filter = -1; + + if (ppmu->bhrb_filter_map) + bhrb_filter = ppmu->bhrb_filter_map( + event->attr.branch_sample_type); + + if (bhrb_filter != -1) { + cpuhw->bhrb_filter = bhrb_filter; + power_pmu_bhrb_enable(event); /* Does bhrb_users++ */ + } } perf_pmu_enable(event->pmu); @@ -1839,7 +1846,6 @@ static int power_pmu_event_init(struct perf_event *event) int n; int err; struct cpu_hw_events *cpuhw; - u64 bhrb_filter; if (!ppmu) return -ENOENT; @@ -1945,7 +1951,10 @@ static int power_pmu_event_init(struct perf_event *event) err = power_check_constraints(cpuhw, events, cflags, n + 1); if (has_branch_stack(event)) { - bhrb_filter = ppmu->bhrb_filter_map( + u64 bhrb_filter = -1; + + if (ppmu->bhrb_filter_map) + bhrb_filter = ppmu->bhrb_filter_map( event->attr.branch_sample_type); if (bhrb_filter == -1) {
The bhrb_filter_map ("The Branch History Rolling Buffer") callback is only defined in raw CPUs' power_pmu structs. The "architected" CPUs use generic_compat_pmu which does not have this callback and crashed occur. This add a NULL pointer check for bhrb_filter_map() which behaves as if the callback returned an error. This does not add the same check for config_bhrb() as the only caller checks for cpuhw->bhrb_users which remains zero if bhrb_filter_map==0. Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- arch/powerpc/perf/core-book3s.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)