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.

Also fix error path after perf_event_attrs()
and remove redundant 'extern'.

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

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

---
v2->v3:
. refactor checks based on Wangnan's and Peter's feedback while refactoring realized that these two issues need fixes as well:
. fix perf_event_attrs() error path
. remove redundant extern

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

Even in the worst case the crash is not possible.
Only warn_on_once, so imo net-next is ok.

  include/linux/bpf.h      |    1 -
  kernel/bpf/arraymap.c    |   25 ++++++++++++++++---------
  kernel/trace/bpf_trace.c |    7 ++++++-
  3 files changed, 22 insertions(+), 11 deletions(-)

include/linux/bpf.h +0 -1

index e3a51b74e275..75718fa28260 100644
@@ -194,7 +194,6 @@ extern const struct bpf_func_proto bpf_map_lookup_elem_proto;
  extern const struct bpf_func_proto bpf_map_update_elem_proto;
  extern const struct bpf_func_proto bpf_map_delete_elem_proto;
 
-extern const struct bpf_func_proto bpf_perf_event_read_proto;
  extern const struct bpf_func_proto bpf_get_prandom_u32_proto;
  extern const struct bpf_func_proto bpf_get_smp_processor_id_proto;
  extern const struct bpf_func_proto bpf_tail_call_proto;

kernel/bpf/arraymap.c +1 -2

index e3cfe46b074f..3f4c99e06c6b 100644
@@ -292,16 +292,23 @@ static void *perf_event_fd_array_get_ptr(struct bpf_map *map, int fd)
 
  	attr = perf_event_attrs(event);
  	if (IS_ERR(attr))
-		return (void *)attr;
+		goto err;
 
-	if (attr->type != PERF_TYPE_RAW &&

kernel/bpf/arraymap.c +12 -0

+	if (attr->type == PERF_TYPE_RAW)
+		return event;
+
+	if (attr->type == PERF_TYPE_HARDWARE)
+		return event;
+
+	if (attr->type == PERF_TYPE_SOFTWARE &&
+	    attr->config == PERF_COUNT_SW_BPF_OUTPUT)
+		return event;
+err:
+	perf_event_release_kernel(event);
+	return ERR_PTR(-EINVAL);
  }
 
  static void perf_event_fd_array_put_ptr(void *ptr)

kernel/trace/bpf_trace.c +6 -1

index 47febbe7998e..003df3887287 100644
@@ -199,6 +199,11 @@ static u64 bpf_perf_event_read(u64 r1, u64 index, u64 r3, u64 r4, u64 r5)
  	if (!event)
  		return -ENOENT;
 
+	/* make sure event is local and doesn't have pmu::count */
+	if (event->oncpu != smp_processor_id() ||
+	    event->pmu->count)
+		return -EINVAL;
+
  	/*
  	 * we don't know if the function is run successfully by the
  	 * return value. It can be judged in other places, such as

@@ -207,7 +212,7 @@ static u64 bpf_perf_event_read(u64 r1, u64 index, u64 r3, u64 r4, u64 r5)
  	return perf_event_read_local(event);
  }
 
-const struct bpf_func_proto bpf_perf_event_read_proto = {
+static const struct bpf_func_proto bpf_perf_event_read_proto = {
  	.func		= bpf_perf_event_read,
  	.gpl_only	= false,
  	.ret_type	= RET_INTEGER,

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

Quoted 3 lines

kernel/bpf/arraymap.c +3 -0

+    if (attr->inherit)
+        goto err;
+

Since Peter suggest it is pointless for a system-wide perf_event has inherit bit set [1], I think it should be safe to enable system-wide perf_event pass this check?

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.

Also fix error path after perf_event_attrs()
and remove redundant 'extern'.

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

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

So the bpf_perf_event_read() returns the count value, does this not also mean that returning -EINVAL here is also 'wrong'?

I mean, sure an actual count value that high is unlikely, but its still a broken interface.

Quoted 5 lines · Peter Zijlstra

So the bpf_perf_event_read() returns the count value, does this not also mean that returning -EINVAL here is also 'wrong'?

I mean, sure an actual count value that high is unlikely, but its still a broken interface.

Agree. that's not pretty interface. I wish I looked at it more carefully when it was introduced. Now it's too late to change.

Quoted 5 lines · Peter Zijlstra

So the bpf_perf_event_read() returns the count value, does this not also mean that returning -EINVAL here is also 'wrong'?

I mean, sure an actual count value that high is unlikely, but its still a broken interface.

Agree. that's not pretty interface. I wish I looked at it more carefully when it was introduced. Now it's too late to change.

Then old crap can be de-emphasised and eventually removed, instead of having to live with crap forever ...

bpf_perf_event_read() muxes of -EINVAL into return value, but it's non ambiguous to the program whether it got an error or real counter value.

Quoted 2 lines · Alexei Starovoitov

bpf_perf_event_read() muxes of -EINVAL into return value, but it's non ambiguous to the program whether it got an error or real counter value.

How can that be, the (u64)-EINVAL value is a valid counter value.. unlikely maybe, but still quite possible.

Quoted 1 line · Peter Zijlstra

On Sun, Oct 25, 2015 at 09:23:36AM -0700, Alexei Starovoitov wrote:

Quoted 4 lines

Quoted 2 lines

bpf_perf_event_read() muxes of -EINVAL into return value, but it's non ambiguous to the program whether it got an error or real counter value.

How can that be, the (u64)-EINVAL value is a valid counter value.. unlikely maybe, but still quite possible.

In our real usecase we simply treat return value larger than 0x7fffffffffffffff
as error result. We can make it even larger, for example, to 0xffffffffffff0000.

Resuling values can be pre-processed by a script to filter potential error result
out so it is not a very big problem for our real usecases.

For a better interface, I suggest

  u64 bpf_perf_event_read(bool *perror);

which still returns counter value through its return value but put error code
to stack. Then BPF program can pass NULL to the function if BPF problem doesn't want to deal with error code.

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.

Also fix error path after perf_event_attrs()
and remove redundant 'extern'.

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

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