Re: [PATCH] libbpf: Fix the incorrect register read for syscalls on x86_64
From: Andrii Nakryiko <hidden>
Date: 2021-12-22 06:47:38
On Tue, Dec 21, 2021 at 10:20 PM [off-list ref] wrote:
quoted
quoted
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 <redacted>; Yonghong Song <redacted> Cc: Andrii Nakryiko <andrii@kernel.org>; bpf <redacted>; Alexei Starovoitov <ast@kernel.org>; Daniel Borkmann <daniel@iogearbox.net>; Martin Lau <redacted>; Song Liu <redacted>; john fastabend <john.fastabend@gmail.com>; KP Singh <kpsingh@kernel.org> Subject: RE: [PATCH] libbpf: Fix the incorrect register read for syscalls on x86_64quoted
quoted
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 defineCurrently, 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.
quoted
quoted
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 <redacted> Sent: Wednesday, December 22, 2021 7:58 AM To: Yonghong Song <redacted> Cc: Tada, Kenta (SGC) <redacted>; Andrii Nakryiko <andrii@kernel.org>; bpf <redacted>; Alexei Starovoitov <ast@kernel.org>; Daniel Borkmann <daniel@iogearbox.net>; Martin Lau <redacted>; Song Liu <redacted>; john fastabend <john.fastabend@gmail.com>; KP Singh <kpsingh@kernel.org> 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 [off-list ref] wrote:quoted
On 12/21/21 3:21 AM, Kenta.Tada@sony.com wrote:quoted
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 <redacted> --- tools/lib/bpf/bpf_tracing.h | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)diff --git a/tools/lib/bpf/bpf_tracing.hb/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]quoted
#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.quoted
Also, could you add a selftest to use this macro, esp. for parameter 4?+1