On Tue, Dec 21, 2021 at 10:20 PM <Kenta.Tada@xxxxxxxx> wrote: > > >> > >> Also, could you add a selftest to use this macro, esp. for parameter 4? > > I misunderstood this comment. > I'll just add a test of this new macro. > > -----Original Message----- > From: Tada, Kenta (SGC) > Sent: Wednesday, December 22, 2021 2:52 PM > To: Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx>; Yonghong Song <yhs@xxxxxx> > Cc: Andrii Nakryiko <andrii@xxxxxxxxxx>; bpf <bpf@xxxxxxxxxxxxxxx>; Alexei Starovoitov <ast@xxxxxxxxxx>; Daniel Borkmann <daniel@xxxxxxxxxxxxx>; Martin Lau <kafai@xxxxxx>; Song Liu <songliubraving@xxxxxx>; john fastabend <john.fastabend@xxxxxxxxx>; KP Singh <kpsingh@xxxxxxxxxx> > Subject: RE: [PATCH] libbpf: Fix the incorrect register read for syscalls on x86_64 > > >> Looks like macros only available for x86_64. Can we make it also > >> available for other architectures so we won't introduce arch specific > >> codes into bpf program? > > > >Yeah, but instead of copy/pasting it for each architecture, let's > >define PT_REGS_PARM4/PT_REGS_PARM4_CORE for x86-64 (is this the only > >arch with such inconsistency?) and then after all the architectures > >defined their macro define > > Currently, I think x86_64 is the only arch which causes this issue. > But I'll add #ifndef to not only the 4th parameter but also all parameters for future extensibility. Let's not add unnecessary #ifndefs. If we get a case for other args, we'll add #ifndefs as necessary. But after looking at your and Hengqi's patches in the last few days, I've looked at the current state of bpf_tracing.h and felt like there is too much repetition. So I've refactored it to not require as many similar macro definitions. I'm going to finish it up and submit it tomorrow, so please hold on a bit with your additions until then so that you can base it off the refactored bpf_tracing.h. Thanks. > > >> > >> Also, could you add a selftest to use this macro, esp. for parameter 4? > > Sure. > I'll add the same macro to bpf_tracing.h in selftests. > > Thanks! > > -----Original Message----- > From: Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> > Sent: Wednesday, December 22, 2021 7:58 AM > To: Yonghong Song <yhs@xxxxxx> > Cc: Tada, Kenta (SGC) <Kenta.Tada@xxxxxxxx>; Andrii Nakryiko <andrii@xxxxxxxxxx>; bpf <bpf@xxxxxxxxxxxxxxx>; Alexei Starovoitov <ast@xxxxxxxxxx>; Daniel Borkmann <daniel@xxxxxxxxxxxxx>; Martin Lau <kafai@xxxxxx>; Song Liu <songliubraving@xxxxxx>; john fastabend <john.fastabend@xxxxxxxxx>; KP Singh <kpsingh@xxxxxxxxxx> > Subject: Re: [PATCH] libbpf: Fix the incorrect register read for syscalls on x86_64 > > On Tue, Dec 21, 2021 at 7:51 AM Yonghong Song <yhs@xxxxxx> wrote: > > > > > > > > On 12/21/21 3:21 AM, Kenta.Tada@xxxxxxxx wrote: > > > Currently, rcx is read as the fourth parameter of syscall on x86_64. > > > But x86_64 Linux System Call convention uses r10 actually. > > > This commit adds the wrapper for users who want to access to syscall > > > params to analyze the user space. > > > > > > Signed-off-by: Kenta Tada <Kenta.Tada@xxxxxxxx> > > > --- > > > tools/lib/bpf/bpf_tracing.h | 20 ++++++++++++++++++++ > > > 1 file changed, 20 insertions(+) > > > > > > diff --git a/tools/lib/bpf/bpf_tracing.h > > > b/tools/lib/bpf/bpf_tracing.h index db05a5937105..f6fcccd9b10c > > > 100644 > > > --- a/tools/lib/bpf/bpf_tracing.h > > > +++ b/tools/lib/bpf/bpf_tracing.h > > > @@ -67,10 +67,15 @@ > > > #if defined(__KERNEL__) || defined(__VMLINUX_H__) > > > > > > #define PT_REGS_PARM1(x) ((x)->di) > > > +#define PT_REGS_PARM1_SYSCALL(x) PT_REGS_PARM1(x) > > > #define PT_REGS_PARM2(x) ((x)->si) > > > +#define PT_REGS_PARM2_SYSCALL(x) PT_REGS_PARM2(x) > > > #define PT_REGS_PARM3(x) ((x)->dx) > > > +#define PT_REGS_PARM3_SYSCALL(x) PT_REGS_PARM3(x) > > > #define PT_REGS_PARM4(x) ((x)->cx) > > > +#define PT_REGS_PARM4_SYSCALL(x) ((x)->r10) /* syscall uses r10 */ > > > > I think this is correct. We have a bcc commit doing similar thing. > > https://urldefense.com/v3/__https://github.com/iovisor/bcc/commit/c234 > > 48e34ecd3cc9bfc19f0b43f4325f77c2e4cc*diff-c78ffb58f59e85eaba9bf9977b72 > > 02f3e50f17e2a9ee556c36a311f9a9ab5d6e__;Iw!!JmoZiZGBv3RvKRSx!qa2pOQlUSd > > WfoxmZ7EuPvZpWAlK5IBVP1eC-2d3njBLe3yehAXyV_0wI9mGRF5Q$ [github[.]com] > > > > > #define PT_REGS_PARM5(x) ((x)->r8) > > > +#define PT_REGS_PARM5_SYSCALL(x) PT_REGS_PARM5(x) > > > #define PT_REGS_RET(x) ((x)->sp) > > > #define PT_REGS_FP(x) ((x)->bp) > > > #define PT_REGS_RC(x) ((x)->ax) > > > @@ -78,10 +83,15 @@ > > > #define PT_REGS_IP(x) ((x)->ip) > > > > > > #define PT_REGS_PARM1_CORE(x) BPF_CORE_READ((x), di) > > > +#define PT_REGS_PARM1_CORE_SYSCALL(x) PT_REGS_PARM1_CORE(x) > > > #define PT_REGS_PARM2_CORE(x) BPF_CORE_READ((x), si) > > > +#define PT_REGS_PARM2_CORE_SYSCALL(x) PT_REGS_PARM2_CORE(x) > > > #define PT_REGS_PARM3_CORE(x) BPF_CORE_READ((x), dx) > > > +#define PT_REGS_PARM3_CORE_SYSCALL(x) PT_REGS_PARM3_CORE(x) > > > #define PT_REGS_PARM4_CORE(x) BPF_CORE_READ((x), cx) > > > +#define PT_REGS_PARM4_CORE_SYSCALL(x) BPF_CORE_READ((x), r10) /* > > > +syscall uses r10 */ > > > #define PT_REGS_PARM5_CORE(x) BPF_CORE_READ((x), r8) > > > +#define PT_REGS_PARM5_CORE_SYSCALL(x) PT_REGS_PARM5_CORE(x) > > > #define PT_REGS_RET_CORE(x) BPF_CORE_READ((x), sp) > > > #define PT_REGS_FP_CORE(x) BPF_CORE_READ((x), bp) > > > #define PT_REGS_RC_CORE(x) BPF_CORE_READ((x), ax) @@ -117,10 > > > +127,15 @@ > > > #else > > > > > > #define PT_REGS_PARM1(x) ((x)->rdi) > > > +#define PT_REGS_PARM1_SYSCALL(x) PT_REGS_PARM1(x) > > > #define PT_REGS_PARM2(x) ((x)->rsi) > > > +#define PT_REGS_PARM2_SYSCALL(x) PT_REGS_PARM2(x) > > > #define PT_REGS_PARM3(x) ((x)->rdx) > > > +#define PT_REGS_PARM3_SYSCALL(x) PT_REGS_PARM3(x) > > > #define PT_REGS_PARM4(x) ((x)->rcx) > > > +#define PT_REGS_PARM4_SYSCALL(x) ((x)->r10) /* syscall uses r10 */ > > > #define PT_REGS_PARM5(x) ((x)->r8) > > > +#define PT_REGS_PARM5(x) PT_REGS_PARM5(x) > > > #define PT_REGS_RET(x) ((x)->rsp) > > > #define PT_REGS_FP(x) ((x)->rbp) > > > #define PT_REGS_RC(x) ((x)->rax) > > > @@ -128,10 +143,15 @@ > > > #define PT_REGS_IP(x) ((x)->rip) > > > > > > #define PT_REGS_PARM1_CORE(x) BPF_CORE_READ((x), rdi) > > > +#define PT_REGS_PARM1_CORE_SYSCALL(x) PT_REGS_PARM1_CORE(x) > > > #define PT_REGS_PARM2_CORE(x) BPF_CORE_READ((x), rsi) > > > +#define PT_REGS_PARM2_CORE_SYSCALL(x) PT_REGS_PARM2_CORE(x) > > > #define PT_REGS_PARM3_CORE(x) BPF_CORE_READ((x), rdx) > > > +#define PT_REGS_PARM3_CORE_SYSCALL(x) PT_REGS_PARM3_CORE(x) > > > #define PT_REGS_PARM4_CORE(x) BPF_CORE_READ((x), rcx) > > > +#define PT_REGS_PARM4_CORE_SYSCALL(x) BPF_CORE_READ((x), r10) /* > > > +syscall uses r10 */ > > > #define PT_REGS_PARM5_CORE(x) BPF_CORE_READ((x), r8) > > > +#define PT_REGS_PARM5_CORE_SYSCALL(x) PT_REGS_PARM5_CORE(x) > > > #define PT_REGS_RET_CORE(x) BPF_CORE_READ((x), rsp) > > > #define PT_REGS_FP_CORE(x) BPF_CORE_READ((x), rbp) > > > #define PT_REGS_RC_CORE(x) BPF_CORE_READ((x), rax) > > > > Looks like macros only available for x86_64. Can we make it also > > available for other architectures so we won't introduce arch specific > > codes into bpf program? > > Yeah, but instead of copy/pasting it for each architecture, let's define PT_REGS_PARM4/PT_REGS_PARM4_CORE for x86-64 (is this the only arch with such inconsistency?) and then after all the architectures defined their macro define > > #define PT_REGS_PARM1_SYSCALL(x) PT_REGS_PARM1(x) ... > #ifndef PT_REGS_PARM4_SYSCALL(x) > #define PT_REGS_PARM4_SYSCALL(x) PT_REGS_PARM4(x) #endif > > That way we'll avoid all the extra "no-op" definitions. > > > > > > Also, could you add a selftest to use this macro, esp. for parameter 4? > > +1
