All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] perf ioctl: Add check for the sample_period value
@ 2019-05-11  2:42 ` Ravi Bangoria
  0 siblings, 0 replies; 24+ messages in thread
From: Ravi Bangoria @ 2019-05-11  2:42 UTC (permalink / raw)
  To: peterz, jolsa, mpe, maddy; +Cc: acme, linux-kernel, linuxppc-dev, Ravi Bangoria

Add a check for sample_period value sent from userspace. Negative
value does not make sense. And in powerpc arch code this could cause
a recursive PMI leading to a hang (reported when running perf-fuzzer).

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
---
 kernel/events/core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index abbd4b3b96c2..e44c90378940 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -5005,6 +5005,9 @@ static int perf_event_period(struct perf_event *event, u64 __user *arg)
 	if (perf_event_check_period(event, value))
 		return -EINVAL;
 
+	if (!event->attr.freq && (value & (1ULL << 63)))
+		return -EINVAL;
+
 	event_function_call(event, __perf_event_period, &value);
 
 	return 0;
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 1/2] perf ioctl: Add check for the sample_period value
@ 2019-05-11  2:42 ` Ravi Bangoria
  0 siblings, 0 replies; 24+ messages in thread
From: Ravi Bangoria @ 2019-05-11  2:42 UTC (permalink / raw)
  To: peterz, jolsa, mpe, maddy; +Cc: Ravi Bangoria, linuxppc-dev, linux-kernel, acme

Add a check for sample_period value sent from userspace. Negative
value does not make sense. And in powerpc arch code this could cause
a recursive PMI leading to a hang (reported when running perf-fuzzer).

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
---
 kernel/events/core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index abbd4b3b96c2..e44c90378940 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -5005,6 +5005,9 @@ static int perf_event_period(struct perf_event *event, u64 __user *arg)
 	if (perf_event_check_period(event, value))
 		return -EINVAL;
 
+	if (!event->attr.freq && (value & (1ULL << 63)))
+		return -EINVAL;
+
 	event_function_call(event, __perf_event_period, &value);
 
 	return 0;
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 2/2] powerpc/perf: Fix mmcra corruption by bhrb_filter
  2019-05-11  2:42 ` Ravi Bangoria
@ 2019-05-11  2:42   ` Ravi Bangoria
  -1 siblings, 0 replies; 24+ messages in thread
From: Ravi Bangoria @ 2019-05-11  2:42 UTC (permalink / raw)
  To: peterz, jolsa, mpe, maddy; +Cc: acme, linux-kernel, linuxppc-dev, Ravi Bangoria

Consider a scenario where user creates two events:

  1st event:
    attr.sample_type |= PERF_SAMPLE_BRANCH_STACK;
    attr.branch_sample_type = PERF_SAMPLE_BRANCH_ANY;
    fd = perf_event_open(attr, 0, 1, -1, 0);

  This sets cpuhw->bhrb_filter to 0 and returns valid fd.

  2nd event:
    attr.sample_type |= PERF_SAMPLE_BRANCH_STACK;
    attr.branch_sample_type = PERF_SAMPLE_BRANCH_CALL;
    fd = perf_event_open(attr, 0, 1, -1, 0);

  It overrides cpuhw->bhrb_filter to -1 and returns with error.

Now if power_pmu_enable() gets called by any path other than
power_pmu_add(), ppmu->config_bhrb(-1) will set mmcra to -1.

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
---
 arch/powerpc/perf/core-book3s.c | 6 ++++--
 arch/powerpc/perf/power8-pmu.c  | 3 +++
 arch/powerpc/perf/power9-pmu.c  | 3 +++
 3 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
