| Message ID | 20220204041955.1958263-6-iii@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | libbpf: Fix accessing syscall arguments | expand |
On Thu, Feb 3, 2022 at 8:20 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote: > > Some architectures pass a pointer to struct pt_regs to syscall > handlers, others unpack it into individual function parameters. > Introduce a macro to describe what a particular arch does, using > `passing pt_regs *` as a default. > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > --- > tools/lib/bpf/bpf_tracing.h | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h > index 30f0964f8c9e..400a4f002f77 100644 > --- a/tools/lib/bpf/bpf_tracing.h > +++ b/tools/lib/bpf/bpf_tracing.h > @@ -334,6 +334,15 @@ struct pt_regs; > > #endif /* defined(bpf_target_defined) */ > > +/* > + * When invoked from a syscall handler kprobe, returns a pointer to a > + * struct pt_regs containing syscall arguments and suitable for passing to > + * PT_REGS_PARMn_SYSCALL() and PT_REGS_PARMn_CORE_SYSCALL(). > + */ > +#ifndef PT_REGS_SYSCALL > +#define PT_REGS_SYSCALL(ctx) ((struct pt_regs *)PT_REGS_PARM1(ctx)) > +#endif maybe PT_REGS_SYSCALL_REGS? It returns regs, not the "syscall". PT_REGS prefix is for consistency with all other pt_regs macros, but "SYSCALL_REGS" is specifying what is actually returned by the macro > + > #ifndef ___bpf_concat > #define ___bpf_concat(a, b) a ## b > #endif > -- > 2.34.1 >
On Thu, Feb 3, 2022 at 9:22 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Thu, Feb 3, 2022 at 8:20 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote: > > > > Some architectures pass a pointer to struct pt_regs to syscall > > handlers, others unpack it into individual function parameters. > > Introduce a macro to describe what a particular arch does, using > > `passing pt_regs *` as a default. > > > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > > --- > > tools/lib/bpf/bpf_tracing.h | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h > > index 30f0964f8c9e..400a4f002f77 100644 > > --- a/tools/lib/bpf/bpf_tracing.h > > +++ b/tools/lib/bpf/bpf_tracing.h > > @@ -334,6 +334,15 @@ struct pt_regs; > > > > #endif /* defined(bpf_target_defined) */ > > > > +/* > > + * When invoked from a syscall handler kprobe, returns a pointer to a > > + * struct pt_regs containing syscall arguments and suitable for passing to > > + * PT_REGS_PARMn_SYSCALL() and PT_REGS_PARMn_CORE_SYSCALL(). > > + */ > > +#ifndef PT_REGS_SYSCALL > > +#define PT_REGS_SYSCALL(ctx) ((struct pt_regs *)PT_REGS_PARM1(ctx)) > > +#endif > > maybe PT_REGS_SYSCALL_REGS? It returns regs, not the "syscall". > PT_REGS prefix is for consistency with all other pt_regs macros, but > "SYSCALL_REGS" is specifying what is actually returned by the macro > Oh, and instead of casting to `struct pt_regs *` directly, maybe use __PT_REGS_CAST() instead? For some architectures it probably should stay user_pt_regs (or whatever it is there). > > + > > #ifndef ___bpf_concat > > #define ___bpf_concat(a, b) a ## b > > #endif > > -- > > 2.34.1 > >
On Thu, 2022-02-03 at 21:23 -0800, Andrii Nakryiko wrote: > On Thu, Feb 3, 2022 at 9:22 PM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Thu, Feb 3, 2022 at 8:20 PM Ilya Leoshkevich <iii@linux.ibm.com> > > wrote: > > > > > > Some architectures pass a pointer to struct pt_regs to syscall > > > handlers, others unpack it into individual function parameters. > > > Introduce a macro to describe what a particular arch does, using > > > `passing pt_regs *` as a default. > > > > > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > > > --- > > > tools/lib/bpf/bpf_tracing.h | 9 +++++++++ > > > 1 file changed, 9 insertions(+) > > > > > > diff --git a/tools/lib/bpf/bpf_tracing.h > > > b/tools/lib/bpf/bpf_tracing.h > > > index 30f0964f8c9e..400a4f002f77 100644 > > > --- a/tools/lib/bpf/bpf_tracing.h > > > +++ b/tools/lib/bpf/bpf_tracing.h > > > @@ -334,6 +334,15 @@ struct pt_regs; > > > > > > #endif /* defined(bpf_target_defined) */ > > > > > > +/* > > > + * When invoked from a syscall handler kprobe, returns a pointer > > > to a > > > + * struct pt_regs containing syscall arguments and suitable for > > > passing to > > > + * PT_REGS_PARMn_SYSCALL() and PT_REGS_PARMn_CORE_SYSCALL(). > > > + */ > > > +#ifndef PT_REGS_SYSCALL > > > +#define PT_REGS_SYSCALL(ctx) ((struct pt_regs > > > *)PT_REGS_PARM1(ctx)) > > > +#endif > > > > maybe PT_REGS_SYSCALL_REGS? It returns regs, not the "syscall". > > PT_REGS prefix is for consistency with all other pt_regs macros, > > but > > "SYSCALL_REGS" is specifying what is actually returned by the macro > > > > Oh, and instead of casting to `struct pt_regs *` directly, maybe use > __PT_REGS_CAST() instead? For some architectures it probably should > stay user_pt_regs (or whatever it is there). > > > > + > > > #ifndef ___bpf_concat > > > #define ___bpf_concat(a, b) a ## b > > > #endif > > > -- > > > 2.34.1 > > > I think it's better to keep this as struct pt_regs *, so that in bpf progs we can do struct pt_regs *real_regs = PT_REGS_SYSCALL(ctx); without having to worry about which arch we are on, or using the opaque void *.
On Fri, Feb 4, 2022 at 4:30 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote: > > On Thu, 2022-02-03 at 21:23 -0800, Andrii Nakryiko wrote: > > On Thu, Feb 3, 2022 at 9:22 PM Andrii Nakryiko > > <andrii.nakryiko@gmail.com> wrote: > > > > > > On Thu, Feb 3, 2022 at 8:20 PM Ilya Leoshkevich <iii@linux.ibm.com> > > > wrote: > > > > > > > > Some architectures pass a pointer to struct pt_regs to syscall > > > > handlers, others unpack it into individual function parameters. > > > > Introduce a macro to describe what a particular arch does, using > > > > `passing pt_regs *` as a default. > > > > > > > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > > > > --- > > > > tools/lib/bpf/bpf_tracing.h | 9 +++++++++ > > > > 1 file changed, 9 insertions(+) > > > > > > > > diff --git a/tools/lib/bpf/bpf_tracing.h > > > > b/tools/lib/bpf/bpf_tracing.h > > > > index 30f0964f8c9e..400a4f002f77 100644 > > > > --- a/tools/lib/bpf/bpf_tracing.h > > > > +++ b/tools/lib/bpf/bpf_tracing.h > > > > @@ -334,6 +334,15 @@ struct pt_regs; > > > > > > > > #endif /* defined(bpf_target_defined) */ > > > > > > > > +/* > > > > + * When invoked from a syscall handler kprobe, returns a pointer > > > > to a > > > > + * struct pt_regs containing syscall arguments and suitable for > > > > passing to > > > > + * PT_REGS_PARMn_SYSCALL() and PT_REGS_PARMn_CORE_SYSCALL(). > > > > + */ > > > > +#ifndef PT_REGS_SYSCALL > > > > +#define PT_REGS_SYSCALL(ctx) ((struct pt_regs > > > > *)PT_REGS_PARM1(ctx)) > > > > +#endif > > > > > > maybe PT_REGS_SYSCALL_REGS? It returns regs, not the "syscall". > > > PT_REGS prefix is for consistency with all other pt_regs macros, > > > but > > > "SYSCALL_REGS" is specifying what is actually returned by the macro > > > > > > > Oh, and instead of casting to `struct pt_regs *` directly, maybe use > > __PT_REGS_CAST() instead? For some architectures it probably should > > stay user_pt_regs (or whatever it is there). > > > > > > + > > > > #ifndef ___bpf_concat > > > > #define ___bpf_concat(a, b) a ## b > > > > #endif > > > > -- > > > > 2.34.1 > > > > > > I think it's better to keep this as struct pt_regs *, so that in > bpf progs we can do > > struct pt_regs *real_regs = PT_REGS_SYSCALL(ctx); > > without having to worry about which arch we are on, or using the > opaque void *. Makes sense, sounds good to me.
diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h index 30f0964f8c9e..400a4f002f77 100644 --- a/tools/lib/bpf/bpf_tracing.h +++ b/tools/lib/bpf/bpf_tracing.h @@ -334,6 +334,15 @@ struct pt_regs; #endif /* defined(bpf_target_defined) */ +/* + * When invoked from a syscall handler kprobe, returns a pointer to a + * struct pt_regs containing syscall arguments and suitable for passing to + * PT_REGS_PARMn_SYSCALL() and PT_REGS_PARMn_CORE_SYSCALL(). + */ +#ifndef PT_REGS_SYSCALL +#define PT_REGS_SYSCALL(ctx) ((struct pt_regs *)PT_REGS_PARM1(ctx)) +#endif + #ifndef ___bpf_concat #define ___bpf_concat(a, b) a ## b #endif
Some architectures pass a pointer to struct pt_regs to syscall handlers, others unpack it into individual function parameters. Introduce a macro to describe what a particular arch does, using `passing pt_regs *` as a default. Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> --- tools/lib/bpf/bpf_tracing.h | 9 +++++++++ 1 file changed, 9 insertions(+)