Message-ID: <CAADnVQ+DF18nKEf9i1RKEQN+ybH+duu7U-91YZDaa_PiqUx17g@mail.gmail.com>
Date: Tue, 29 Apr 2025 19:46:05 -0700
From: Alexei Starovoitov <alexei.starovoitov@...il.com>
To: KaFai Wan <mannkafai@...il.com>
Cc: Song Liu <song@...nel.org>, Jiri Olsa <jolsa@...nel.org>, 
	Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>, 
	Andrii Nakryiko <andrii@...nel.org>, Martin KaFai Lau <martin.lau@...ux.dev>, Eduard <eddyz87@...il.com>, 
	Yonghong Song <yonghong.song@...ux.dev>, John Fastabend <john.fastabend@...il.com>, 
	KP Singh <kpsingh@...nel.org>, Stanislav Fomichev <sdf@...ichev.me>, Hao Luo <haoluo@...gle.com>, 
	Matt Bobrowski <mattbobrowski@...gle.com>, Steven Rostedt <rostedt@...dmis.org>, 
	Masami Hiramatsu <mhiramat@...nel.org>, Mathieu Desnoyers <mathieu.desnoyers@...icios.com>, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>, 
	Mykola Lysenko <mykolal@...com>, Shuah Khan <shuah@...nel.org>, LKML <linux-kernel@...r.kernel.org>, 
	bpf <bpf@...r.kernel.org>, 
	linux-trace-kernel <linux-trace-kernel@...r.kernel.org>, 
	Network Development <netdev@...r.kernel.org>, 
	"open list:KERNEL SELFTEST FRAMEWORK" <linux-kselftest@...r.kernel.org>, Leon Hwang <leon.hwang@...ux.dev>
Subject: Re: [PATCH bpf-next 1/4] bpf: Allow get_func_[arg|arg_cnt] helpers in
 raw tracepoint programs

On Sat, Apr 26, 2025 at 9:00 AM KaFai Wan <mannkafai@...il.com> wrote:
>
> Adding support to use get_func_[arg|arg_cnt] helpers in raw_tp/tp_btf
> programs.
>
> We can use get_func_[arg|ret|arg_cnt] helpers in fentry/fexit/fmod_ret
> programs currently. If we try to use get_func_[arg|arg_cnt] helpers in
> raw_tp/tp_btf programs, verifier will fail to load the program with:
>
> ; __u64 cnt = bpf_get_func_arg_cnt(ctx);
> 3: (85) call bpf_get_func_arg_cnt#185
> unknown func bpf_get_func_arg_cnt#185
>
> Adding get_func_[arg|arg_cnt] helpers in raw_tp_prog_func_proto and
> tracing_prog_func_proto for raw tracepoint.
>
> Adding 1 arg on ctx of raw tracepoint program and make it stores number of
> arguments on ctx-8, so it's easy to verify argument index and find
> argument's position.
>
> Signed-off-by: KaFai Wan <mannkafai@...il.com>
> ---
>  kernel/trace/bpf_trace.c | 17 ++++++++++++++---
>  net/bpf/test_run.c       | 13 +++++--------
>  2 files changed, 19 insertions(+), 11 deletions(-)
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 52c432a44aeb..eb4c56013493 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -1892,6 +1892,10 @@ raw_tp_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>                 return &bpf_get_stackid_proto_raw_tp;
>         case BPF_FUNC_get_stack:
>                 return &bpf_get_stack_proto_raw_tp;
> +       case BPF_FUNC_get_func_arg:
> +               return &bpf_get_func_arg_proto;
> +       case BPF_FUNC_get_func_arg_cnt:
> +               return &bpf_get_func_arg_cnt_proto;
>         case BPF_FUNC_get_attach_cookie:
>                 return &bpf_get_attach_cookie_proto_tracing;
>         default:
> @@ -1950,10 +1954,16 @@ tracing_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>         case BPF_FUNC_d_path:
>                 return &bpf_d_path_proto;
>         case BPF_FUNC_get_func_arg:
> +               if (prog->type == BPF_PROG_TYPE_TRACING &&
> +                   prog->expected_attach_type == BPF_TRACE_RAW_TP)
> +                       return &bpf_get_func_arg_proto;
>                 return bpf_prog_has_trampoline(prog) ? &bpf_get_func_arg_proto : NULL;
>         case BPF_FUNC_get_func_ret:
>                 return bpf_prog_has_trampoline(prog) ? &bpf_get_func_ret_proto : NULL;
>         case BPF_FUNC_get_func_arg_cnt:
> +               if (prog->type == BPF_PROG_TYPE_TRACING &&
> +                   prog->expected_attach_type == BPF_TRACE_RAW_TP)
> +                       return &bpf_get_func_arg_cnt_proto;
>                 return bpf_prog_has_trampoline(prog) ? &bpf_get_func_arg_cnt_proto : NULL;
>         case BPF_FUNC_get_attach_cookie:
>                 if (prog->type == BPF_PROG_TYPE_TRACING &&
> @@ -2312,7 +2322,7 @@ void __bpf_trace_run(struct bpf_raw_tp_link *link, u64 *args)
>  #define REPEAT(X, FN, DL, ...)         REPEAT_##X(FN, DL, __VA_ARGS__)
>
>  #define SARG(X)                u64 arg##X
> -#define COPY(X)                args[X] = arg##X
> +#define COPY(X)                args[X + 1] = arg##X
>
>  #define __DL_COM       (,)
>  #define __DL_SEM       (;)
> @@ -2323,9 +2333,10 @@ void __bpf_trace_run(struct bpf_raw_tp_link *link, u64 *args)
>         void bpf_trace_run##x(struct bpf_raw_tp_link *link,             \
>                               REPEAT(x, SARG, __DL_COM, __SEQ_0_11))    \
>         {                                                               \
> -               u64 args[x];                                            \
> +               u64 args[x + 1];                                        \
> +               args[0] = x;                                            \
>                 REPEAT(x, COPY, __DL_SEM, __SEQ_0_11);                  \
> -               __bpf_trace_run(link, args);                            \
> +               __bpf_trace_run(link, args + 1);                        \

This is neat, but what is this for?
The program that attaches to a particular raw_tp knows what it is
attaching to and how many arguments are there,
so bpf_get_func_arg_cnt() is a 5th wheel.

If the reason is "for completeness" then it's not a good reason
to penalize performance. Though it's just an extra 8 byte of stack
and a single store of a constant.

pw-bot: cr

Powered by blists - more mailing lists