index b0723002a396..8eb5dc5df62b 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c
@@ -1846,6 +1846,7 @@ 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;
@@ -1951,13 +1952,14 @@ static int power_pmu_event_init(struct perf_event *event)
 	err = power_check_constraints(cpuhw, events, cflags, n + 1);
 
 	if (has_branch_stack(event)) {
-		cpuhw->bhrb_filter = ppmu->bhrb_filter_map(
+		bhrb_filter = ppmu->bhrb_filter_map(
 					event->attr.branch_sample_type);
 
-		if (cpuhw->bhrb_filter == -1) {
+		if (bhrb_filter == -1) {
 			put_cpu_var(cpu_hw_events);
 			return -EOPNOTSUPP;
 		}
+		cpuhw->bhrb_filter = bhrb_filter;
 	}
 
 	put_cpu_var(cpu_hw_events);
diff --git a/arch/powerpc/perf/power8-pmu.c b/arch/powerpc/perf/power8-pmu.c
index d12a2db26353..d10feef93b6b 100644
--- a/arch/powerpc/perf/power8-pmu.c
+++ b/arch/powerpc/perf/power8-pmu.c
@@ -29,6 +29,7 @@ enum {
 #define	POWER8_MMCRA_IFM1		0x0000000040000000UL
 #define	POWER8_MMCRA_IFM2		0x0000000080000000UL
 #define	POWER8_MMCRA_IFM3		0x00000000C0000000UL
+#define	POWER8_MMCRA_BHRB_MASK		0x00000000C0000000UL
 
 /*
  * Raw event encoding for PowerISA v2.07 (Power8):
@@ -243,6 +244,8 @@ static u64 power8_bhrb_filter_map(u64 branch_sample_type)
 
 static void power8_config_bhrb(u64 pmu_bhrb_filter)
 {
+	pmu_bhrb_filter &= POWER8_MMCRA_BHRB_MASK;
+
 	/* Enable BHRB filter in PMU */
 	mtspr(SPRN_MMCRA, (mfspr(SPRN_MMCRA) | pmu_bhrb_filter));
 }
diff --git a/arch/powerpc/perf/power9-pmu.c b/arch/powerpc/perf/power9-pmu.c
index 030544e35959..f3987915cadc 100644
--- a/arch/powerpc/perf/power9-pmu.c
+++ b/arch/powerpc/perf/power9-pmu.c
@@ -92,6 +92,7 @@ enum {
 #define POWER9_MMCRA_IFM1		0x0000000040000000UL
 #define POWER9_MMCRA_IFM2		0x0000000080000000UL
 #define POWER9_MMCRA_IFM3		0x00000000C0000000UL
+#define POWER9_MMCRA_BHRB_MASK		0x00000000C0000000UL
 
 /* Nasty Power9 specific hack */
 #define PVR_POWER9_CUMULUS		0x00002000
@@ -300,6 +301,8 @@ static u64 power9_bhrb_filter_map(u64 branch_sample_type)
 
 static void power9_config_bhrb(u64 pmu_bhrb_filter)
 {
+	pmu_bhrb_filter &= POWER9_MMCRA_BHRB_MASK;
+
 	/* Enable BHRB filter in PMU */
 	mtspr(SPRN_MMCRA, (mfspr(SPRN_MMCRA) | pmu_bhrb_filter));
 }
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 2/2] powerpc/perf: Fix mmcra corruption by bhrb_filter
@ 2019-05-11  2:42   ` Ravi Bangoria
  0 siblings, 0 replies; 24+ messages in thread
From: Ravi Bangoria @ 2019-05-11  2:42 UTC (permalink / raw)
  To: peterz, jolsa, mpe, maddy; +Cc: Ravi Bangoria, linuxppc-dev, linux-kernel, acme

Consider a scenario where user creates two events:

  1st event:
    attr.sample_type |= PERF_SAMPLE_BRANCH_STACK;
    attr.branch_sample_type = PERF_SAMPLE_BRANCH_ANY;
    fd = perf_event_open(attr, 0, 1, -1, 0);

  This sets cpuhw->bhrb_filter to 0 and returns valid fd.

  2nd event:
    attr.sample_type |= PERF_SAMPLE_BRANCH_STACK;
    attr.branch_sample_type = PERF_SAMPLE_BRANCH_CALL;
    fd = perf_event_open(attr, 0, 1, -1, 0);

  It overrides cpuhw->bhrb_filter to -1 and returns with error.

Now if power_pmu_enable() gets called by any path other than
power_pmu_add(), ppmu->config_bhrb(-1) will set mmcra to -1.

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
---
 arch/powerpc/perf/core-book3s.c | 6 ++++--
 arch/powerpc/perf/power8-pmu.c  | 3 +++
 arch/powerpc/perf/power9-pmu.c  | 3 +++
 3 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
index b0723002a396..8eb5dc5df62b 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c
@@ -1846,6 +1846,7 @@ 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;
@@ -1951,13 +1952,14 @@ static int power_pmu_event_init(struct perf_event *event)
 	err = power_check_constraints(cpuhw, events, cflags, n + 1);
 
 	if (has_branch_stack(event)) {
-		cpuhw->bhrb_filter = ppmu->bhrb_filter_map(
+		bhrb_filter = ppmu->bhrb_filter_map(
 					event->attr.branch_sample_type);
 
-		if (cpuhw->bhrb_filter == -1) {
+		if (bhrb_filter == -1) {
 			put_cpu_var(cpu_hw_events);
 			return -EOPNOTSUPP;
 		}
+		cpuhw->bhrb_filter = bhrb_filter;
 	}
 
 	put_cpu_var(cpu_hw_events);
diff --git a/arch/powerpc/perf/power8-pmu.c b/arch/powerpc/perf/power8-pmu.c
index d12a2db26353..d10feef93b6b 100644
--- a/arch/powerpc/perf/power8-pmu.c
+++ b/arch/powerpc/perf/power8-pmu.c
@@ -29,6 +29,7 @@ enum {
 #define	POWER8_MMCRA_IFM1		0x0000000040000000UL
 #define	POWER8_MMCRA_IFM2		0x0000000080000000UL
 #define	POWER8_MMCRA_IFM3		0x00000000C0000000UL
+#define	POWER8_MMCRA_BHRB_MASK		0x00000000C0000000UL
 
 /*
  * Raw event encoding for PowerISA v2.07 (Power8):
@@ -243,6 +244,8 @@ static u64 power8_bhrb_filter_map(u64 branch_sample_type)
 
 static void power8_config_bhrb(u64 pmu_bhrb_filter)
 {
+	pmu_bhrb_filter &= POWER8_MMCRA_BHRB_MASK;
+
 	/* Enable BHRB filter in PMU */
 	mtspr(SPRN_MMCRA, (mfspr(SPRN_MMCRA) | pmu_bhrb_filter));
 }
diff --git a/arch/powerpc/perf/power9-pmu.c b/arch/powerpc/perf/power9-pmu.c
index 030544e35959..f3987915cadc 100644
--- a/arch/powerpc/perf/power9-pmu.c
+++ b/arch/powerpc/perf/power9-pmu.c
@@ -92,6 +92,7 @@ enum {
 #define POWER9_MMCRA_IFM1		0x0000000040000000UL
 #define POWER9_MMCRA_IFM2		0x0000000080000000UL
 #define POWER9_MMCRA_IFM3		0x00000000C0000000UL
+#define POWER9_MMCRA_BHRB_MASK		0x00000000C0000000UL
 
 /* Nasty Power9 specific hack */
 #define PVR_POWER9_CUMULUS		0x00002000
@@ -300,6 +301,8 @@ static u64 power9_bhrb_filter_map(u64 branch_sample_type)
 
 static void power9_config_bhrb(u64 pmu_bhrb_filter)
 {
+	pmu_bhrb_filter &= POWER9_MMCRA_BHRB_MASK;
+
 	/* Enable BHRB filter in PMU */
 	mtspr(SPRN_MMCRA, (mfspr(SPRN_MMCRA) | pmu_bhrb_filter));
 }
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH 2/2] powerpc/perf: Fix mmcra corruption by bhrb_filter
  2019-05-11  2:42   ` Ravi Bangoria
@ 2019-05-11  2:47     ` Ravi Bangoria
  -1 siblings, 0 replies; 24+ messages in thread
From: Ravi Bangoria @ 2019-05-11  2:47 UTC (permalink / raw)
  To: peterz, jolsa, mpe, maddy; +Cc: acme, linux-kernel, linuxppc-dev, Ravi Bangoria



On 5/11/19 8:12 AM, Ravi Bangoria wrote:
> Consider a scenario where user creates two events:
> 
>   1st event:
>     attr.sample_type |= PERF_SAMPLE_BRANCH_STACK;
>     attr.branch_sample_type = PERF_SAMPLE_BRANCH_ANY;
>     fd = perf_event_open(attr, 0, 1, -1, 0);
> 
>   This sets cpuhw->bhrb_filter to 0 and returns valid fd.
> 
>   2nd event:
>     attr.sample_type |= PERF_SAMPLE_BRANCH_STACK;
>     attr.branch_sample_type = PERF_SAMPLE_BRANCH_CALL;
>     fd = perf_event_open(attr, 0, 1, -1, 0);
> 
>   It overrides cpuhw->bhrb_filter to -1 and returns with error.
> 
> Now if power_pmu_enable() gets called by any path other than
> power_pmu_add(), ppmu->config_bhrb(-1) will set mmcra to -1.
> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>

Fixes: 3925f46bb590 ("powerpc/perf: Enable branch stack sampling framework")


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 2/2] powerpc/perf: Fix mmcra corruption by bhrb_filter
@ 2019-05-11  2:47     ` Ravi Bangoria
  0 siblings, 0 replies; 24+ messages in thread
From: Ravi Bangoria @ 2019-05-11  2:47 UTC (permalink / raw)
  To: peterz, jolsa, mpe, maddy; +Cc: Ravi Bangoria, linuxppc-dev, linux-kernel, acme



On 5/11/19 8:12 AM, Ravi Bangoria wrote:
> Consider a scenario where user creates two events:
> 
>   1st event:
>     attr.sample_type |= PERF_SAMPLE_BRANCH_STACK;
>     attr.branch_sample_type = PERF_SAMPLE_BRANCH_ANY;
>     fd = perf_event_open(attr, 0, 1, -1, 0);
> 
>   This sets cpuhw->bhrb_filter to 0 and returns valid fd.
> 
>   2nd event:
>     attr.sample_type |= PERF_SAMPLE_BRANCH_STACK;
>     attr.branch_sample_type = PERF_SAMPLE_BRANCH_CALL;
>     fd = perf_event_open(attr, 0, 1, -1, 0);
> 
>   It overrides cpuhw->bhrb_filter to -1 and returns with error.
> 
> Now if power_pmu_enable() gets called by any path other than
> power_pmu_add(), ppmu->config_bhrb(-1) will set mmcra to -1.
> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>

Fixes: 3925f46bb590 ("powerpc/perf: Enable branch stack sampling framework")


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/2] perf ioctl: Add check for the sample_period value
  2019-05-11  2:42 ` Ravi Bangoria
@ 2019-05-13  7:42   ` Peter Zijlstra
  -1 siblings, 0 replies; 24+ messages in thread
From: Peter Zijlstra @ 2019-05-13  7:42 UTC (permalink / raw)
  To: Ravi Bangoria; +Cc: jolsa, mpe, maddy, acme, linux-kernel, linuxppc-dev

On Sat, May 11, 2019 at 08:12:16AM +0530, Ravi Bangoria wrote:
> Add a check for sample_period value sent from userspace. Negative
> value does not make sense. And in powerpc arch code this could cause
> a recursive PMI leading to a hang (reported when running perf-fuzzer).
> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> ---
>  kernel/events/core.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index abbd4b3b96c2..e44c90378940 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -5005,6 +5005,9 @@ static int perf_event_period(struct perf_event *event, u64 __user *arg)
>  	if (perf_event_check_period(event, value))
>  		return -EINVAL;
>  
> +	if (!event->attr.freq && (value & (1ULL << 63)))
> +		return -EINVAL;

Well, perf_event_attr::sample_period is __u64. Would not be the site
using it as signed be the one in error?

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/2] perf ioctl: Add check for the sample_period value
@ 2019-05-13  7:42   ` Peter Zijlstra
  0 siblings, 0 replies; 24+ messages in thread
From: Peter Zijlstra @ 2019-05-13  7:42 UTC (permalink / raw)
  To: Ravi Bangoria; +Cc: maddy, linuxppc-dev, linux-kernel, acme, jolsa

On Sat, May 11, 2019 at 08:12:16AM +0530, Ravi Bangoria wrote:
> Add a check for sample_period value sent from userspace. Negative
> value does not make sense. And in powerpc arch code this could cause
> a recursive PMI leading to a hang (reported when running perf-fuzzer).
> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> ---
>  kernel/events/core.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index abbd4b3b96c2..e44c90378940 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -5005,6 +5005,9 @@ static int perf_event_period(struct perf_event *event, u64 __user *arg)
>  	if (perf_event_check_period(event, value))
>  		return -EINVAL;
>  
> +	if (!event->attr.freq && (value & (1ULL << 63)))
> +		return -EINVAL;

Well, perf_event_attr::sample_period is __u64. Would not be the site
using it as signed be the one in error?

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/2] perf ioctl: Add check for the sample_period value
  2019-05-13  7:42   ` Peter Zijlstra
