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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

>>  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



[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux