diff mbox series

[bpf] bpf,perf: Fix perf_event_detach_bpf_prog error handling

Message ID 20241023100131.3400274-1-jolsa@kernel.org (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series [bpf] bpf,perf: Fix perf_event_detach_bpf_prog error handling | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for bpf
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 5 this patch: 5
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers fail 1 blamed authors not CCed: yonghong.song@linux.dev; 10 maintainers not CCed: mhiramat@kernel.org mathieu.desnoyers@efficios.com song@kernel.org linux-trace-kernel@vger.kernel.org martin.lau@linux.dev mattbobrowski@google.com kpsingh@kernel.org yonghong.song@linux.dev rostedt@goodmis.org eddyz87@gmail.com
netdev/build_clang success Errors and warnings before: 3 this patch: 3
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 20 this patch: 20
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 17 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-PR success PR summary
bpf/vmtest-bpf-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-VM_Test-17 success Logs for set-matrix
bpf/vmtest-bpf-VM_Test-18 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-VM_Test-16 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-VM_Test-15 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-VM_Test-19 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-VM_Test-20 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-25 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-26 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-27 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-28 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-VM_Test-29 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-34 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-36 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-35 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-VM_Test-33 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-41 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-14 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-VM_Test-13 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-VM_Test-21 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-22 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18

Commit Message

Jiri Olsa Oct. 23, 2024, 10:01 a.m. UTC 
Peter reported that perf_event_detach_bpf_prog might skip to release
the bpf program for -ENOENT error from bpf_prog_array_copy.

This can't happen because bpf program is stored in perf event and is
detached and released only when perf event is freed.

Let's make it obvious and add WARN_ON_ONCE on the -ENOENT check and
make sure the bpf program is released in any case.

Cc: Sean Young <sean@mess.org>
Fixes: 170a7e3ea070 ("bpf: bpf_prog_array_copy() should return -ENOENT if exclude_prog not found")
Closes: https://lore.kernel.org/lkml/20241022111638.GC16066@noisy.programming.kicks-ass.net/
Reported-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/trace/bpf_trace.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Peter Zijlstra Oct. 23, 2024, 10:05 a.m. UTC | #1 
On Wed, Oct 23, 2024 at 12:01:31PM +0200, Jiri Olsa wrote:
> Peter reported that perf_event_detach_bpf_prog might skip to release
> the bpf program for -ENOENT error from bpf_prog_array_copy.
> 
> This can't happen because bpf program is stored in perf event and is
> detached and released only when perf event is freed.
> 
> Let's make it obvious and add WARN_ON_ONCE on the -ENOENT check and
> make sure the bpf program is released in any case.
> 
> Cc: Sean Young <sean@mess.org>
> Fixes: 170a7e3ea070 ("bpf: bpf_prog_array_copy() should return -ENOENT if exclude_prog not found")
> Closes: https://lore.kernel.org/lkml/20241022111638.GC16066@noisy.programming.kicks-ass.net/
> Reported-by: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Thanks Jiri!

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

> ---
>  kernel/trace/bpf_trace.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 95b6b3b16bac..2c064ba7b0bd 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -2216,8 +2216,8 @@ void perf_event_detach_bpf_prog(struct perf_event *event)
>  
>  	old_array = bpf_event_rcu_dereference(event->tp_event->prog_array);
>  	ret = bpf_prog_array_copy(old_array, event->prog, NULL, 0, &new_array);
> -	if (ret == -ENOENT)
> -		goto unlock;
> +	if (WARN_ON_ONCE(ret == -ENOENT))
> +		goto put;
>  	if (ret < 0) {
>  		bpf_prog_array_delete_safe(old_array, event->prog);
>  	} else {
> @@ -2225,6 +2225,7 @@ void perf_event_detach_bpf_prog(struct perf_event *event)
>  		bpf_prog_array_free_sleepable(old_array);
>  	}
>  
> +put:
>  	bpf_prog_put(event->prog);
>  	event->prog = NULL;
>  
> -- 
> 2.46.2
>
Sean Young Oct. 23, 2024, 10:32 a.m. UTC | #2 
On Wed, Oct 23, 2024 at 12:01:31PM +0200, Jiri Olsa wrote:
> Peter reported that perf_event_detach_bpf_prog might skip to release
> the bpf program for -ENOENT error from bpf_prog_array_copy.
> 
> This can't happen because bpf program is stored in perf event and is
> detached and released only when perf event is freed.
> 
> Let's make it obvious and add WARN_ON_ONCE on the -ENOENT check and
> make sure the bpf program is released in any case.

Looks good. Should be unreachable anyway, so it doesn't matter. My
preference would be to just delete the lines, but no harm in belt and braces.

Acked-by: Sean Young <sean@mess.org>
> 
> Cc: Sean Young <sean@mess.org>
> Fixes: 170a7e3ea070 ("bpf: bpf_prog_array_copy() should return -ENOENT if exclude_prog not found")
> Closes: https://lore.kernel.org/lkml/20241022111638.GC16066@noisy.programming.kicks-ass.net/
> Reported-by: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  kernel/trace/bpf_trace.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 95b6b3b16bac..2c064ba7b0bd 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -2216,8 +2216,8 @@ void perf_event_detach_bpf_prog(struct perf_event *event)
>  
>  	old_array = bpf_event_rcu_dereference(event->tp_event->prog_array);
>  	ret = bpf_prog_array_copy(old_array, event->prog, NULL, 0, &new_array);
> -	if (ret == -ENOENT)
> -		goto unlock;
> +	if (WARN_ON_ONCE(ret == -ENOENT))
> +		goto put;
>  	if (ret < 0) {
>  		bpf_prog_array_delete_safe(old_array, event->prog);
>  	} else {
> @@ -2225,6 +2225,7 @@ void perf_event_detach_bpf_prog(struct perf_event *event)
>  		bpf_prog_array_free_sleepable(old_array);
>  	}
>  
> +put:
>  	bpf_prog_put(event->prog);
>  	event->prog = NULL;
>  
> -- 
> 2.46.2
Andrii Nakryiko Oct. 23, 2024, 4:01 p.m. UTC | #3 
On Wed, Oct 23, 2024 at 3:01 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Peter reported that perf_event_detach_bpf_prog might skip to release
> the bpf program for -ENOENT error from bpf_prog_array_copy.
>
> This can't happen because bpf program is stored in perf event and is
> detached and released only when perf event is freed.
>
> Let's make it obvious and add WARN_ON_ONCE on the -ENOENT check and
> make sure the bpf program is released in any case.
>
> Cc: Sean Young <sean@mess.org>
> Fixes: 170a7e3ea070 ("bpf: bpf_prog_array_copy() should return -ENOENT if exclude_prog not found")
> Closes: https://lore.kernel.org/lkml/20241022111638.GC16066@noisy.programming.kicks-ass.net/
> Reported-by: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  kernel/trace/bpf_trace.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 95b6b3b16bac..2c064ba7b0bd 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -2216,8 +2216,8 @@ void perf_event_detach_bpf_prog(struct perf_event *event)
>
>         old_array = bpf_event_rcu_dereference(event->tp_event->prog_array);
>         ret = bpf_prog_array_copy(old_array, event->prog, NULL, 0, &new_array);
> -       if (ret == -ENOENT)
> -               goto unlock;
> +       if (WARN_ON_ONCE(ret == -ENOENT))
> +               goto put;
>         if (ret < 0) {
>                 bpf_prog_array_delete_safe(old_array, event->prog);

seeing

if (ret < 0)
    bpf_prog_array_delete_safe(old_array, event->prog);

I think neither ret == -ENOENT nor WARN_ON_ONCE is necessary,  tbh. So
now I feel like just dropping WARN_ON_ONCE() is better.

>         } else {
> @@ -2225,6 +2225,7 @@ void perf_event_detach_bpf_prog(struct perf_event *event)
>                 bpf_prog_array_free_sleepable(old_array);
>         }
>
> +put:
>         bpf_prog_put(event->prog);
>         event->prog = NULL;
>
> --
> 2.46.2
>
Jiri Olsa Oct. 23, 2024, 7:08 p.m. UTC | #4 
On Wed, Oct 23, 2024 at 09:01:02AM -0700, Andrii Nakryiko wrote:
> On Wed, Oct 23, 2024 at 3:01 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Peter reported that perf_event_detach_bpf_prog might skip to release
> > the bpf program for -ENOENT error from bpf_prog_array_copy.
> >
> > This can't happen because bpf program is stored in perf event and is
> > detached and released only when perf event is freed.
> >
> > Let's make it obvious and add WARN_ON_ONCE on the -ENOENT check and
> > make sure the bpf program is released in any case.
> >
> > Cc: Sean Young <sean@mess.org>
> > Fixes: 170a7e3ea070 ("bpf: bpf_prog_array_copy() should return -ENOENT if exclude_prog not found")
> > Closes: https://lore.kernel.org/lkml/20241022111638.GC16066@noisy.programming.kicks-ass.net/
> > Reported-by: Peter Zijlstra <peterz@infradead.org>
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  kernel/trace/bpf_trace.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index 95b6b3b16bac..2c064ba7b0bd 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -2216,8 +2216,8 @@ void perf_event_detach_bpf_prog(struct perf_event *event)
> >
> >         old_array = bpf_event_rcu_dereference(event->tp_event->prog_array);
> >         ret = bpf_prog_array_copy(old_array, event->prog, NULL, 0, &new_array);
> > -       if (ret == -ENOENT)
> > -               goto unlock;
> > +       if (WARN_ON_ONCE(ret == -ENOENT))
> > +               goto put;
> >         if (ret < 0) {
> >                 bpf_prog_array_delete_safe(old_array, event->prog);
> 
> seeing
> 
> if (ret < 0)
>     bpf_prog_array_delete_safe(old_array, event->prog);
> 
> I think neither ret == -ENOENT nor WARN_ON_ONCE is necessary,  tbh. So
> now I feel like just dropping WARN_ON_ONCE() is better.

heh, I was going back and forth with that and decided with 'safer' option,
but it's 2 of you now asking for that, I'll send v2 then

jirka

> 
> >         } else {
> > @@ -2225,6 +2225,7 @@ void perf_event_detach_bpf_prog(struct perf_event *event)
> >                 bpf_prog_array_free_sleepable(old_array);
> >         }
> >
> > +put:
> >         bpf_prog_put(event->prog);
> >         event->prog = NULL;
> >
> > --
> > 2.46.2
> >
diff mbox series

Patch

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 95b6b3b16bac..2c064ba7b0bd 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -2216,8 +2216,8 @@  void perf_event_detach_bpf_prog(struct perf_event *event)
 
 	old_array = bpf_event_rcu_dereference(event->tp_event->prog_array);
 	ret = bpf_prog_array_copy(old_array, event->prog, NULL, 0, &new_array);
-	if (ret == -ENOENT)
-		goto unlock;
+	if (WARN_ON_ONCE(ret == -ENOENT))
+		goto put;
 	if (ret < 0) {
 		bpf_prog_array_delete_safe(old_array, event->prog);
 	} else {
@@ -2225,6 +2225,7 @@  void perf_event_detach_bpf_prog(struct perf_event *event)
 		bpf_prog_array_free_sleepable(old_array);
 	}
 
+put:
 	bpf_prog_put(event->prog);
 	event->prog = NULL;