[PATCH bpf-next v3 0/4] Fix accessing first syscall argument on RV64

On RV64, as Ilya mentioned before [0], the first syscall parameter should be
accessed through orig_a0 (see arch/riscv64/include/asm/syscall.h),
otherwise it will cause selftests like bpf_syscall_macro, vmlinux,
test_lsm, etc. to fail on RV64.

Link: https://lore.kernel.org/bpf/20220209021745.2215452-1-iii@linux.ibm.com [0]

v3:
- Fix test case error.

v2: https://lore.kernel.org/all/20240831023646.1558629-1-pulehui@huaweicloud.com/
- Access first syscall argument with CO-RE direct read. (Andrii)

v1: https://lore.kernel.org/all/20240829133453.882259-1-pulehui@huaweicloud.com/

Pu Lehui (4):
  libbpf: Access first syscall argument with CO-RE direct read on s390
  libbpf: Access first syscall argument with CO-RE direct read on arm64
  selftests/bpf: Enable test_bpf_syscall_macro:syscall_arg1 on s390 and
    arm64
  libbpf: Fix accessing first syscall argument on RV64

 tools/lib/bpf/bpf_tracing.h                     | 17 ++++++++++++-----
 .../bpf/prog_tests/test_bpf_syscall_macro.c     |  4 ----
 .../selftests/bpf/progs/bpf_syscall_macro.c     |  2 --
 3 files changed, 12 insertions(+), 11 deletions(-)

-- 
2.34.1

From: Pu Lehui <pulehui@huawei.com>

Considering that CO-RE direct read access to the first system call
argument is already available on s390 and arm64, let's enable
test_bpf_syscall_macro:syscall_arg1 on these architectures.

Signed-off-by: Pu Lehui <pulehui@huawei.com>
---
 .../testing/selftests/bpf/prog_tests/test_bpf_syscall_macro.c | 4 ----
 tools/testing/selftests/bpf/progs/bpf_syscall_macro.c         | 2 --
 2 files changed, 6 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/test_bpf_syscall_macro.c b/tools/testing/selftests/bpf/prog_tests/test_bpf_syscall_macro.c
index 2900c5e9a016..1750c29b94f8 100644
--- a/tools/testing/selftests/bpf/prog_tests/test_bpf_syscall_macro.c
+++ b/tools/testing/selftests/bpf/prog_tests/test_bpf_syscall_macro.c

@@ -38,11 +38,7 @@ void test_bpf_syscall_macro(void)
 	/* check whether args of syscall are copied correctly */
 	prctl(exp_arg1, exp_arg2, exp_arg3, exp_arg4, exp_arg5);
 
-#if defined(__aarch64__) || defined(__s390__)
-	ASSERT_NEQ(skel->bss->arg1, exp_arg1, "syscall_arg1");
-#else
 	ASSERT_EQ(skel->bss->arg1, exp_arg1, "syscall_arg1");
-#endif
 	ASSERT_EQ(skel->bss->arg2, exp_arg2, "syscall_arg2");
 	ASSERT_EQ(skel->bss->arg3, exp_arg3, "syscall_arg3");
 	/* it cannot copy arg4 when uses PT_REGS_PARM4 on x86_64 */

diff --git a/tools/testing/selftests/bpf/progs/bpf_syscall_macro.c b/tools/testing/selftests/bpf/progs/bpf_syscall_macro.c
index 1a476d8ed354..9e7d9674ce2a 100644
--- a/tools/testing/selftests/bpf/progs/bpf_syscall_macro.c
+++ b/tools/testing/selftests/bpf/progs/bpf_syscall_macro.c

@@ -43,9 +43,7 @@ int BPF_KPROBE(handle_sys_prctl)
 
 	/* test for PT_REGS_PARM */
 
-#if !defined(bpf_target_arm64) && !defined(bpf_target_s390)
 	bpf_probe_read_kernel(&tmp, sizeof(tmp), &PT_REGS_PARM1_SYSCALL(real_regs));
-#endif
 	arg1 = tmp;
 	bpf_probe_read_kernel(&arg2, sizeof(arg2), &PT_REGS_PARM2_SYSCALL(real_regs));
 	bpf_probe_read_kernel(&arg3, sizeof(arg3), &PT_REGS_PARM3_SYSCALL(real_regs));

