Re: [PATCH bpf-next 1/2] libbpf: Add BPF_KPROBE_SYSCALL/BPF_KRETPROBE_SYSCALL macros

From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Hengqi Chen <hengqi.chen@gmail.com>
Cc: bpf <bpf@vger.kernel.org>, Andrii Nakryiko <andrii@kernel.org>
Subject: Re: [PATCH bpf-next 1/2] libbpf: Add BPF_KPROBE_SYSCALL/BPF_KRETPROBE_SYSCALL macros
Date: Tue, 21 Dec 2021 16:18:18 -0800	[thread overview]
Message-ID: <CAEf4BzZQy-KUd8D4jj0Th2Po4d8UbQL7xnywRcF3xwy99+127g@mail.gmail.com> (raw)
In-Reply-To: <20211221055312.3371414-2-hengqi.chen@gmail.com>

On Mon, Dec 20, 2021 at 9:53 PM Hengqi Chen <hengqi.chen@gmail.com> wrote:
>
> Add syscall-specific variants of BPF_KPROBE/BPF_KRETPROBE named
> BPF_KPROBE_SYSCALL/BPF_KRETPROBE_SYSCALL ([0]). These new macros
> hide the underlying way of getting syscall input arguments and
> return values. With these new macros, the following code:
>
>     SEC("kprobe/__x64_sys_close")
>     int BPF_KPROBE(do_sys_close, struct pt_regs *regs)
>     {
>         int fd;
>
>         fd = PT_REGS_PARM1_CORE(regs);
>         /* do something with fd */
>     }
>
> can be written as:
>
>     SEC("kprobe/__x64_sys_close")
>     int BPF_KPROBE_SYSCALL(do_sys_close, int fd)
>     {
>         /* do something with fd */
>     }
>
>   [0] Closes: https://github.com/libbpf/libbpf/issues/425
>
> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
> ---

As Yonghong mentioned, let's wait for PT_REGS_PARMx_SYSCALL macros to
land and use those (due to 4th argument quirkiness on x86 arches).

>  tools/lib/bpf/bpf_tracing.h | 45 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
>
> diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
> index db05a5937105..eb4b567e443f 100644
> --- a/tools/lib/bpf/bpf_tracing.h
> +++ b/tools/lib/bpf/bpf_tracing.h
> @@ -489,4 +489,49 @@ typeof(name(0)) name(struct pt_regs *ctx)                              \
>  }                                                                          \
>  static __always_inline typeof(name(0)) ____##name(struct pt_regs *ctx, ##args)
>
> +#define ___bpf_syscall_args0() ctx, regs
> +#define ___bpf_syscall_args1(x) \
> +       ___bpf_syscall_args0(), (void *)PT_REGS_PARM1_CORE(regs)
> +#define ___bpf_syscall_args2(x, args...) \
> +       ___bpf_syscall_args1(args), (void *)PT_REGS_PARM2_CORE(regs)
> +#define ___bpf_syscall_args3(x, args...) \
> +       ___bpf_syscall_args2(args), (void *)PT_REGS_PARM3_CORE(regs)
> +#define ___bpf_syscall_args4(x, args...) \
> +       ___bpf_syscall_args3(args), (void *)PT_REGS_PARM4_CORE(regs)
> +#define ___bpf_syscall_args5(x, args...) \
> +       ___bpf_syscall_args4(args), (void *)PT_REGS_PARM5_CORE(regs)
> +#define ___bpf_syscall_args(args...) \
> +       ___bpf_apply(___bpf_syscall_args, ___bpf_narg(args))(args)

try keeping each definition on a single line, make them much more
readable and I think still fits in 100 character limit

> +
> +/*
> + * BPF_KPROBE_SYSCALL is a variant of BPF_KPROBE, which is intended for
> + * tracing syscall functions. It hides the underlying platform-specific

let's add a simple example to explain what kind of tracing syscall
functions we mean.

"tracing syscall functions, like __x64_sys_close." ?

> + * low-level way of getting syscall input arguments from struct pt_regs, and
> + * provides a familiar typed and named function arguments syntax and
> + * semantics of accessing syscall input paremeters.

typo: parameters

> + *
> + * Original struct pt_regs* context is preserved as 'ctx' argument. This might
> + * be necessary when using BPF helpers like bpf_perf_event_output().
> + */
> +#define BPF_KPROBE_SYSCALL(name, args...)                                  \
> +name(struct pt_regs *ctx);                                                 \
> +static __attribute__((always_inline)) typeof(name(0))                      \
> +____##name(struct pt_regs *ctx, struct pt_regs *regs, ##args);             \
> +typeof(name(0)) name(struct pt_regs *ctx)                                  \
> +{                                                                          \
> +       _Pragma("GCC diagnostic push")                                      \
> +       _Pragma("GCC diagnostic ignored \"-Wint-conversion\"")              \
> +       struct pt_regs *regs = PT_REGS_PARM1(ctx);                          \

please move it out of _Pragma region, no need to guard it

> +       return ____##name(___bpf_syscall_args(args));                       \
> +       _Pragma("GCC diagnostic pop")                                       \
> +}                                                                          \
> +static __attribute__((always_inline)) typeof(name(0))                      \
> +____##name(struct pt_regs *ctx, struct pt_regs *regs, ##args)

I don't think we need to add another magical hidden argument "regs".
Anyone who will need it for something can get it from the hidden ctx
with PT_REGS_PARM1(ctx) anyways.

> +
> +/*
> + * BPF_KRETPROBE_SYSCALL is just an alias to BPF_KRETPROBE,
> + * it provides optional return value (in addition to `struct pt_regs *ctx`)
> + */
> +#define BPF_KRETPROBE_SYSCALL BPF_KRETPROBE
> +

hm... do we even need BPF_KRETPROBE_SYSCALL then? Let's drop it, it
doesn't provide much value, just creates a confusion.

>  #endif
> --
> 2.30.2