@ 2019-05-13  8:56     ` Peter Zijlstra
  -1 siblings, 0 replies; 24+ messages in thread
From: Peter Zijlstra @ 2019-05-13  8:56 UTC (permalink / raw)
  To: Ravi Bangoria; +Cc: jolsa, mpe, maddy, acme, linux-kernel, linuxppc-dev

On Mon, May 13, 2019 at 09:42:13AM +0200, Peter Zijlstra wrote:
> On Sat, May 11, 2019 at 08:12:16AM +0530, Ravi Bangoria wrote:
> > Add a check for sample_period value sent from userspace. Negative
> > value does not make sense. And in powerpc arch code this could cause
> > a recursive PMI leading to a hang (reported when running perf-fuzzer).
> > 
> > Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> > ---
> >  kernel/events/core.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > index abbd4b3b96c2..e44c90378940 100644
> > --- a/kernel/events/core.c
> > +++ b/kernel/events/core.c
> > @@ -5005,6 +5005,9 @@ static int perf_event_period(struct perf_event *event, u64 __user *arg)
> >  	if (perf_event_check_period(event, value))
> >  		return -EINVAL;
> >  
> > +	if (!event->attr.freq && (value & (1ULL << 63)))
> > +		return -EINVAL;
> 
> Well, perf_event_attr::sample_period is __u64. Would not be the site
> using it as signed be the one in error?

You forgot to mention commit: 0819b2e30ccb9, so I guess this just makes
it consistent and is fine.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/2] perf ioctl: Add check for the sample_period value
@ 2019-05-13  8:56     ` Peter Zijlstra
  0 siblings, 0 replies; 24+ messages in thread
From: Peter Zijlstra @ 2019-05-13  8:56 UTC (permalink / raw)
  To: Ravi Bangoria; +Cc: maddy, linuxppc-dev, linux-kernel, acme, jolsa

On Mon, May 13, 2019 at 09:42:13AM +0200, Peter Zijlstra wrote:
> On Sat, May 11, 2019 at 08:12:16AM +0530, Ravi Bangoria wrote:
> > Add a check for sample_period value sent from userspace. Negative
> > value does not make sense. And in powerpc arch code this could cause
> > a recursive PMI leading to a hang (reported when running perf-fuzzer).
> > 
> > Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> > ---
> >  kernel/events/core.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > index abbd4b3b96c2..e44c90378940 100644
> > --- a/kernel/events/core.c
> > +++ b/kernel/events/core.c
> > @@ -5005,6 +5005,9 @@ static int perf_event_period(struct perf_event *event, u64 __user *arg)
> >  	if (perf_event_check_period(event, value))
> >  		return -EINVAL;
> >  
> > +	if (!event->attr.freq && (value & (1ULL << 63)))
> > +		return -EINVAL;
> 
> Well, perf_event_attr::sample_period is __u64. Would not be the site
> using it as signed be the one in error?

