diff mbox series

[kernel] powerpc/perf: Stop crashing with generic_compat_pmu

Message ID 20200602025612.62707-1-aik@ozlabs.ru
State Accepted
Commit b460b512417ae9c8b51a3bdcc09020cd6c60ff69
Headers show
Series [kernel] powerpc/perf: Stop crashing with generic_compat_pmu | expand

Checks

Context Check Description
snowpatch_ozlabs/needsstable success Patch has no Fixes tags
snowpatch_ozlabs/checkpatch warning total: 0 errors, 0 warnings, 2 checks, 37 lines checked
snowpatch_ozlabs/build-pmac32 success Build succeeded
snowpatch_ozlabs/build-ppc64e success Build succeeded
snowpatch_ozlabs/build-ppc64be success Build succeeded
snowpatch_ozlabs/build-ppc64le success Build succeeded
snowpatch_ozlabs/apply_patch success Successfully applied on branch powerpc/merge (00ec79b0b767994422c43792d73ff1327714a73f)

Commit Message

Alexey Kardashevskiy June 2, 2020, 2:56 a.m. UTC
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(-)

Comments

Madhavan Srinivasan June 3, 2020, 4:34 p.m. UTC | #1
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) {
Michael Ellerman Aug. 27, 2020, 7:46 a.m. UTC | #2
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
Alexey Kardashevskiy Dec. 2, 2020, 3:01 a.m. UTC | #3
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) {
>
Madhavan Srinivasan Dec. 3, 2020, 5:27 a.m. UTC | #4
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) {
>>
>
Alexey Kardashevskiy Feb. 16, 2021, 1:06 a.m. UTC | #5
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 mbox series

Patch

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) {