Re: [PATCH bpf-next] perf/bpf: Use prog to emit ksymbol event for main program

[Krister Johansen <kjlx@templeofstupid.com>]

On Sun, Jul 14, 2024 at 02:55:33PM +0800, Hou Tao wrote:
> From: Hou Tao <houtao1@huawei.com>
> 
> Since commit 0108a4e9f358 ("bpf: ensure main program has an extable"),
> prog->aux->func[0]->kallsyms is left as uninitialized. For bpf program
> with subprogs, the symbol for the main program is missed just as shown
> in the output of perf script below:
> 
>  ffffffff81284b69 qp_trie_lookup_elem+0xb9 ([kernel.kallsyms])
>  ffffffffc0011125 bpf_prog_a4a0eb0651e6af8b_lookup_qp_trie+0x5d (bpf...)
>  ffffffff8127bc2b bpf_for_each_array_elem+0x7b ([kernel.kallsyms])
>  ffffffffc00110a1 +0x25 ()
>  ffffffff8121a89a trace_call_bpf+0xca ([kernel.kallsyms])
> 
> Fix it by always using prog instead prog->aux->func[0] to emit ksymbol
> event for the main program. After the fix, the output of perf script
> will be correct:
> 
>  ffffffff81284b96 qp_trie_lookup_elem+0xe6 ([kernel.kallsyms])
>  ffffffffc001382d bpf_prog_a4a0eb0651e6af8b_lookup_qp_trie+0x5d (bpf...)
>  ffffffff8127bc2b bpf_for_each_array_elem+0x7b ([kernel.kallsyms])
>  ffffffffc0013779 bpf_prog_245c55ab25cfcf40_qp_trie_lookup+0x25 (bpf...)
>  ffffffff8121a89a trace_call_bpf+0xca ([kernel.kallsyms])
> 
> Fixes: 0108a4e9f358 ("bpf: ensure main program has an extable")
> Signed-off-by: Hou Tao <houtao1@huawei.com>
> ---
> Hi,
> 
> ksymbol for bpf program had been broken twice, and I think it is better
> to add a bpf selftest for it, but I'm not so familiar with the
> perf_event_open(), for now I just post the fix patch and will post the
> selftest later.
> 
>  kernel/events/core.c | 28 +++++++++++++---------------
>  1 file changed, 13 insertions(+), 15 deletions(-)
> 
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index f0128c5ff278..e1b7d9e61fa0 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -9289,21 +9289,19 @@ static void perf_event_bpf_emit_ksymbols(struct bpf_prog *prog,
>  	bool unregister = type == PERF_BPF_EVENT_PROG_UNLOAD;
>  	int i;
>  
> -	if (prog->aux->func_cnt == 0) {
> -		perf_event_ksymbol(PERF_RECORD_KSYMBOL_TYPE_BPF,
> -				   (u64)(unsigned long)prog->bpf_func,
> -				   prog->jited_len, unregister,
> -				   prog->aux->ksym.name);
> -	} else {
> -		for (i = 0; i < prog->aux->func_cnt; i++) {
> -			struct bpf_prog *subprog = prog->aux->func[i];
> -
> -			perf_event_ksymbol(
> -				PERF_RECORD_KSYMBOL_TYPE_BPF,
> -				(u64)(unsigned long)subprog->bpf_func,
> -				subprog->jited_len, unregister,
> -				subprog->aux->ksym.name);
> -		}
> +	perf_event_ksymbol(PERF_RECORD_KSYMBOL_TYPE_BPF,
> +			   (u64)(unsigned long)prog->bpf_func,
> +			   prog->jited_len, unregister,
> +			   prog->aux->ksym.name);
> +
> +	for (i = 1; i < prog->aux->func_cnt; i++) {
> +		struct bpf_prog *subprog = prog->aux->func[i];
> +
> +		perf_event_ksymbol(
> +			PERF_RECORD_KSYMBOL_TYPE_BPF,
> +			(u64)(unsigned long)subprog->bpf_func,
> +			subprog->jited_len, unregister,
> +			subprog->aux->ksym.name);
>  	}
>  }

Thanks, this looks correct to me.  Sorry that I missed this earlier.

Reviewed-by: Krister Johansen <kjlx@templeofstupid.com>

-K