You forgot to mention commit: 0819b2e30ccb9, so I guess this just makes
it consistent and is fine.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/2] perf ioctl: Add check for the sample_period value
  2019-05-13  8:56     ` Peter Zijlstra
@ 2019-05-13 10:07       ` Ravi Bangoria
  -1 siblings, 0 replies; 24+ messages in thread
From: Ravi Bangoria @ 2019-05-13 10:07 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: jolsa, mpe, maddy, acme, linux-kernel, linuxppc-dev, Ravi Bangoria



On 5/13/19 2:26 PM, Peter Zijlstra wrote:
> On Mon, May 13, 2019 at 09:42:13AM +0200, Peter Zijlstra wrote:
>> On Sat, May 11, 2019 at 08:12:16AM +0530, Ravi Bangoria wrote:
>>> Add a check for sample_period value sent from userspace. Negative
>>> value does not make sense. And in powerpc arch code this could cause
>>> a recursive PMI leading to a hang (reported when running perf-fuzzer).
>>>
>>> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
>>> ---
>>>  kernel/events/core.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/kernel/events/core.c b/kernel/events/core.c
>>> index abbd4b3b96c2..e44c90378940 100644
>>> --- a/kernel/events/core.c
>>> +++ b/kernel/events/core.c
>>> @@ -5005,6 +5005,9 @@ static int perf_event_period(struct perf_event *event, u64 __user *arg)
>>>  	if (perf_event_check_period(event, value))
>>>  		return -EINVAL;
>>>  
>>> +	if (!event->attr.freq && (value & (1ULL << 63)))
>>> +		return -EINVAL;
>>
>> Well, perf_event_attr::sample_period is __u64. Would not be the site
>> using it as signed be the one in error?
> 
> You forgot to mention commit: 0819b2e30ccb9, so I guess this just makes
> it consistent and is fine.
> 

Yeah, I was about to reply :)


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/2] perf ioctl: Add check for the sample_period value
@ 2019-05-13 10:07       ` Ravi Bangoria
  0 siblings, 0 replies; 24+ messages in thread
From: Ravi Bangoria @ 2019-05-13 10:07 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Ravi Bangoria, maddy, linuxppc-dev, linux-kernel, acme, jolsa



On 5/13/19 2:26 PM, Peter Zijlstra wrote:
> On Mon, May 13, 2019 at 09:42:13AM +0200, Peter Zijlstra wrote:
>> On Sat, May 11, 2019 at 08:12:16AM +0530, Ravi Bangoria wrote:
>>> Add a check for sample_period value sent from userspace. Negative
>>> value does not make sense. And in powerpc arch code this could cause
>>> a recursive PMI leading to a hang (reported when running perf-fuzzer).
>>>
>>> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
>>> ---
>>>  kernel/events/core.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/kernel/events/core.c b/kernel/events/core.c
>>> index abbd4b3b96c2..e44c90378940 100644
>>> --- a/kernel/events/core.c
>>> +++ b/kernel/events/core.c
>>> @@ -5005,6 +5005,9 @@ static int perf_event_period(struct perf_event *event, u64 __user *arg)
>>>  	if (perf_event_check_period(event, value))
>>>  		return -EINVAL;
>>>  
>>> +	if (!event->attr.freq && (value & (1ULL << 63)))
>>> +		return -EINVAL;
>>
>> Well, perf_event_attr::sample_period is __u64. Would not be the site
>> using it as signed be the one in error?
> 
> You forgot to mention commit: 0819b2e30ccb9, so I guess this just makes
> it consistent and is fine.
> 

Yeah, I was about to reply :)


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 2/2] powerpc/perf: Fix mmcra corruption by bhrb_filter
  2019-05-11  2:42   ` Ravi Bangoria
@ 2019-05-22  5:01     ` Madhavan Srinivasan
  -1 siblings, 0 replies; 24+ messages in thread
From: Madhavan Srinivasan @ 2019-05-22  5:01 UTC (permalink / raw)
  To: Ravi Bangoria, peterz, jolsa, mpe; +Cc: acme, linux-kernel, linuxppc-dev


On 11/05/19 8:12 AM, Ravi Bangoria wrote:
> Consider a scenario where user creates two events:
>
>    1st event:
>      attr.sample_type |= PERF_SAMPLE_BRANCH_STACK;
>      attr.branch_sample_type = PERF_SAMPLE_BRANCH_ANY;
>      fd = perf_event_open(attr, 0, 1, -1, 0);
>
>    This sets cpuhw->bhrb_filter to 0 and returns valid fd.
>
>    2nd event:
>      attr.sample_type |= PERF_SAMPLE_BRANCH_STACK;
>      attr.branch_sample_type = PERF_SAMPLE_BRANCH_CALL;
>      fd = perf_event_open(attr, 0, 1, -1, 0);
>
>    It overrides cpuhw->bhrb_filter to -1 and returns with error.
>
> Now if power_pmu_enable() gets called by any path other than
> power_pmu_add(), ppmu->config_bhrb(-1) will set mmcra to -1.

Reviewed-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>


> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> ---
>   arch/powerpc/perf/core-book3s.c | 6 ++++--
>   arch/powerpc/perf/power8-pmu.c  | 3 +++
>   arch/powerpc/perf/power9-pmu.c  | 3 +++
>   3 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
> index b0723002a396..8eb5dc5df62b 100644
> --- a/arch/powerpc/perf/core-book3s.c
> +++ b/arch/powerpc/perf/core-book3s.c
> @@ -1846,6 +1846,7 @@ 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;
> @@ -1951,13 +1952,14 @@ static int power_pmu_event_init(struct perf_event *event)
>   	err = power_check_constraints(cpuhw, events, cflags, n + 1);
>
>   	if (has_branch_stack(event)) {
> -		cpuhw->bhrb_filter = ppmu->bhrb_filter_map(
> +		bhrb_filter = ppmu->bhrb_filter_map(
>   					event->attr.branch_sample_type);
>
> -		if (cpuhw->bhrb_filter == -1) {
> +		if (bhrb_filter == -1) {
>   			put_cpu_var(cpu_hw_events);
>   			return -EOPNOTSUPP;
>   		}
> +		cpuhw->bhrb_filter = bhrb_filter;
>   	}
>
>   	put_cpu_var(cpu_hw_events);
> diff --git a/arch/powerpc/perf/power8-pmu.c b/arch/powerpc/perf/power8-pmu.c
> index d12a2db26353..d10feef93b6b 100644
> --- a/arch/powerpc/perf/power8-pmu.c
> +++ b/arch/powerpc/perf/power8-pmu.c
> @@ -29,6 +29,7 @@ enum {
>   #define	POWER8_MMCRA_IFM1		0x0000000040000000UL
>   #define	POWER8_MMCRA_IFM2		0x0000000080000000UL
>   #define	POWER8_MMCRA_IFM3		0x00000000C0000000UL
> +#define	POWER8_MMCRA_BHRB_MASK		0x00000000C0000000UL
>
>   /*
>    * Raw event encoding for PowerISA v2.07 (Power8):
> @@ -243,6 +244,8 @@ static u64 power8_bhrb_filter_map(u64 branch_sample_type)
>
>   static void power8_config_bhrb(u64 pmu_bhrb_filter)
>   {
> +	pmu_bhrb_filter &= POWER8_MMCRA_BHRB_MASK;
> +
>   	/* Enable BHRB filter in PMU */
>   	mtspr(SPRN_MMCRA, (mfspr(SPRN_MMCRA) | pmu_bhrb_filter));
>   }
> diff --git a/arch/powerpc/perf/power9-pmu.c b/arch/powerpc/perf/power9-pmu.c
> index 030544e35959..f3987915cadc 100644
> --- a/arch/powerpc/perf/power9-pmu.c
> +++ b/arch/powerpc/perf/power9-pmu.c
> @@ -92,6 +92,7 @@ enum {
>   #define POWER9_MMCRA_IFM1		0x0000000040000000UL
>   #define POWER9_MMCRA_IFM2		0x0000000080000000UL
>   #define POWER9_MMCRA_IFM3		0x00000000C0000000UL
> +#define POWER9_MMCRA_BHRB_MASK		0x00000000C0000000UL
>
>   /* Nasty Power9 specific hack */
>   #define PVR_POWER9_CUMULUS		0x00002000
> @@ -300,6 +301,8 @@ static u64 power9_bhrb_filter_map(u64 branch_sample_type)
>
>   static void power9_config_bhrb(u64 pmu_bhrb_filter)
>   {
> +	pmu_bhrb_filter &= POWER9_MMCRA_BHRB_MASK;
> +
>   	/* Enable BHRB filter in PMU */
>   	mtspr(SPRN_MMCRA, (mfspr(SPRN_MMCRA) | pmu_bhrb_filter));
>   }


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 2/2] powerpc/perf: Fix mmcra corruption by bhrb_filter
@ 2019-05-22  5:01     ` Madhavan Srinivasan
  0 siblings, 0 replies; 24+ messages in thread
