* Re: [PATCH bpf-next] perf/bpf: Use prog to emit ksymbol event for main program
2024-07-14 6:55 [PATCH bpf-next] perf/bpf: Use prog to emit ksymbol event for main program Hou Tao
@ 2024-07-16 1:36 ` Krister Johansen
2024-07-16 7:27 ` Jiri Olsa
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Krister Johansen @ 2024-07-16 1:36 UTC (permalink / raw)
To: Hou Tao
Cc: bpf, linux-perf-users, Arnaldo Carvalho de Melo, Jiri Olsa,
Alexei Starovoitov, Yonghong Song, Krister Johansen, houtao1
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH bpf-next] perf/bpf: Use prog to emit ksymbol event for main program
2024-07-14 6:55 [PATCH bpf-next] perf/bpf: Use prog to emit ksymbol event for main program Hou Tao
2024-07-16 1:36 ` Krister Johansen
@ 2024-07-16 7:27 ` Jiri Olsa
2024-07-19 2:25 ` Yonghong Song
2024-07-19 15:30 ` patchwork-bot+netdevbpf
3 siblings, 0 replies; 5+ messages in thread
From: Jiri Olsa @ 2024-07-16 7:27 UTC (permalink / raw)
To: Hou Tao
Cc: bpf, linux-perf-users, Arnaldo Carvalho de Melo,
Alexei Starovoitov, Yonghong Song, Krister Johansen, houtao1
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.
good idea, lgtm
Reviewed-by: Jiri Olsa <jolsa@kernel.org>
jirka
>
> 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);
> }
> }
>
> --
> 2.29.2
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH bpf-next] perf/bpf: Use prog to emit ksymbol event for main program
2024-07-14 6:55 [PATCH bpf-next] perf/bpf: Use prog to emit ksymbol event for main program Hou Tao
2024-07-16 1:36 ` Krister Johansen
2024-07-16 7:27 ` Jiri Olsa
@ 2024-07-19 2:25 ` Yonghong Song
2024-07-19 15:30 ` patchwork-bot+netdevbpf
3 siblings, 0 replies; 5+ messages in thread
From: Yonghong Song @ 2024-07-19 2:25 UTC (permalink / raw)
To: Hou Tao, bpf
Cc: linux-perf-users, Arnaldo Carvalho de Melo, Jiri Olsa,
Alexei Starovoitov, Yonghong Song, Krister Johansen, houtao1
On 7/13/24 11:55 PM, 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(-)
I actually just hit this issue with patch in
https://lore.kernel.org/bpf/20240718205158.3651529-1-yonghong.song@linux.dev/T/#m92fd74865d93306757a10eb27d1e4f84cfffd5a8
This patch fixed the issue. So
Tested-by: Yonghong Song <yonghong.song@linux.dev>
>
> 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);
> }
> }
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH bpf-next] perf/bpf: Use prog to emit ksymbol event for main program
2024-07-14 6:55 [PATCH bpf-next] perf/bpf: Use prog to emit ksymbol event for main program Hou Tao
` (2 preceding siblings ...)
2024-07-19 2:25 ` Yonghong Song
@ 2024-07-19 15:30 ` patchwork-bot+netdevbpf
3 siblings, 0 replies; 5+ messages in thread
From: patchwork-bot+netdevbpf @ 2024-07-19 15:30 UTC (permalink / raw)
To: Hou Tao; +Cc: bpf, linux-perf-users, acme, jolsa, ast, yhs, kjlx, houtao1
Hello:
This patch was applied to bpf/bpf.git (master)
by Daniel Borkmann <daniel@iogearbox.net>:
On Sun, 14 Jul 2024 14:55:33 +0800 you 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:
>
> [...]
Here is the summary with links:
- [bpf-next] perf/bpf: Use prog to emit ksymbol event for main program
https://git.kernel.org/bpf/bpf/c/0be9ae5486cd
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 5+ messages in thread