On Tue, Feb 8, 2022 at 3:26 PM Ilya Leoshkevich <iii@xxxxxxxxxxxxx> wrote: > > On Tue, 2022-02-08 at 14:08 -0800, Andrii Nakryiko wrote: > > On Mon, Feb 7, 2022 at 9:16 PM Ilya Leoshkevich <iii@xxxxxxxxxxxxx> > > wrote: > > > > > > Depending on whether or not an arch has ARCH_HAS_SYSCALL_WRAPPER, > > > syscall arguments must be accessed through a different set of > > > registers. Provide PT_REGS_SYSCALL_REGS macro to abstract away > > > that difference. > > > > > > Reported-by: Heiko Carstens <hca@xxxxxxxxxxxxx> > > > Co-developed-by: Naveen N. Rao <naveen.n.rao@xxxxxxxxxxxxxxxxxx> > > > Signed-off-by: Naveen N. Rao <naveen.n.rao@xxxxxxxxxxxxxxxxxx> > > > Signed-off-by: Ilya Leoshkevich <iii@xxxxxxxxxxxxx> > > > --- > > > > Again, there was nothing wrong with the way you did it in v3, please > > revert to that one. > > I've realized that, even though fully correct, v3 looked somewhat > ad-hoc: it defined PT_REGS_SYSCALL_REGS for different architectures > without explaining why this particular arch has this parciular way to > access syscall arguments. > > So I've decided to switch to the existing terminology, as Naveen > proposed [1]: > > - arches that select ARCH_HAS_SYSCALL_WRAPPER in Kconfig get a > __BPF_ARCH_HAS_SYSCALL_WRAPPER in bpf_tracing.h > > - syscall handler calling convention is (at least partially) determined > by whether or not an arch has a sycall wrapper as described in > ARCH_HAS_SYSCALL_WRAPPER help text > > I can, of course, switch back to v3 - both patches look functionally > identical - but this one seems to be a bit easier to understand. > > [1] > https://lore.kernel.org/bpf/1643991537.bfyv1b2oym.naveen@xxxxxxxxxxxxx/#t > > > > > > tools/lib/bpf/bpf_tracing.h | 17 +++++++++++++++++ > > > 1 file changed, 17 insertions(+) > > > > > > diff --git a/tools/lib/bpf/bpf_tracing.h > > > b/tools/lib/bpf/bpf_tracing.h > > > index 82f1e935d549..7a015ee8fb11 100644 > > > --- a/tools/lib/bpf/bpf_tracing.h > > > +++ b/tools/lib/bpf/bpf_tracing.h > > > @@ -64,6 +64,8 @@ > > > > > > #if defined(bpf_target_x86) > > > > > > +#define __BPF_ARCH_HAS_SYSCALL_WRAPPER > > > + > > > #if defined(__KERNEL__) || defined(__VMLINUX_H__) > > > > > > #define __PT_PARM1_REG di > > > @@ -114,6 +116,8 @@ > > > > > > #elif defined(bpf_target_s390) > > > > > > +#define __BPF_ARCH_HAS_SYSCALL_WRAPPER > > > + > > > /* s390 provides user_pt_regs instead of struct pt_regs to > > > userspace */ > > > #define __PT_REGS_CAST(x) ((const user_pt_regs *)(x)) > > > #define __PT_PARM1_REG gprs[2] > > > @@ -142,6 +146,8 @@ > > > > > > #elif defined(bpf_target_arm64) > > > > > > +#define __BPF_ARCH_HAS_SYSCALL_WRAPPER > > > + > > > /* arm64 provides struct user_pt_regs instead of struct pt_regs to > > > userspace */ > > > #define __PT_REGS_CAST(x) ((const struct user_pt_regs *)(x)) > > > #define __PT_PARM1_REG regs[0] > > > @@ -344,6 +350,17 @@ struct pt_regs; > > > > > > #endif /* defined(bpf_target_defined) */ > > > > > > +/* > > > + * When invoked from a syscall handler BPF_KPROBE, returns a > > > pointer to a > > > + * struct pt_regs containing syscall arguments, that is suitable > > > for passing to > > > + * PT_REGS_PARMn_SYSCALL() and PT_REGS_PARMn_CORE_SYSCALL(). You can mention ARCH_HAS_SYSCALL_WRAPPER here for documentation purposes. I like the previous approach because it clearly shows which architectures deviate from "common" behavior (whatever "common" we chose as the default). With __BPF_ARCH_HAS_SYSCALL_WRAPPER I'll go and start grepping what else depends on that, etc. Also, ARCH_HAS_SYSCALL_WRAPPER can change over time, so it depends on kernel version just as much as architecture (which with CO-RE we should be able to handle transparently, btw). Anyways, the previous one looks cleaner and easier to follow to me, please use the previous version. > > > + */ > > > +#ifdef __BPF_ARCH_HAS_SYSCALL_WRAPPER > > > +#define PT_REGS_SYSCALL_REGS(ctx) ((struct pt_regs > > > *)PT_REGS_PARM1(ctx)) > > > +#else > > > +#define PT_REGS_SYSCALL_REGS(ctx) ctx > > > +#endif > > > + > > > #ifndef ___bpf_concat > > > #define ___bpf_concat(a, b) a ## b > > > #endif > > > -- > > > 2.34.1 > > > >