From: Madhavan Srinivasan @ 2019-05-22  5:01 UTC (permalink / raw)
  To: Ravi Bangoria, peterz, jolsa, mpe; +Cc: linuxppc-dev, linux-kernel, acme


On 11/05/19 8:12 AM, Ravi Bangoria wrote:
> Consider a scenario where user creates two events:
>
>    1st event:
>      attr.sample_type |= PERF_SAMPLE_BRANCH_STACK;
>      attr.branch_sample_type = PERF_SAMPLE_BRANCH_ANY;
>      fd = perf_event_open(attr, 0, 1, -1, 0);
>
>    This sets cpuhw->bhrb_filter to 0 and returns valid fd.
>
>    2nd event:
>      attr.sample_type |= PERF_SAMPLE_BRANCH_STACK;
>      attr.branch_sample_type = PERF_SAMPLE_BRANCH_CALL;
>      fd = perf_event_open(attr, 0, 1, -1, 0);
>
>    It overrides cpuhw->bhrb_filter to -1 and returns with error.
>
> Now if power_pmu_enable() gets called by any path other than
> power_pmu_add(), ppmu->config_bhrb(-1) will set mmcra to -1.

Reviewed-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>


> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> ---
>   arch/powerpc/perf/core-book3s.c | 6 ++++--
>   arch/powerpc/perf/power8-pmu.c  | 3 +++
>   arch/powerpc/perf/power9-pmu.c  | 3 +++
>   3 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
> index b0723002a396..8eb5dc5df62b 100644
> --- a/arch/powerpc/perf/core-book3s.c
> +++ b/arch/powerpc/perf/core-book3s.c
> @@ -1846,6 +1846,7 @@ 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;
> @@ -1951,13 +1952,14 @@ static int power_pmu_event_init(struct perf_event *event)
>   	err = power_check_constraints(cpuhw, events, cflags, n + 1);
>
>   	if (has_branch_stack(event)) {
> -		cpuhw->bhrb_filter = ppmu->bhrb_filter_map(
> +		bhrb_filter = ppmu->bhrb_filter_map(
>   					event->attr.branch_sample_type);
>
> -		if (cpuhw->bhrb_filter == -1) {
> +		if (bhrb_filter == -1) {
>   			put_cpu_var(cpu_hw_events);
>   			return -EOPNOTSUPP;
>   		}
> +		cpuhw->bhrb_filter = bhrb_filter;
>   	}
>
>   	put_cpu_var(cpu_hw_events);
> diff --git a/arch/powerpc/perf/power8-pmu.c b/arch/powerpc/perf/power8-pmu.c
> index d12a2db26353..d10feef93b6b 100644
> --- a/arch/powerpc/perf/power8-pmu.c
> +++ b/arch/powerpc/perf/power8-pmu.c
> @@ -29,6 +29,7 @@ enum {
>   #define	POWER8_MMCRA_IFM1		0x0000000040000000UL
>   #define	POWER8_MMCRA_IFM2		0x0000000080000000UL
>   #define	POWER8_MMCRA_IFM3		0x00000000C0000000UL
> +#define	POWER8_MMCRA_BHRB_MASK		0x00000000C0000000UL
>
>   /*
>    * Raw event encoding for PowerISA v2.07 (Power8):
> @@ -243,6 +244,8 @@ static u64 power8_bhrb_filter_map(u64 branch_sample_type)
>
>   static void power8_config_bhrb(u64 pmu_bhrb_filter)
>   {
> +	pmu_bhrb_filter &= POWER8_MMCRA_BHRB_MASK;
> +
>   	/* Enable BHRB filter in PMU */
>   	mtspr(SPRN_MMCRA, (mfspr(SPRN_MMCRA) | pmu_bhrb_filter));
>   }
> diff --git a/arch/powerpc/perf/power9-pmu.c b/arch/powerpc/perf/power9-pmu.c
> index 030544e35959..f3987915cadc 100644
> --- a/arch/powerpc/perf/power9-pmu.c
> +++ b/arch/powerpc/perf/power9-pmu.c
> @@ -92,6 +92,7 @@ enum {
>   #define POWER9_MMCRA_IFM1		0x0000000040000000UL
>   #define POWER9_MMCRA_IFM2		0x0000000080000000UL
>   #define POWER9_MMCRA_IFM3		0x00000000C0000000UL
> +#define POWER9_MMCRA_BHRB_MASK		0x00000000C0000000UL
>
>   /* Nasty Power9 specific hack */
>   #define PVR_POWER9_CUMULUS		0x00002000
> @@ -300,6 +301,8 @@ static u64 power9_bhrb_filter_map(u64 branch_sample_type)
>
>   static void power9_config_bhrb(u64 pmu_bhrb_filter)
>   {
> +	pmu_bhrb_filter &= POWER9_MMCRA_BHRB_MASK;
> +
>   	/* Enable BHRB filter in PMU */
>   	mtspr(SPRN_MMCRA, (mfspr(SPRN_MMCRA) | pmu_bhrb_filter));
>   }


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 2/2] powerpc/perf: Fix mmcra corruption by bhrb_filter
  2019-05-11  2:42   ` Ravi Bangoria
                     ` (2 preceding siblings ...)
  (?)
@ 2019-05-25  0:54   ` Michael Ellerman
  -1 siblings, 0 replies; 24+ messages in thread
From: Michael Ellerman @ 2019-05-25  0:54 UTC (permalink / raw)
  To: Ravi Bangoria, peterz, jolsa, maddy
  Cc: Ravi Bangoria, linuxppc-dev, linux-kernel, acme

On Sat, 2019-05-11 at 02:42:17 UTC, Ravi Bangoria wrote:
> Consider a scenario where user creates two events:
> 
>   1st event:
>     attr.sample_type |= PERF_SAMPLE_BRANCH_STACK;
>     attr.branch_sample_type = PERF_SAMPLE_BRANCH_ANY;
>     fd = perf_event_open(attr, 0, 1, -1, 0);
> 
>   This sets cpuhw->bhrb_filter to 0 and returns valid fd.
> 
>   2nd event:
>     attr.sample_type |= PERF_SAMPLE_BRANCH_STACK;
>     attr.branch_sample_type = PERF_SAMPLE_BRANCH_CALL;
>     fd = perf_event_open(attr, 0, 1, -1, 0);
> 
>   It overrides cpuhw->bhrb_filter to -1 and returns with error.
> 
> Now if power_pmu_enable() gets called by any path other than
> power_pmu_add(), ppmu->config_bhrb(-1) will set mmcra to -1.
> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> Reviewed-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>

Applied to powerpc fixes, thanks.

https://git.kernel.org/powerpc/c/3202e35ec1c8fc19cea24253ff83edf7

cheers

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/2] perf ioctl: Add check for the sample_period value
  2019-05-13 10:07       ` Ravi Bangoria
@ 2019-05-28  9:50         ` Michael Ellerman
  -1 siblings, 0 replies; 24+ messages in thread
From: Michael Ellerman @ 2019-05-28  9:50 UTC (permalink / raw)
  To: Ravi Bangoria, Peter Zijlstra
  Cc: jolsa, maddy, acme, linux-kernel, linuxppc-dev, Ravi Bangoria

Ravi Bangoria <ravi.bangoria@linux.ibm.com> writes:
> On 5/13/19 2:26 PM, Peter Zijlstra wrote:
>> On Mon, May 13, 2019 at 09:42:13AM +0200, Peter Zijlstra wrote:
>>> On Sat, May 11, 2019 at 08:12:16AM +0530, Ravi Bangoria wrote:
>>>> Add a check for sample_period value sent from userspace. Negative
>>>> value does not make sense. And in powerpc arch code this could cause
>>>> a recursive PMI leading to a hang (reported when running perf-fuzzer).
>>>>
>>>> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
>>>> ---
>>>>  kernel/events/core.c | 3 +++
>>>>  1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/kernel/events/core.c b/kernel/events/core.c
>>>> index abbd4b3b96c2..e44c90378940 100644
>>>> --- a/kernel/events/core.c
>>>> +++ b/kernel/events/core.c
>>>> @@ -5005,6 +5005,9 @@ static int perf_event_period(struct perf_event *event, u64 __user *arg)
>>>>  	if (perf_event_check_period(event, value))
>>>>  		return -EINVAL;
>>>>  
>>>> +	if (!event->attr.freq && (value & (1ULL << 63)))
>>>> +		return -EINVAL;
>>>
>>> Well, perf_event_attr::sample_period is __u64. Would not be the site
>>> using it as signed be the one in error?
>> 
>> You forgot to mention commit: 0819b2e30ccb9, so I guess this just makes
>> it consistent and is fine.
>> 
>
> Yeah, I was about to reply :)

I've taken patch 2. You should probably do a v2 of patch 1 with an
updated change log that explains things fully?

cheers

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/2] perf ioctl: Add check for the sample_period value
@ 2019-05-28  9:50         ` Michael Ellerman
  0 siblings, 0 replies; 24+ messages in thread
From: Michael Ellerman @ 2019-05-28  9:50 UTC (permalink / raw)
  To: Ravi Bangoria, Peter Zijlstra
  Cc: Ravi Bangoria, maddy, jolsa, linux-kernel, acme, linuxppc-dev

Ravi Bangoria <ravi.bangoria@linux.ibm.com> writes:
> On 5/13/19 2:26 PM, Peter Zijlstra wrote:
>> On Mon, May 13, 2019 at 09:42:13AM +0200, Peter Zijlstra wrote:
>>> On Sat, May 11, 2019 at 08:12:16AM +0530, Ravi Bangoria wrote:
>>>> Add a check for sample_period value sent from userspace. Negative
>>>> value does not make sense. And in powerpc arch code this could cause
>>>> a recursive PMI leading to a hang (reported when running perf-fuzzer).
>>>>
>>>> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
>>>> ---
>>>>  kernel/events/core.c | 3 +++
>>>>  1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/kernel/events/core.c b/kernel/events/core.c
>>>> index abbd4b3b96c2..e44c90378940 100644
>>>> --- a/kernel/events/core.c
>>>> +++ b/kernel/events/core.c
>>>> @@ -5005,6 +5005,9 @@ static int perf_event_period(struct perf_event *event, u64 __user *arg)
>>>>  	if (perf_event_check_period(event, value))
>>>>  		return -EINVAL;
>>>>  
>>>> +	if (!event->attr.freq && (value & (1ULL << 63)))
>>>> +		return -EINVAL;
>>>
>>> Well, perf_event_attr::sample_period is __u64. Would not be the site
>>> using it as signed be the one in error?
>> 
>> You forgot to mention commit: 0819b2e30ccb9, so I guess this just makes
>> it consistent and is fine.
>> 
>
> Yeah, I was about to reply :)

I've taken patch 2. You should probably do a v2 of patch 1 with an
updated change log that explains things fully?

cheers

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v2] perf ioctl: Add check for the sample_period value
  2019-05-28  9:50         ` Michael Ellerman
@ 2019-06-04  4:29           ` Ravi Bangoria
  -1 siblings, 0 replies; 24+ messages in thread
