Re: [PATCH bpf] bpf,perf: Fix perf_event_detach_bpf_prog error handling

[Sean Young <sean@mess.org>]

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