| Message ID | 20211221055312.3371414-2-hengqi.chen@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | libbpf: Add syscall-specific variants of BPF_KPROBE/BPF_KRETPROBE | expand |
On 12/20/21 9:53 PM, Hengqi Chen 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> > --- > 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) We probably need to use a syscall variant of PT_REGS_PARAM4 here, see https://lore.kernel.org/bpf/TYCPR01MB59360988D96E23FBA97DAE0AF57C9@TYCPR01MB5936.jpnprd01.prod.outlook.com/ > +#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) > + > +/* > + * BPF_KPROBE_SYSCALL is a variant of BPF_KPROBE, which is intended for > + * tracing syscall functions. It hides the underlying platform-specific > + * 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. > + * > + * 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); \ > + 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) > + > +/* > + * 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 > + > #endif > -- > 2.30.2
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
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@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). > 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
On Thu, Dec 23, 2021 at 4:11 AM Hengqi Chen <hengqi.chen@gmail.com> 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@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). > > > > 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
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) + +/* + * BPF_KPROBE_SYSCALL is a variant of BPF_KPROBE, which is intended for + * tracing syscall functions. It hides the underlying platform-specific + * 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. + * + * 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); \ + 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) + +/* + * 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 + #endif
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> --- tools/lib/bpf/bpf_tracing.h | 45 +++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) -- 2.30.2