From: Ravi Bangoria @ 2019-06-04  4:29 UTC (permalink / raw)
  To: mpe, peterz; +Cc: jolsa, maddy, acme, linux-kernel, linuxppc-dev, Ravi Bangoria

perf_event_open() limits the sample_period to 63 bits. See
commit 0819b2e30ccb ("perf: Limit perf_event_attr::sample_period
to 63 bits"). Make ioctl() consistent with it.

Also on powerpc, negative sample_period could cause a recursive
PMIs leading to a hang (reported when running perf-fuzzer).

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
---
 kernel/events/core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index abbd4b3b96c2..e44c90378940 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -5005,6 +5005,9 @@ static int perf_event_period(struct perf_event *event, u64 __user *arg)
 	if (perf_event_check_period(event, value))
 		return -EINVAL;
 
+	if (!event->attr.freq && (value & (1ULL << 63)))
+		return -EINVAL;
+
 	event_function_call(event, __perf_event_period, &value);
 
 	return 0;
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v2] perf ioctl: Add check for the sample_period value
@ 2019-06-04  4:29           ` Ravi Bangoria
  0 siblings, 0 replies; 24+ messages in thread
From: Ravi Bangoria @ 2019-06-04  4:29 UTC (permalink / raw)
  To: mpe, peterz; +Cc: Ravi Bangoria, maddy, jolsa, linux-kernel, acme, linuxppc-dev

perf_event_open() limits the sample_period to 63 bits. See
commit 0819b2e30ccb ("perf: Limit perf_event_attr::sample_period
to 63 bits"). Make ioctl() consistent with it.

Also on powerpc, negative sample_period could cause a recursive
PMIs leading to a hang (reported when running perf-fuzzer).

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
---
 kernel/events/core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index abbd4b3b96c2..e44c90378940 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -5005,6 +5005,9 @@ static int perf_event_period(struct perf_event *event, u64 __user *arg)
 	if (perf_event_check_period(event, value))
 		return -EINVAL;
 
+	if (!event->attr.freq && (value & (1ULL << 63)))
+		return -EINVAL;
+
 	event_function_call(event, __perf_event_period, &value);
 
 	return 0;
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] perf ioctl: Add check for the sample_period value
  2019-06-04  4:29           ` Ravi Bangoria
