RE: [PATCH] libbpf: Fix the incorrect register read for syscalls on x86_64

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_64

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 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.

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:

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.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]

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.

Also, could you add a selftest to use this macro, esp. for parameter 4?

+1