bpf: fix bpf_perf_event_read() helper

PATCH Superseded Alexei Starovoitov 3 participants 12 messages Oct 21, 2015 – Oct 22, 2015

Subsystems (4)

Networking Network drivers BPF perf events

lore

Share & tools

lore

Alexei Starovoitov

author

2015-10-21 22:58 UTC #0 lore raw

PATCH v2 1/1 bpf: fix bpf_perf_event_read() helper

Fix safety checks for bpf_perf_event_read():
- only non-inherited events can be added to perf_event_array map (do this check statically at map insertion time)
- dynamically check that event is local and !pmu->count Otherwise buggy bpf program can cause kernel splat.

Fixes 35578d798400 ("bpf: Implement function bpf_perf_event_read() that get the selected hardware PMU conuter")

Signed-off-by Alexei Starovoitov <[email protected]>

---
v1->v2: fix compile in case of !CONFIG_PERF_EVENTS

This patch is on top of
http://patchwork.ozlabs.org/patch/533585/
to avoid conflicts.
Even in the worst case the crash is not possible.
Only warn_on_once, so imo net-next is ok.

 kernel/bpf/arraymap.c |    9 +++++----
 kernel/events/core.c  |   16 ++++++++++------
 2 files changed, 15 insertions(+), 10 deletions(-)

kernel/bpf/arraymap.c +5 -4

index e3cfe46b074f..75529cc94304 100644
@@ -294,10 +294,11 @@ static void *perf_event_fd_array_get_ptr(struct bpf_map *map, int fd)
 	if (IS_ERR(attr))
 		return (void *)attr;
 