@ 2019-06-17  8:38             ` Ravi Bangoria
  -1 siblings, 0 replies; 24+ messages in thread
From: Ravi Bangoria @ 2019-06-17  8:38 UTC (permalink / raw)
  To: mpe, peterz; +Cc: jolsa, maddy, acme, linux-kernel, linuxppc-dev, Ravi Bangoria

Peter / mpe,

Is the v2 looks good? If so, can anyone of you please pick this up.

On 6/4/19 9:59 AM, Ravi Bangoria wrote:
> perf_event_open() limits the sample_period to 63 bits. See
> commit 0819b2e30ccb ("perf: Limit perf_event_attr::sample_period
> to 63 bits"). Make ioctl() consistent with it.
> 
> Also on powerpc, negative sample_period could cause a recursive
> PMIs leading to a hang (reported when running perf-fuzzer).
> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> ---
>  kernel/events/core.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index abbd4b3b96c2..e44c90378940 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -5005,6 +5005,9 @@ static int perf_event_period(struct perf_event *event, u64 __user *arg)
>  	if (perf_event_check_period(event, value))
>  		return -EINVAL;
>  
> +	if (!event->attr.freq && (value & (1ULL << 63)))
> +		return -EINVAL;
> +
>  	event_function_call(event, __perf_event_period, &value);
>  
>  	return 0;
> 


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] perf ioctl: Add check for the sample_period value
@ 2019-06-17  8:38             ` Ravi Bangoria
  0 siblings, 0 replies; 24+ messages in thread
From: Ravi Bangoria @ 2019-06-17  8:38 UTC (permalink / raw)
  To: mpe, peterz; +Cc: Ravi Bangoria, maddy, jolsa, linux-kernel, acme, linuxppc-dev

Peter / mpe,

Is the v2 looks good? If so, can anyone of you please pick this up.

On 6/4/19 9:59 AM, Ravi Bangoria wrote:
> perf_event_open() limits the sample_period to 63 bits. See
> commit 0819b2e30ccb ("perf: Limit perf_event_attr::sample_period
> to 63 bits"). Make ioctl() consistent with it.
> 
> Also on powerpc, negative sample_period could cause a recursive
> PMIs leading to a hang (reported when running perf-fuzzer).
> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> ---
>  kernel/events/core.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index abbd4b3b96c2..e44c90378940 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -5005,6 +5005,9 @@ static int perf_event_period(struct perf_event *event, u64 __user *arg)
>  	if (perf_event_check_period(event, value))
>  		return -EINVAL;
>  
> +	if (!event->attr.freq && (value & (1ULL << 63)))
> +		return -EINVAL;
> +
>  	event_function_call(event, __perf_event_period, &value);
>  
>  	return 0;
> 


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] perf ioctl: Add check for the sample_period value
  2019-06-17  8:38             ` Ravi Bangoria
@ 2019-06-18 12:28               ` Michael Ellerman
  -1 siblings, 0 replies; 24+ messages in thread
From: Michael Ellerman @ 2019-06-18 12:28 UTC (permalink / raw)
  To: Ravi Bangoria, peterz
  Cc: jolsa, maddy, acme, linux-kernel, linuxppc-dev, Ravi Bangoria

Ravi Bangoria <ravi.bangoria@linux.ibm.com> writes:
> Peter / mpe,
>
> Is the v2 looks good? If so, can anyone of you please pick this up.

I usually wouldn't take it, it's generic perf code. Unless
peter/ingo/acme tell me otherwise.

It's sort of a bug fix for 0819b2e30ccb, should it have a fixes and/or
stable tag?

  Fixes: 0819b2e30ccb ("perf: Limit perf_event_attr::sample_period to 63 bits")
  Cc: stable@vger.kernel.org # v3.15+

cheers

