[PATCH kernel] powerpc/perf: Stop crashing with generic_compat_pmu

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

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

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:

quoted

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

quoted

Signed-off-by: Alexey Kardashevskiy <redacted>
---
  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

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:

quoted

On 6/2/20 8:26 AM, Alexey Kardashevskiy wrote:

quoted

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

quoted

Signed-off-by: Alexey Kardashevskiy <redacted>
---
  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:

quoted

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

quoted

On 04/06/2020 02:34, Madhavan Srinivasan wrote:

quoted

On 6/2/20 8:26 AM, Alexey Kardashevskiy wrote:

quoted

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

quoted

Signed-off-by: Alexey Kardashevskiy <redacted>
---
  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