-	if (attr->type != PERF_TYPE_RAW &&
-	    !(attr->type == PERF_TYPE_SOFTWARE &&
-	      attr->config == PERF_COUNT_SW_BPF_OUTPUT) &&
-	    attr->type != PERF_TYPE_HARDWARE) {
+	if ((attr->type != PERF_TYPE_RAW &&
+	     !(attr->type == PERF_TYPE_SOFTWARE &&
+	       attr->config == PERF_COUNT_SW_BPF_OUTPUT) &&
+	     attr->type != PERF_TYPE_HARDWARE) ||
+	    attr->inherit) {
 		perf_event_release_kernel(event);
 		return ERR_PTR(-EINVAL);
 	}

kernel/events/core.c +10 -6

index 64754bfecd70..0b6333265872 100644
@@ -3258,7 +3258,7 @@ static inline u64 perf_event_count(struct perf_event *event)
 u64 perf_event_read_local(struct perf_event *event)
 {
 	unsigned long flags;
-	u64 val;
+	u64 val = -EINVAL;
 
 	/*
 	 * Disabling interrupts avoids all counter scheduling (context

@@ -3267,12 +3267,14 @@ u64 perf_event_read_local(struct perf_event *event)
 	local_irq_save(flags);
 
 	/* If this is a per-task event, it must be for current */
-	WARN_ON_ONCE((event->attach_state & PERF_ATTACH_TASK) &&
-		     event->hw.target != current);
+	if ((event->attach_state & PERF_ATTACH_TASK) &&
+	    event->hw.target != current)
+		goto out;
 
 	/* If this is a per-CPU event, it must be for this CPU */
-	WARN_ON_ONCE(!(event->attach_state & PERF_ATTACH_TASK) &&
-		     event->cpu != smp_processor_id());
+	if (!(event->attach_state & PERF_ATTACH_TASK) &&
+	    event->cpu != smp_processor_id())
+		goto out;
 
 	/*
 	 * It must not be an event with inherit set, we cannot read

@@ -3284,7 +3286,8 @@ u64 perf_event_read_local(struct perf_event *event)
 	 * It must not have a pmu::count method, those are not
 	 * NMI safe.
 	 */
-	WARN_ON_ONCE(event->pmu->count);
+	if (event->pmu->count)
+		goto out;
 
 	/*
 	 * If the event is currently on this CPU, its either a per-task event,

@@ -3295,6 +3298,7 @@ u64 perf_event_read_local(struct perf_event *event)
 		event->pmu->read(event);
 
 	val = local64_read(&event->count);
+out:
 	local_irq_restore(flags);
 
 	return val;

Signature · 1 line

1.7.9.5

Wangnan (F)

2015-10-22 04:49 UTC #1 lore raw

After applying this patch I'm unable to use perf passing perf_event again like this:

 # perf record -a -e evt=cycles -e 
./test_config_map.c/maps.pmu_map.event=evt/ --exclude-perf ls

With -v it output:

...
adding perf_bpf_probe:func_write
adding perf_bpf_probe:func_write to 0x367d6a0
add bpf event perf_bpf_probe:func_write_return and attach bpf program 6 adding perf_bpf_probe:func_write_return
adding perf_bpf_probe:func_write_return to 0x3a7fc40
mmap size 528384B
ERROR: failed to insert value to pmu_map[0]
ERROR: Apply config to BPF failed: Invalid option for map, add -v to see detail Opening /sys/kernel/debug/tracing//kprobe_events write=
...

Looks like perf sets attr.inherit for cycles? I'll look into this problem.

Thank you.

Quoted 91 lines · Alexei Starovoitov

Fixes 35578d798400 ("bpf: Implement function bpf_perf_event_read() that get the selected hardware PMU conuter")

Signed-off-by Alexei Starovoitov <[email protected]>

---
v1->v2: fix compile in case of !CONFIG_PERF_EVENTS

This patch is on top of
http://patchwork.ozlabs.org/patch/533585/
to avoid conflicts.
Even in the worst case the crash is not possible.
Only warn_on_once, so imo net-next is ok.

  kernel/bpf/arraymap.c |    9 +++++----
  kernel/events/core.c  |   16 ++++++++++------
  2 files changed, 15 insertions(+), 10 deletions(-)

kernel/bpf/arraymap.c +5 -4

index e3cfe46b074f..75529cc94304 100644
@@ -294,10 +294,11 @@ static void *perf_event_fd_array_get_ptr(struct bpf_map *map, int fd)
  	if (IS_ERR(attr))
  		return (void *)attr;
 
-	if (attr->type != PERF_TYPE_RAW &&
-	    !(attr->type == PERF_TYPE_SOFTWARE &&
-	      attr->config == PERF_COUNT_SW_BPF_OUTPUT) &&
-	    attr->type != PERF_TYPE_HARDWARE) {
+	if ((attr->type != PERF_TYPE_RAW &&
+	     !(attr->type == PERF_TYPE_SOFTWARE &&
+	       attr->config == PERF_COUNT_SW_BPF_OUTPUT) &&
+	     attr->type != PERF_TYPE_HARDWARE) ||
+	    attr->inherit) {
  		perf_event_release_kernel(event);
  		return ERR_PTR(-EINVAL);
  	}

kernel/events/core.c +10 -6

index 64754bfecd70..0b6333265872 100644
@@ -3258,7 +3258,7 @@ static inline u64 perf_event_count(struct perf_event *event)
  u64 perf_event_read_local(struct perf_event *event)
  {
  	unsigned long flags;
-	u64 val;
+	u64 val = -EINVAL;
 
  	/*
  	 * Disabling interrupts avoids all counter scheduling (context

@@ -3267,12 +3267,14 @@ u64 perf_event_read_local(struct perf_event *event)
  	local_irq_save(flags);
 
  	/* If this is a per-task event, it must be for current */
-	WARN_ON_ONCE((event->attach_state & PERF_ATTACH_TASK) &&
-		     event->hw.target != current);
+	if ((event->attach_state & PERF_ATTACH_TASK) &&
+	    event->hw.target != current)
+		goto out;
 
  	/* If this is a per-CPU event, it must be for this CPU */
-	WARN_ON_ONCE(!(event->attach_state & PERF_ATTACH_TASK) &&
-		     event->cpu != smp_processor_id());
+	if (!(event->attach_state & PERF_ATTACH_TASK) &&
+	    event->cpu != smp_processor_id())
+		goto out;
 
  	/*
  	 * It must not be an event with inherit set, we cannot read

@@ -3284,7 +3286,8 @@ u64 perf_event_read_local(struct perf_event *event)
  	 * It must not have a pmu::count method, those are not
  	 * NMI safe.
  	 */
-	WARN_ON_ONCE(event->pmu->count);
+	if (event->pmu->count)
+		goto out;
 
  	/*
  	 * If the event is currently on this CPU, its either a per-task event,

@@ -3295,6 +3298,7 @@ u64 perf_event_read_local(struct perf_event *event)
  		event->pmu->read(event);
 
  	val = local64_read(&event->count);
+out:
  	local_irq_restore(flags);
 
  	return val;

Alexei Starovoitov

author

2015-10-22 05:00 UTC #2 lore raw

Quoted 2 lines · Wangnan (F)

After applying this patch I'm unable to use perf passing perf_event again like this:

please do not top post and trim your replies.

Quoted 18 lines

  # perf record -a -e evt=cycles -e
./test_config_map.c/maps.pmu_map.event=evt/ --exclude-perf ls
With -v it output:
...
adding perf_bpf_probe:func_write
adding perf_bpf_probe:func_write to 0x367d6a0
add bpf event perf_bpf_probe:func_write_return and attach bpf program 6 adding perf_bpf_probe:func_write_return
adding perf_bpf_probe:func_write_return to 0x3a7fc40
mmap size 528384B
ERROR: failed to insert value to pmu_map[0]
ERROR: Apply config to BPF failed: Invalid option for map, add -v to see detail
Opening /sys/kernel/debug/tracing//kprobe_events write=
...

Quoted 1 line

Looks like perf sets attr.inherit for cycles? I'll look into this problem.

yes. that's perf default.
How did it even work before?!
I was testing with your samples/bpf/tracex6 that sets inherit to zero.

Wangnan (F)

2015-10-22 05:30 UTC #3 lore raw

Quoted 24 lines · Alexei Starovoitov

Quoted 2 lines · Wangnan (F)
After applying this patch I'm unable to use perf passing perf_event again like this:
please do not top post and trim your replies.
Quoted 18 lines
  # perf record -a -e evt=cycles -e
./test_config_map.c/maps.pmu_map.event=evt/ --exclude-perf ls
With -v it output:
...
adding perf_bpf_probe:func_write
adding perf_bpf_probe:func_write to 0x367d6a0
add bpf event perf_bpf_probe:func_write_return and attach bpf program 6 adding perf_bpf_probe:func_write_return
adding perf_bpf_probe:func_write_return to 0x3a7fc40
mmap size 528384B
ERROR: failed to insert value to pmu_map[0]
ERROR: Apply config to BPF failed: Invalid option for map, add -v to see detail
Opening /sys/kernel/debug/tracing//kprobe_events write=
...

Quoted 6 lines

Quoted 1 line
Looks like perf sets attr.inherit for cycles? I'll look into this problem.
yes. that's perf default.
How did it even work before?!
I was testing with your samples/bpf/tracex6 that sets inherit to zero.

Tested perf record -i option and it works for me:

# echo "" > /sys/kernel/debug/tracing/trace
# perf record -i -a -e evt=cycles -e ./test_config_map.c/maps.pmu_map.event=evt/ --exclude-perf ls
# cat /sys/kernel/debug/tracing/trace  | grep ls
              ls-8227  [001] dN..  2526.184611: : pmu inc: 82270
              ls-8227  [001] dN..  2526.184626: : pmu inc: 40951
              ls-8227  [001] dN..  2526.184642: : pmu inc: 50659
              ls-8227  [001] dN..  2526.184657: : pmu inc: 43511
              ls-8227  [001] dN..  2526.184675: : pmu inc: 56921
...
And no warning message found in dmesg.

So I think your fix is good, we should improve perf.

Thank you.

Wangnan (F)

2015-10-22 05:31 UTC #4 lore raw

In reply to Alexei Starovoitov

Quoted 28 lines · Alexei Starovoitov

Fix safety checks for bpf_perf_event_read():
- only non-inherited events can be added to perf_event_array map (do this check statically at map insertion time)
- dynamically check that event is local and !pmu->count Otherwise buggy bpf program can cause kernel splat.
Fixes 35578d798400 ("bpf: Implement function bpf_perf_event_read() that get the selected hardware PMU conuter")
Signed-off-by Alexei Starovoitov <[email protected]>
---
v1->v2: fix compile in case of !CONFIG_PERF_EVENTS
This patch is on top of
http://patchwork.ozlabs.org/patch/533585/
to avoid conflicts.
Even in the worst case the crash is not possible.
Only warn_on_once, so imo net-next is ok.
  kernel/bpf/arraymap.c |    9 +++++----
  kernel/events/core.c  |   16 ++++++++++------
  2 files changed, 15 insertions(+), 10 deletions(-)
kernel/bpf/arraymap.c +0 -0
index e3cfe46b074f..75529cc94304 100644
@@ -294,10 +294,11 @@ static void *perf_event_fd_array_get_ptr(struct bpf_map *map, int fd)
  	if (IS_ERR(attr))
  		return (void *)attr;

kernel/bpf/arraymap.c static void *perf_event_fd_array_get_ptr(struct bpf_map *map, int fd) · line 301

Quoted 10 lines

kernel/bpf/arraymap.c +5 -4

 
-	if (attr->type != PERF_TYPE_RAW &&
-	    !(attr->type == PERF_TYPE_SOFTWARE &&
-	      attr->config == PERF_COUNT_SW_BPF_OUTPUT) &&
-	    attr->type != PERF_TYPE_HARDWARE) {
+	if ((attr->type != PERF_TYPE_RAW &&
+	     !(attr->type == PERF_TYPE_SOFTWARE &&
+	       attr->config == PERF_COUNT_SW_BPF_OUTPUT) &&
+	     attr->type != PERF_TYPE_HARDWARE) ||
+	    attr->inherit) {

This 'if' statement is so complex. What about using a inline function instead?

Thank you.

Alexei Starovoitov

author

2015-10-22 06:21 UTC #5 lore raw

Quoted 8 lines · Wangnan (F)

kernel/bpf/arraymap.c static void *perf_event_fd_array_get_ptr(struct bpf_map *map, int fd) · line 301
Quoted 5 lines
kernel/bpf/arraymap.c +5 -0
+    if ((attr->type != PERF_TYPE_RAW &&
+         !(attr->type == PERF_TYPE_SOFTWARE &&
+           attr->config == PERF_COUNT_SW_BPF_OUTPUT) &&
+         attr->type != PERF_TYPE_HARDWARE) ||
+        attr->inherit) {
This 'if' statement is so complex. What about using a inline function instead?

hmm. don't see how inline function will help readability.

Wangnan (F)

2015-10-22 07:39 UTC #6 lore raw

Quoted 7 lines · Alexei Starovoitov

Quoted 6 lines · Wangnan (F)

Quoted 5 lines

kernel/bpf/arraymap.c +5 -0

+    if ((attr->type != PERF_TYPE_RAW &&
+         !(attr->type == PERF_TYPE_SOFTWARE &&
+           attr->config == PERF_COUNT_SW_BPF_OUTPUT) &&
+         attr->type != PERF_TYPE_HARDWARE) ||
+        attr->inherit) {

Quoted 5 lines

Quoted 2 lines
This 'if' statement is so complex. What about using a inline function instead?
hmm. don't see how inline function will help readability.

For example (not tested):

static inline bool perf_event_can_insert_to_map(struct perf_event_attr
*attr)

 {
    /* is inherit? */
    if (attr->inherit)
        return false;

    /* is software event? */
    if (attr->type == PERF_TYPE_SOFTWARE)
        if (attr->config == PERF_COUNT_SW_BPF_OUTPUT)
            return true;
        else
            return false;

    /* Comment... */
    if (attr->type == PERF_TYPE_RAW)
        return true;
    if (attr->type == PERF_TYPE_HARDWARE)
        return true;
    return false;

}

...
if (!perf_event_can_insert_to_map(attr))

    ....

Do you think redability is improved?

Thank you.

Wangnan (F)

2015-10-22 12:30 UTC #8 lore raw

In reply to Alexei Starovoitov

On 2015/10/22 6:58, Alexei Starovoitov wrote:

[SNIP]

Quoted 10 lines

kernel/bpf/arraymap.c +0 -2

index e3cfe46b074f..75529cc94304 100644
@@ -294,10 +294,11 @@ static void *perf_event_fd_array_get_ptr(struct bpf_map *map, int fd)
  	if (IS_ERR(attr))
  		return (void *)attr;
 
-	if (attr->type != PERF_TYPE_RAW &&
-	    !(attr->type == PERF_TYPE_SOFTWARE &&

kernel/bpf/arraymap.c static void *perf_event_fd_array_get_ptr(struct bpf_map *map, int fd) · line 304

Quoted 10 lines

kernel/bpf/arraymap.c +5 -2

-	      attr->config == PERF_COUNT_SW_BPF_OUTPUT) &&
-	    attr->type != PERF_TYPE_HARDWARE) {
+	if ((attr->type != PERF_TYPE_RAW &&
+	     !(attr->type == PERF_TYPE_SOFTWARE &&
+	       attr->config == PERF_COUNT_SW_BPF_OUTPUT) &&
+	     attr->type != PERF_TYPE_HARDWARE) ||
+	    attr->inherit) {
  		perf_event_release_kernel(event);
  		return ERR_PTR(-EINVAL);
  	}

I have a question on inherit, not related to this patch:
Is it safe for perf to disable attr->inherit if the event is system wide? I haven't read relate code completely. In my current knowledge the behavior of a system wide perf event should be same whether inherit is set or not. Is that true?

Thank you.

Peter Zijlstra

maintainer

2015-10-22 12:41 UTC #9 lore raw

Quoted 5 lines · Wangnan (F)

I have a question on inherit, not related to this patch:
Is it safe for perf to disable attr->inherit if the event is system wide? I haven't read relate code completely. In my current knowledge the behavior of a system wide perf event should be same whether inherit is set or not. Is that true?

Yes, .inherit is pointless for cpu wide events, if we allow creating cpu events with .inherit set that's unfortunate.

Peter Zijlstra

maintainer

2015-10-22 13:57 UTC #10 lore raw

In reply to Alexei Starovoitov

kernel/events/core.c static inline u64 perf_event_count(struct perf_event *event) · line 3261

Quoted 10 lines · Alexei Starovoitov

kernel/events/core.c +1 -1

index 64754bfecd70..0b6333265872 100644
@@ -3258,7 +3258,7 @@ static inline u64 perf_event_count(struct perf_event *event)
 u64 perf_event_read_local(struct perf_event *event)
 {
 	unsigned long flags;
-	u64 val;
+	u64 val = -EINVAL;

No, you cannot do this, -EINVAL is a valid count value. You simply must not call this function on !local events, ever.

Alexei Starovoitov

author

2015-10-22 15:51 UTC #7 lore raw

In reply to Wangnan (F)

Quoted 5 lines · Wangnan (F)

...
if (!perf_event_can_insert_to_map(attr))
     ....
Do you think redability is improved?

yes. makes sense. will respin.

Alexei Starovoitov

author

2015-10-22 15:53 UTC #11 lore raw

In reply to Peter Zijlstra

Quoted 14 lines · Peter Zijlstra

kernel/events/core.c static inline u64 perf_event_count(struct perf_event *event) · line 3261
Quoted 10 lines · Alexei Starovoitov
kernel/events/core.c +1 -1
index 64754bfecd70..0b6333265872 100644
@@ -3258,7 +3258,7 @@ static inline u64 perf_event_count(struct perf_event *event)
  u64 perf_event_read_local(struct perf_event *event)
  {
  	unsigned long flags;
-	u64 val;
+	u64 val = -EINVAL;
No, you cannot do this, -EINVAL is a valid count value. You simply must not call this function on !local events, ever.

agree. Will keep perf_event_read_local() as-is and do all safety checks on bpf side.