> On 6/4/19 9:59 AM, Ravi Bangoria wrote:
>> perf_event_open() limits the sample_period to 63 bits. See
>> commit 0819b2e30ccb ("perf: Limit perf_event_attr::sample_period
>> to 63 bits"). Make ioctl() consistent with it.
>> 
>> Also on powerpc, negative sample_period could cause a recursive
>> PMIs leading to a hang (reported when running perf-fuzzer).
>> 
>> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
>> ---
>>  kernel/events/core.c | 3 +++
>>  1 file changed, 3 insertions(+)
>> 
>> diff --git a/kernel/events/core.c b/kernel/events/core.c
>> index abbd4b3b96c2..e44c90378940 100644
>> --- a/kernel/events/core.c
>> +++ b/kernel/events/core.c
>> @@ -5005,6 +5005,9 @@ static int perf_event_period(struct perf_event *event, u64 __user *arg)
>>  	if (perf_event_check_period(event, value))
>>  		return -EINVAL;
>>  
>> +	if (!event->attr.freq && (value & (1ULL << 63)))
>> +		return -EINVAL;
>> +
>>  	event_function_call(event, __perf_event_period, &value);
>>  
>>  	return 0;
>> 

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] perf ioctl: Add check for the sample_period value
@ 2019-06-18 12:28               ` Michael Ellerman
  0 siblings, 0 replies; 24+ messages in thread
From: Michael Ellerman @ 2019-06-18 12:28 UTC (permalink / raw)
  To: Ravi Bangoria, peterz
  Cc: Ravi Bangoria, maddy, jolsa, linux-kernel, acme, linuxppc-dev

Ravi Bangoria <ravi.bangoria@linux.ibm.com> writes:
> Peter / mpe,
>
> Is the v2 looks good? If so, can anyone of you please pick this up.

I usually wouldn't take it, it's generic perf code. Unless
peter/ingo/acme tell me otherwise.

It's sort of a bug fix for 0819b2e30ccb, should it have a fixes and/or
stable tag?

  Fixes: 0819b2e30ccb ("perf: Limit perf_event_attr::sample_period to 63 bits")
  Cc: stable@vger.kernel.org # v3.15+

cheers

> On 6/4/19 9:59 AM, Ravi Bangoria wrote:
>> perf_event_open() limits the sample_period to 63 bits. See
>> commit 0819b2e30ccb ("perf: Limit perf_event_attr::sample_period
>> to 63 bits"). Make ioctl() consistent with it.
>> 
>> Also on powerpc, negative sample_period could cause a recursive
>> PMIs leading to a hang (reported when running perf-fuzzer).
>> 
>> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
>> ---
>>  kernel/events/core.c | 3 +++
>>  1 file changed, 3 insertions(+)
>> 
>> diff --git a/kernel/events/core.c b/kernel/events/core.c
>> index abbd4b3b96c2..e44c90378940 100644
>> --- a/kernel/events/core.c
>> +++ b/kernel/events/core.c
>> @@ -5005,6 +5005,9 @@ static int perf_event_period(struct perf_event *event, u64 __user *arg)
>>  	if (perf_event_check_period(event, value))
>>  		return -EINVAL;
>>  
>> +	if (!event->attr.freq && (value & (1ULL << 63)))
>> +		return -EINVAL;
>> +
>>  	event_function_call(event, __perf_event_period, &value);
>>  
>>  	return 0;
>> 

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [tip:perf/urgent] perf/ioctl: Add check for the sample_period value
  2019-06-04  4:29           ` Ravi Bangoria
  (?)
  (?)
@ 2019-06-25  8:19           ` tip-bot for Ravi Bangoria
  -1 siblings, 0 replies; 24+ messages in thread
From: tip-bot for Ravi Bangoria @ 2019-06-25  8:19 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: jolsa, hpa, peterz, alexander.shishkin, eranian, mingo,
	ravi.bangoria, linux-kernel, tglx, vincent.weaver, acme,
	torvalds

Commit-ID:  913a90bc5a3a06b1f04c337320e9aeee2328dd77
Gitweb:     https://git.kernel.org/tip/913a90bc5a3a06b1f04c337320e9aeee2328dd77
Author:     Ravi Bangoria <ravi.bangoria@linux.ibm.com>
AuthorDate: Tue, 4 Jun 2019 09:59:53 +0530
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Mon, 24 Jun 2019 19:19:22 +0200

perf/ioctl: Add check for the sample_period value

perf_event_open() limits the sample_period to 63 bits. See:

  0819b2e30ccb ("perf: Limit perf_event_attr::sample_period to 63 bits")

Make ioctl() consistent with it.

Also on PowerPC, negative sample_period could cause a recursive
PMIs leading to a hang (reported when running perf-fuzzer).

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Vince Weaver <vincent.weaver@maine.edu>
Cc: acme@kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
Cc: maddy@linux.vnet.ibm.com
Cc: mpe@ellerman.id.au
Fixes: 0819b2e30ccb ("perf: Limit perf_event_attr::sample_period to 63 bits")
Link: https://lkml.kernel.org/r/20190604042953.914-1-ravi.bangoria@linux.ibm.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/events/core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 2e32faac5511..8d1c62df20a7 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -5005,6 +5005,9 @@ static int perf_event_period(struct perf_event *event, u64 __user *arg)
 	if (perf_event_check_period(event, value))
 		return -EINVAL;
 
+	if (!event->attr.freq && (value & (1ULL << 63)))
+		return -EINVAL;
+
 	event_function_call(event, __perf_event_period, &value);
 
 	return 0;

^ permalink raw reply related	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2019-06-25  8:19 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-11  2:42 [PATCH 1/2] perf ioctl: Add check for the sample_period value Ravi Bangoria
2019-05-11  2:42 ` Ravi Bangoria
2019-05-11  2:42 ` [PATCH 2/2] powerpc/perf: Fix mmcra corruption by bhrb_filter Ravi Bangoria
2019-05-11  2:42   ` Ravi Bangoria
2019-05-11  2:47   ` Ravi Bangoria
2019-05-11  2:47     ` Ravi Bangoria
2019-05-22  5:01   ` Madhavan Srinivasan
2019-05-22  5:01     ` Madhavan Srinivasan
2019-05-25  0:54   ` Michael Ellerman
2019-05-13  7:42 ` [PATCH 1/2] perf ioctl: Add check for the sample_period value Peter Zijlstra
2019-05-13  7:42   ` Peter Zijlstra
2019-05-13  8:56   ` Peter Zijlstra
2019-05-13  8:56     ` Peter Zijlstra
2019-05-13 10:07     ` Ravi Bangoria
2019-05-13 10:07       ` Ravi Bangoria
2019-05-28  9:50       ` Michael Ellerman
2019-05-28  9:50         ` Michael Ellerman
2019-06-04  4:29         ` [PATCH v2] " Ravi Bangoria
2019-06-04  4:29           ` Ravi Bangoria
2019-06-17  8:38           ` Ravi Bangoria
2019-06-17  8:38             ` Ravi Bangoria
2019-06-18 12:28             ` Michael Ellerman
2019-06-18 12:28               ` Michael Ellerman
2019-06-25  8:19           ` [tip:perf/urgent] perf/ioctl: " tip-bot for Ravi Bangoria

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.