On Thu, Dec 23, 2021 at 4:11 AM Hengqi Chen <hengqi.chen@xxxxxxxxx> wrote:
>
> Hi, Andrii
>
> On 2021/12/22 8:18 AM, Andrii Nakryiko wrote:
> > On Mon, Dec 20, 2021 at 9:53 PM Hengqi Chen <hengqi.chen@xxxxxxxxx> 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@xxxxxxxxx>
> >> ---
> >
> > 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).
> >
>
> I see those patches, will wait.
They got merged, feel free to resubmit.
>
> >> 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
> >
>
> This should be addressed by your patch, will build on top of it.
>
> >> +
> >> +/*
> >> + * 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
> >
>
> Ack.
>
> >> + *
> >> + * 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
> >
>
> Ack.
>
> >> + 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.
> >
>
> Yes, this should be removed, otherwise it may conflict with user-defined args.
>
> >> +
> >> +/*
> >> + * 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.
> >
>
> OK, will drop it.
>
> >
> >> #endif
> >> --
> >> 2.30.2
>
> ---
> Hengqi
