Re: [PATCH 11/19] bpf: Add support to load multi func tracing program

Yonghong Song
2021-06-07T03:58:56Z

On 6/5/21 4:10 AM, Jiri Olsa wrote:
> Adding support to load tracing program with new BPF_F_MULTI_FUNC flag,
> that allows the program to be loaded without specific function to be
> attached to.
>
> The verifier assumes the program is using all (6) available arguments

Is this a verifier failure or it is due to the check in the
beginning of function arch_prepare_bpf_trampoline()?

/* x86-64 supports up to 6 arguments. 7+ can be added in the
future */
if (nr_args > 6)
return -ENOTSUPP;

If it is indeed due to arch_prepare_bpf_trampoline() maybe we
can improve it instead of specially processing the first argument
"ip" in quite some places?

> as unsigned long values. We can't add extra ip argument at this time,
> because JIT on x86 would fail to process this function. Instead we
> allow to access extra first 'ip' argument in btf_ctx_access.
>
> Such program will be allowed to be attached to multiple functions
> in following patches.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> include/linux/bpf.h | 1 +
> include/uapi/linux/bpf.h | 7 +++++++
> kernel/bpf/btf.c | 5 +++++
> kernel/bpf/syscall.c | 35 +++++++++++++++++++++++++++++-----
> kernel/bpf/verifier.c | 3 ++-
> tools/include/uapi/linux/bpf.h | 7 +++++++
> 6 files changed, 52 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 6cbf3c81c650..23221e0e8d3c 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -845,6 +845,7 @@ struct bpf_prog_aux {
> bool sleepable;
> bool tail_call_reachable;
> struct hlist_node tramp_hlist;
> + bool multi_func;

Move this field right after "tail_call_reachable"?

> /* BTF_KIND_FUNC_PROTO for valid attach_btf_id */
> const struct btf_type *attach_func_proto;
> /* function name for valid attach_btf_id */
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 2c1ba70abbf1..ad9340fb14d4 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -1109,6 +1109,13 @@ enum bpf_link_type {
> */
> #define BPF_F_SLEEPABLE (1U << 4)
>
> +/* If BPF_F_MULTI_FUNC is used in BPF_PROG_LOAD command, the verifier does
> + * not expect BTF ID for the program, instead it assumes it's function
> + * with 6 u64 arguments. No trampoline is created for the program. Such
> + * program can be attached to multiple functions.
> + */
> +#define BPF_F_MULTI_FUNC (1U << 5)
> +
> /* When BPF ldimm64's insn[0].src_reg != 0 then this can have
> * the following extensions:
> *
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index a6e39c5ea0bf..c233aaa6a709 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -4679,6 +4679,11 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
> args++;
> nr_args--;
> }
> + if (prog->aux->multi_func) {
> + if (arg == 0)
> + return true;
> + arg--;

Some comments in the above to mention like "the first 'ip' argument
is omitted" will be good.

> + }
>
> if (arg > nr_args) {
> bpf_log(log, "func '%s' doesn't have %d-th argument\n", …