-- 
2.34.1

From: Pu Lehui <pulehui@huawei.com>

Currently PT_REGS_PARM1 SYSCALL(x) is consistent with PT_REGS_PARM1_CORE
SYSCALL(x), which will introduce the overhead of BPF_CORE_READ(), taking
into account the read pt_regs comes directly from the context, let's use
CO-RE direct read to access the first system call argument.

Suggested-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Pu Lehui <pulehui@huawei.com>
---
 tools/lib/bpf/bpf_tracing.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
index 9314fa95f04e..e7d9382efeb3 100644
--- a/tools/lib/bpf/bpf_tracing.h
+++ b/tools/lib/bpf/bpf_tracing.h

@@ -163,7 +163,7 @@
 
 struct pt_regs___s390 {
 	unsigned long orig_gpr2;
-};
+} __attribute__((preserve_access_index));
 
 /* s390 provides user_pt_regs instead of struct pt_regs to userspace */
 #define __PT_REGS_CAST(x) ((const user_pt_regs *)(x))

@@ -179,7 +179,7 @@ struct pt_regs___s390 {
 #define __PT_PARM4_SYSCALL_REG __PT_PARM4_REG
 #define __PT_PARM5_SYSCALL_REG __PT_PARM5_REG
 #define __PT_PARM6_SYSCALL_REG gprs[7]
-#define PT_REGS_PARM1_SYSCALL(x) PT_REGS_PARM1_CORE_SYSCALL(x)
+#define PT_REGS_PARM1_SYSCALL(x) (((const struct pt_regs___s390 *)(x))->orig_gpr2)
 #define PT_REGS_PARM1_CORE_SYSCALL(x) \
 	BPF_CORE_READ((const struct pt_regs___s390 *)(x), __PT_PARM1_SYSCALL_REG)
 

-- 
2.34.1

From: Pu Lehui <pulehui@huawei.com>

Currently PT_REGS_PARM1 SYSCALL(x) is consistent with PT_REGS_PARM1_CORE
SYSCALL(x), which will introduce the overhead of BPF_CORE_READ(), taking
into account the read pt_regs comes directly from the context, let's use
CO-RE direct read to access the first system call argument.

Suggested-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Pu Lehui <pulehui@huawei.com>
---
 tools/lib/bpf/bpf_tracing.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
index e7d9382efeb3..051c408e6aed 100644
--- a/tools/lib/bpf/bpf_tracing.h
+++ b/tools/lib/bpf/bpf_tracing.h

@@ -222,7 +222,7 @@ struct pt_regs___s390 {
 
 struct pt_regs___arm64 {
 	unsigned long orig_x0;
-};
+} __attribute__((preserve_access_index));
 
 /* arm64 provides struct user_pt_regs instead of struct pt_regs to userspace */
 #define __PT_REGS_CAST(x) ((const struct user_pt_regs *)(x))

@@ -241,7 +241,7 @@ struct pt_regs___arm64 {
 #define __PT_PARM4_SYSCALL_REG __PT_PARM4_REG
 #define __PT_PARM5_SYSCALL_REG __PT_PARM5_REG
 #define __PT_PARM6_SYSCALL_REG __PT_PARM6_REG
-#define PT_REGS_PARM1_SYSCALL(x) PT_REGS_PARM1_CORE_SYSCALL(x)
+#define PT_REGS_PARM1_SYSCALL(x) (((const struct pt_regs___arm64 *)(x))->orig_x0)
 #define PT_REGS_PARM1_CORE_SYSCALL(x) \
 	BPF_CORE_READ((const struct pt_regs___arm64 *)(x), __PT_PARM1_SYSCALL_REG)
 

-- 
2.34.1

From: Pu Lehui <pulehui@huawei.com>

On RV64, as Ilya mentioned before [0], the first syscall parameter should be
accessed through orig_a0 (see arch/riscv64/include/asm/syscall.h),
otherwise it will cause selftests like bpf_syscall_macro, vmlinux,
test_lsm, etc. to fail on RV64. Let's fix it by using the struct pt_regs
style CO-RE direct access.

Link: https://lore.kernel.org/bpf/20220209021745.2215452-1-iii@linux.ibm.com [0]
Signed-off-by: Pu Lehui <pulehui@huawei.com>
---
 tools/lib/bpf/bpf_tracing.h | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
index 051c408e6aed..6a4c49d1ca72 100644
--- a/tools/lib/bpf/bpf_tracing.h
+++ b/tools/lib/bpf/bpf_tracing.h

@@ -351,6 +351,10 @@ struct pt_regs___arm64 {
  * https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-cc.adoc#risc-v-calling-conventions
  */
 
+struct pt_regs___riscv {
+	unsigned long orig_a0;
+} __attribute__((preserve_access_index));
+
 /* riscv provides struct user_regs_struct instead of struct pt_regs to userspace */
 #define __PT_REGS_CAST(x) ((const struct user_regs_struct *)(x))
 #define __PT_PARM1_REG a0

@@ -362,12 +366,15 @@ struct pt_regs___arm64 {
 #define __PT_PARM7_REG a6
 #define __PT_PARM8_REG a7
 
-#define __PT_PARM1_SYSCALL_REG __PT_PARM1_REG
+#define __PT_PARM1_SYSCALL_REG orig_a0
 #define __PT_PARM2_SYSCALL_REG __PT_PARM2_REG
 #define __PT_PARM3_SYSCALL_REG __PT_PARM3_REG
 #define __PT_PARM4_SYSCALL_REG __PT_PARM4_REG
 #define __PT_PARM5_SYSCALL_REG __PT_PARM5_REG
 #define __PT_PARM6_SYSCALL_REG __PT_PARM6_REG
+#define PT_REGS_PARM1_SYSCALL(x) (((const struct pt_regs___riscv *)(x))->orig_a0)
+#define PT_REGS_PARM1_CORE_SYSCALL(x) \
+	BPF_CORE_READ((const struct pt_regs___riscv *)(x), __PT_PARM1_SYSCALL_REG)
 
 #define __PT_RET_REG ra
 #define __PT_FP_REG s0

-- 
2.34.1

On 8/31/2024 12:19 PM, Pu Lehui wrote:

Cool!

Acked-by: Xu Kuohai <redacted>

On 8/31/2024 3:26 PM, Xu Kuohai wrote:

On 8/31/2024 12:19 PM, Pu Lehui wrote:

quoted

From: Pu Lehui <pulehui@huawei.com>

Currently PT_REGS_PARM1 SYSCALL(x) is consistent with PT_REGS_PARM1_CORE
SYSCALL(x), which will introduce the overhead of BPF_CORE_READ(), taking
into account the read pt_regs comes directly from the context, let's use
CO-RE direct read to access the first system call argument.

Suggested-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Pu Lehui <pulehui@huawei.com>
---
  tools/lib/bpf/bpf_tracing.h | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
index e7d9382efeb3..051c408e6aed 100644
--- a/tools/lib/bpf/bpf_tracing.h
+++ b/tools/lib/bpf/bpf_tracing.h

@@ -222,7 +222,7 @@ struct pt_regs___s390 {

  struct pt_regs___arm64 {
      unsigned long orig_x0;
-};
+} __attribute__((preserve_access_index));
  /* arm64 provides struct user_pt_regs instead of struct pt_regs to userspace */
  #define __PT_REGS_CAST(x) ((const struct user_pt_regs *)(x))

@@ -241,7 +241,7 @@ struct pt_regs___arm64 {

  #define __PT_PARM4_SYSCALL_REG __PT_PARM4_REG
  #define __PT_PARM5_SYSCALL_REG __PT_PARM5_REG
  #define __PT_PARM6_SYSCALL_REG __PT_PARM6_REG
-#define PT_REGS_PARM1_SYSCALL(x) PT_REGS_PARM1_CORE_SYSCALL(x)
+#define PT_REGS_PARM1_SYSCALL(x) (((const struct pt_regs___arm64 *)(x))->orig_x0)
  #define PT_REGS_PARM1_CORE_SYSCALL(x) \
      BPF_CORE_READ((const struct pt_regs___arm64 *)(x), __PT_PARM1_SYSCALL_REG)

Cool!

Acked-by: Xu Kuohai <redacted>

Wait, it breaks the following test:

--- a/tools/testing/selftests/bpf/progs/bpf_syscall_macro.c
+++ b/tools/testing/selftests/bpf/progs/bpf_syscall_macro.c

@@ -43,7 +43,7 @@ int BPF_KPROBE(handle_sys_prctl)

         /* test for PT_REGS_PARM */

-       bpf_probe_read_kernel(&tmp, sizeof(tmp), &PT_REGS_PARM1_SYSCALL(real_regs));
+       tmp = PT_REGS_PARM1_SYSCALL(real_regs);
         arg1 = tmp;
         bpf_probe_read_kernel(&arg2, sizeof(arg2), &PT_REGS_PARM2_SYSCALL(real_regs));
         bpf_probe_read_kernel(&arg3, sizeof(arg3), &PT_REGS_PARM3_SYSCALL(real_regs));

Failed with verifier rejection:

0: R1=ctx() R10=fp0
; int BPF_KPROBE(handle_sys_prctl) @ bpf_syscall_macro.c:33
0: (bf) r6 = r1                       ; R1=ctx() R6_w=ctx()
; pid_t pid = bpf_get_current_pid_tgid() >> 32; @ bpf_syscall_macro.c:36
1: (85) call bpf_get_current_pid_tgid#14      ; R0_w=scalar()
; if (pid != filter_pid) @ bpf_syscall_macro.c:39
2: (18) r1 = 0xffff800082e0e000       ; R1_w=map_value(map=bpf_sysc.rodata,ks=4,vs=4)
4: (61) r1 = *(u32 *)(r1 +0)          ; R1_w=607
; pid_t pid = bpf_get_current_pid_tgid() >> 32; @ bpf_syscall_macro.c:36
5: (77) r0 >>= 32                     ; R0_w=scalar(smin=0,smax=umax=0xffffffff,var_off=(0x0; 0xffffffff))
; if (pid != filter_pid) @ bpf_syscall_macro.c:39
6: (5e) if w1 != w0 goto pc+98        ; R0_w=607 R1_w=607
; real_regs = PT_REGS_SYSCALL_REGS(ctx); @ bpf_syscall_macro.c:42
7: (79) r8 = *(u64 *)(r6 +0)          ; R6_w=ctx() R8_w=scalar()
; tmp = PT_REGS_PARM1_SYSCALL(real_regs); @ bpf_syscall_macro.c:46
8: (79) r1 = *(u64 *)(r8 +272)
R8 invalid mem access 'scalar'
processed 8 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0

On Sat, Aug 31, 2024 at 12:57 AM Xu Kuohai [off-list ref] wrote:

On 8/31/2024 3:26 PM, Xu Kuohai wrote:

quoted

On 8/31/2024 12:19 PM, Pu Lehui wrote:

quoted

From: Pu Lehui <pulehui@huawei.com>

Currently PT_REGS_PARM1 SYSCALL(x) is consistent with PT_REGS_PARM1_CORE
SYSCALL(x), which will introduce the overhead of BPF_CORE_READ(), taking
into account the read pt_regs comes directly from the context, let's use
CO-RE direct read to access the first system call argument.

Suggested-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Pu Lehui <pulehui@huawei.com>
---
  tools/lib/bpf/bpf_tracing.h | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
index e7d9382efeb3..051c408e6aed 100644
--- a/tools/lib/bpf/bpf_tracing.h
+++ b/tools/lib/bpf/bpf_tracing.h

@@ -222,7 +222,7 @@ struct pt_regs___s390 {
  struct pt_regs___arm64 {
      unsigned long orig_x0;
-};
+} __attribute__((preserve_access_index));
  /* arm64 provides struct user_pt_regs instead of struct pt_regs to userspace */
  #define __PT_REGS_CAST(x) ((const struct user_pt_regs *)(x))

@@ -241,7 +241,7 @@ struct pt_regs___arm64 {
  #define __PT_PARM4_SYSCALL_REG __PT_PARM4_REG
  #define __PT_PARM5_SYSCALL_REG __PT_PARM5_REG
  #define __PT_PARM6_SYSCALL_REG __PT_PARM6_REG
-#define PT_REGS_PARM1_SYSCALL(x) PT_REGS_PARM1_CORE_SYSCALL(x)
+#define PT_REGS_PARM1_SYSCALL(x) (((const struct pt_regs___arm64 *)(x))->orig_x0)
  #define PT_REGS_PARM1_CORE_SYSCALL(x) \
      BPF_CORE_READ((const struct pt_regs___arm64 *)(x), __PT_PARM1_SYSCALL_REG)

Cool!

Acked-by: Xu Kuohai <redacted>

Wait, it breaks the following test:

You mean, *if you change the existing test like below*, it will break,
right? And that's expected, because arm64 has
ARCH_HAS_SYSCALL_WRAPPER, which means syscall pt_regs are actually not
the kprobe's ctx, so you can't directly access it. Which is why we
have PT_REGS_PARM1_CORE_SYSCALL() variants.

See how BPF_KSYSCALL macro is implemented, there are two cases:
___bpf_syswap_args(), which uses BPF_CORE_READ()-based macros to fetch
arguments, and ___bpf_syscall_args() which uses direct ctx reads.

On Fri, Aug 30, 2024 at 9:17 PM Pu Lehui [off-list ref] wrote:

It would probably be best (for consistency) to stick to using
__PTR_PARM1_SYSCALL_REG instead of hard-coding orig_x0 here, no? I'll
fix it up while applying. Same for patch #1 and #4.

It would be great if you can double-check that final patches in
bpf-next/master compile and work well for arm64, s390x, and RV64 (as I
can't really test that much locally).

 #define PT_REGS_PARM1_CORE_SYSCALL(x) \
        BPF_CORE_READ((const struct pt_regs___arm64 *)(x), __PT_PARM1_SYSCALL_REG)

--
2.34.1

On Fri, Aug 30, 2024 at 9:17 PM Pu Lehui [off-list ref] wrote:

There is no point in having tmp variable now, I cleaned that up as well

        bpf_probe_read_kernel(&arg2, sizeof(arg2), &PT_REGS_PARM2_SYSCALL(real_regs));
        bpf_probe_read_kernel(&arg3, sizeof(arg3), &PT_REGS_PARM3_SYSCALL(real_regs));
--
2.34.1

Hello:

This series was applied to bpf/bpf-next.git (master)
by Andrii Nakryiko [off-list ref]:

On Sat, 31 Aug 2024 04:19:30 +0000 you wrote:

On RV64, as Ilya mentioned before [0], the first syscall parameter should be
accessed through orig_a0 (see arch/riscv64/include/asm/syscall.h),
otherwise it will cause selftests like bpf_syscall_macro, vmlinux,
test_lsm, etc. to fail on RV64.

Link: https://lore.kernel.org/bpf/20220209021745.2215452-1-iii@linux.ibm.com [0]

[...]

Here is the summary with links:
  - [bpf-next,v3,1/4] libbpf: Access first syscall argument with CO-RE direct read on s390
    https://git.kernel.org/bpf/bpf-next/c/65ee11d9c822
  - [bpf-next,v3,2/4] libbpf: Access first syscall argument with CO-RE direct read on arm64
    https://git.kernel.org/bpf/bpf-next/c/ebd8ad474888
  - [bpf-next,v3,3/4] selftests/bpf: Enable test_bpf_syscall_macro:syscall_arg1 on s390 and arm64
    https://git.kernel.org/bpf/bpf-next/c/3a913c4d62e1
  - [bpf-next,v3,4/4] libbpf: Fix accessing first syscall argument on RV64
    https://git.kernel.org/bpf/bpf-next/c/13143c5816bc

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

On 9/5/2024 4:06 AM, Andrii Nakryiko wrote:

On Sat, Aug 31, 2024 at 12:57 AM Xu Kuohai [off-list ref] wrote:

quoted

On 8/31/2024 3:26 PM, Xu Kuohai wrote:

quoted

On 8/31/2024 12:19 PM, Pu Lehui wrote:

quoted

From: Pu Lehui <pulehui@huawei.com>

Currently PT_REGS_PARM1 SYSCALL(x) is consistent with PT_REGS_PARM1_CORE
SYSCALL(x), which will introduce the overhead of BPF_CORE_READ(), taking
into account the read pt_regs comes directly from the context, let's use
CO-RE direct read to access the first system call argument.

Suggested-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Pu Lehui <pulehui@huawei.com>
---
   tools/lib/bpf/bpf_tracing.h | 4 ++--
   1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
index e7d9382efeb3..051c408e6aed 100644
--- a/tools/lib/bpf/bpf_tracing.h
+++ b/tools/lib/bpf/bpf_tracing.h

@@ -222,7 +222,7 @@ struct pt_regs___s390 {
   struct pt_regs___arm64 {
       unsigned long orig_x0;
-};
+} __attribute__((preserve_access_index));
   /* arm64 provides struct user_pt_regs instead of struct pt_regs to userspace */
   #define __PT_REGS_CAST(x) ((const struct user_pt_regs *)(x))

@@ -241,7 +241,7 @@ struct pt_regs___arm64 {
   #define __PT_PARM4_SYSCALL_REG __PT_PARM4_REG
   #define __PT_PARM5_SYSCALL_REG __PT_PARM5_REG
   #define __PT_PARM6_SYSCALL_REG __PT_PARM6_REG
-#define PT_REGS_PARM1_SYSCALL(x) PT_REGS_PARM1_CORE_SYSCALL(x)
+#define PT_REGS_PARM1_SYSCALL(x) (((const struct pt_regs___arm64 *)(x))->orig_x0)
   #define PT_REGS_PARM1_CORE_SYSCALL(x) \
       BPF_CORE_READ((const struct pt_regs___arm64 *)(x), __PT_PARM1_SYSCALL_REG)

Cool!

Acked-by: Xu Kuohai <redacted>

Wait, it breaks the following test:

You mean, *if you change the existing test like below*, it will break,
right? And that's expected, because arm64 has
ARCH_HAS_SYSCALL_WRAPPER, which means syscall pt_regs are actually not
the kprobe's ctx, so you can't directly access it. Which is why we
have PT_REGS_PARM1_CORE_SYSCALL() variants.

See how BPF_KSYSCALL macro is implemented, there are two cases:
___bpf_syswap_args(), which uses BPF_CORE_READ()-based macros to fetch
arguments, and ___bpf_syscall_args() which uses direct ctx reads.

Got it, thanks for the explanation.

quoted

--- a/tools/testing/selftests/bpf/progs/bpf_syscall_macro.c
+++ b/tools/testing/selftests/bpf/progs/bpf_syscall_macro.c

@@ -43,7 +43,7 @@ int BPF_KPROBE(handle_sys_prctl)

          /* test for PT_REGS_PARM */

-       bpf_probe_read_kernel(&tmp, sizeof(tmp), &PT_REGS_PARM1_SYSCALL(real_regs));
+       tmp = PT_REGS_PARM1_SYSCALL(real_regs);
          arg1 = tmp;
          bpf_probe_read_kernel(&arg2, sizeof(arg2), &PT_REGS_PARM2_SYSCALL(real_regs));
          bpf_probe_read_kernel(&arg3, sizeof(arg3), &PT_REGS_PARM3_SYSCALL(real_regs));

Failed with verifier rejection:

0: R1=ctx() R10=fp0
; int BPF_KPROBE(handle_sys_prctl) @ bpf_syscall_macro.c:33
0: (bf) r6 = r1                       ; R1=ctx() R6_w=ctx()
; pid_t pid = bpf_get_current_pid_tgid() >> 32; @ bpf_syscall_macro.c:36
1: (85) call bpf_get_current_pid_tgid#14      ; R0_w=scalar()
; if (pid != filter_pid) @ bpf_syscall_macro.c:39
2: (18) r1 = 0xffff800082e0e000       ; R1_w=map_value(map=bpf_sysc.rodata,ks=4,vs=4)
4: (61) r1 = *(u32 *)(r1 +0)          ; R1_w=607
; pid_t pid = bpf_get_current_pid_tgid() >> 32; @ bpf_syscall_macro.c:36
5: (77) r0 >>= 32                     ; R0_w=scalar(smin=0,smax=umax=0xffffffff,var_off=(0x0; 0xffffffff))
; if (pid != filter_pid) @ bpf_syscall_macro.c:39
6: (5e) if w1 != w0 goto pc+98        ; R0_w=607 R1_w=607
; real_regs = PT_REGS_SYSCALL_REGS(ctx); @ bpf_syscall_macro.c:42
7: (79) r8 = *(u64 *)(r6 +0)          ; R6_w=ctx() R8_w=scalar()
; tmp = PT_REGS_PARM1_SYSCALL(real_regs); @ bpf_syscall_macro.c:46
8: (79) r1 = *(u64 *)(r8 +272)
R8 invalid mem access 'scalar'
processed 8 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0

On 2024/9/5 4:21, Andrii Nakryiko wrote:

On Fri, Aug 30, 2024 at 9:17 PM Pu Lehui [off-list ref] wrote:

quoted

From: Pu Lehui <pulehui@huawei.com>

Currently PT_REGS_PARM1 SYSCALL(x) is consistent with PT_REGS_PARM1_CORE
SYSCALL(x), which will introduce the overhead of BPF_CORE_READ(), taking
into account the read pt_regs comes directly from the context, let's use
CO-RE direct read to access the first system call argument.

Suggested-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Pu Lehui <pulehui@huawei.com>
---
  tools/lib/bpf/bpf_tracing.h | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
index e7d9382efeb3..051c408e6aed 100644
--- a/tools/lib/bpf/bpf_tracing.h
+++ b/tools/lib/bpf/bpf_tracing.h

@@ -222,7 +222,7 @@ struct pt_regs___s390 {

  struct pt_regs___arm64 {
         unsigned long orig_x0;
-};
+} __attribute__((preserve_access_index));

  /* arm64 provides struct user_pt_regs instead of struct pt_regs to userspace */
  #define __PT_REGS_CAST(x) ((const struct user_pt_regs *)(x))

@@ -241,7 +241,7 @@ struct pt_regs___arm64 {
  #define __PT_PARM4_SYSCALL_REG __PT_PARM4_REG
  #define __PT_PARM5_SYSCALL_REG __PT_PARM5_REG
  #define __PT_PARM6_SYSCALL_REG __PT_PARM6_REG
-#define PT_REGS_PARM1_SYSCALL(x) PT_REGS_PARM1_CORE_SYSCALL(x)
+#define PT_REGS_PARM1_SYSCALL(x) (((const struct pt_regs___arm64 *)(x))->orig_x0)

It would probably be best (for consistency) to stick to using
__PTR_PARM1_SYSCALL_REG instead of hard-coding orig_x0 here, no? I'll
fix it up while applying. Same for patch #1 and #4.

It would be great if you can double-check that final patches in
bpf-next/master compile and work well for arm64, s390x, and RV64 (as I
can't really test that much locally).

I check that locally with cross-platform vmtest on RV64, it looks good:

Summary: 569/3944 PASSED, 104 SKIPPED, 0 FAILED

and BPF CI meet happy on arm64, s390x.

quoted

  #define PT_REGS_PARM1_CORE_SYSCALL(x) \
         BPF_CORE_READ((const struct pt_regs___arm64 *)(x), __PT_PARM1_SYSCALL_REG)

--
2.34.1

Hello:

This series was applied to riscv/linux.git (fixes)
by Andrii Nakryiko [off-list ref]:

On Sat, 31 Aug 2024 04:19:30 +0000 you wrote:

On RV64, as Ilya mentioned before [0], the first syscall parameter should be
accessed through orig_a0 (see arch/riscv64/include/asm/syscall.h),
otherwise it will cause selftests like bpf_syscall_macro, vmlinux,
test_lsm, etc. to fail on RV64.

Link: https://lore.kernel.org/bpf/20220209021745.2215452-1-iii@linux.ibm.com [0]

[...]

Here is the summary with links:
  - [bpf-next,v3,1/4] libbpf: Access first syscall argument with CO-RE direct read on s390
    https://git.kernel.org/riscv/c/e4db2a821b6c
  - [bpf-next,v3,2/4] libbpf: Access first syscall argument with CO-RE direct read on arm64
    https://git.kernel.org/riscv/c/9ab94078e868
  - [bpf-next,v3,3/4] selftests/bpf: Enable test_bpf_syscall_macro:syscall_arg1 on s390 and arm64
    https://git.kernel.org/riscv/c/4a4c4c0d0a42
  - [bpf-next,v3,4/4] libbpf: Fix accessing first syscall argument on RV64
    https://git.kernel.org/riscv/c/99857422338b

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html