[PATCH bpf-next v4 00/14] Fix accessing syscall arguments

[PATCH bpf-next v4 00/14] Fix accessing syscall arguments

[Ilya Leoshkevich]

libbpf now has macros to access syscall arguments in an
architecture-agnostic manner, but unfortunately they have a number of
issues on non-Intel arches, which this series aims to fix.

v1: https://lore.kernel.org/bpf/20220201234200.1836443-1-iii@linux.ibm.com/
v1 -> v2:
* Put orig_gpr2 in place of args[1] on s390 (Vasily).
* Fix arm64, powerpc and riscv (Heiko).

v2: https://lore.kernel.org/bpf/20220204041955.1958263-1-iii@linux.ibm.com/
v2 -> v3:
* Undo args[1] change (Andrii).
* Rename PT_REGS_SYSCALL to PT_REGS_SYSCALL_REGS (Andrii).
* Split the riscv patch (Andrii).

v3: https://lore.kernel.org/bpf/20220204145018.1983773-1-iii@linux.ibm.com/
v3 -> v4:
* Undo arm64's and s390's user_pt_regs changes.
* Use struct pt_regs when vmlinux.h is available (Andrii).
* Use offsetofend for accessing orig_gpr2 and orig_x0 (Andrii).
* Move libbpf's copy of offsetofend to a new header.
* Fix riscv's __PT_FP_REG.
* Use PT_REGS_SYSCALL_REGS in test_probe_user.c.
* Test bpf_syscall_macro with userspace headers.
* Use Naveen's suggestions and code in patches 5 and 6.
* Add warnings to arm64's and s390's ptrace.h (Andrii).

Tested on x86_64, s390x, arm64, ppc64el and riscv64 in QEMU.

+cc Mark.

Ilya Leoshkevich (14):
  selftests/bpf: Fix an endianness issue in bpf_syscall_macro test
  selftests/bpf: Fix a potential offsetofend redefinition in
    test_cls_redirect
  selftests/bpf: Compile bpf_syscall_macro test also with user headers
  libbpf: Fix a typo in bpf_tracing.h
  libbpf: Generalize overriding syscall parameter access macros
  libbpf: Add PT_REGS_SYSCALL_REGS macro
  selftests/bpf: Use PT_REGS_SYSCALL_REGS in bpf_syscall_macro
  libbpf: Use struct pt_regs when compiling with kernel headers
  libbpf: Fix riscv register names
  libbpf: Move data structure manipulation macros to
    bpf_common_helpers.h
  libbpf: Fix accessing the first syscall argument on s390
  s390: add a comment that warns that orig_gpr2 should not be moved
  libbpf: Fix accessing the first syscall argument on arm64
  arm64: add a comment that warns that orig_x0 should not be moved

 arch/arm64/include/asm/ptrace.h               |   4 +
 arch/s390/include/asm/ptrace.h                |   4 +
 tools/lib/bpf/Makefile                        |   2 +-
 tools/lib/bpf/bpf_common_helpers.h            |  30 +++++
 tools/lib/bpf/bpf_helpers.h                   |  15 +--
 tools/lib/bpf/bpf_tracing.h                   | 107 +++++++++++++++---
 tools/testing/selftests/bpf/bpf_util.h        |  10 +-
 ...acro.c => test_bpf_syscall_macro_common.h} |   8 +-
 .../test_bpf_syscall_macro_kernel.c           |  13 +++
 .../prog_tests/test_bpf_syscall_macro_user.c  |  13 +++
 tools/testing/selftests/bpf/progs/bpf_misc.h  |   4 -
 ...all_macro.c => bpf_syscall_macro_common.h} |  15 ++-
 .../bpf/progs/bpf_syscall_macro_kernel.c      |   4 +
 .../bpf/progs/bpf_syscall_macro_user.c        |  10 ++
 .../selftests/bpf/progs/test_cls_redirect.c   |   2 +
 .../selftests/bpf/progs/test_probe_user.c     |   8 +-
 16 files changed, 191 insertions(+), 58 deletions(-)
 create mode 100644 tools/lib/bpf/bpf_common_helpers.h
 rename tools/testing/selftests/bpf/prog_tests/{test_bpf_syscall_macro.c => test_bpf_syscall_macro_common.h} (89%)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/test_bpf_syscall_macro_kernel.c
 create mode 100644 tools/testing/selftests/bpf/prog_tests/test_bpf_syscall_macro_user.c
 rename tools/testing/selftests/bpf/progs/{bpf_syscall_macro.c => bpf_syscall_macro_common.h} (79%)
 create mode 100644 tools/testing/selftests/bpf/progs/bpf_syscall_macro_kernel.c
 create mode 100644 tools/testing/selftests/bpf/progs/bpf_syscall_macro_user.c

-- 
2.34.1

[PATCH bpf-next v4 01/14] selftests/bpf: Fix an endianness issue in bpf_syscall_macro test

[Ilya Leoshkevich]

bpf_syscall_macro reads a long argument into an int variable, which
produces a wrong value on big-endian systems. Fix by reading the
argument into an intermediate long variable first.

Fixes: 77fc0330dfe5 ("selftests/bpf: Add a test to confirm PT_REGS_PARM4_SYSCALL")
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/testing/selftests/bpf/progs/bpf_syscall_macro.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/progs/bpf_syscall_macro.c b/tools/testing/selftests/bpf/progs/bpf_syscall_macro.c
index c8e60220cda8..f5c6ef2ff6d1 100644
--- a/tools/testing/selftests/bpf/progs/bpf_syscall_macro.c
+++ b/tools/testing/selftests/bpf/progs/bpf_syscall_macro.c
@@ -28,6 +28,7 @@ int BPF_KPROBE(handle_sys_prctl)
 {
 	struct pt_regs *real_regs;
 	pid_t pid = bpf_get_current_pid_tgid() >> 32;
+	unsigned long tmp;
 
 	if (pid != filter_pid)
 		return 0;
@@ -35,7 +36,9 @@ int BPF_KPROBE(handle_sys_prctl)
 	real_regs = (struct pt_regs *)PT_REGS_PARM1(ctx);
 
 	/* test for PT_REGS_PARM */
-	bpf_probe_read_kernel(&arg1, sizeof(arg1), &PT_REGS_PARM1_SYSCALL(real_regs));
+
+	bpf_probe_read_kernel(&tmp, sizeof(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));
 	bpf_probe_read_kernel(&arg4_cx, sizeof(arg4_cx), &PT_REGS_PARM4(real_regs));
-- 
2.34.1

[PATCH bpf-next v4 02/14] selftests/bpf: Fix a potential offsetofend redefinition in test_cls_redirect

[Ilya Leoshkevich]

If one of the included headers defines offsetofend macro,
test_cls_redirect would fail to build. Wrap the offsetofend definition
in #ifdef to avoid that.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/testing/selftests/bpf/progs/test_cls_redirect.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/testing/selftests/bpf/progs/test_cls_redirect.c b/tools/testing/selftests/bpf/progs/test_cls_redirect.c
index 2833ad722cb7..ed1b9c90a66b 100644
--- a/tools/testing/selftests/bpf/progs/test_cls_redirect.c
+++ b/tools/testing/selftests/bpf/progs/test_cls_redirect.c
@@ -28,8 +28,10 @@
 #define INLINING __always_inline
 #endif
 
+#ifndef offsetofend
 #define offsetofend(TYPE, MEMBER) \
 	(offsetof(TYPE, MEMBER) + sizeof((((TYPE *)0)->MEMBER)))
+#endif
 
 #define IP_OFFSET_MASK (0x1FFF)
 #define IP_MF (0x2000)
-- 
2.34.1

[PATCH bpf-next v4 03/14] selftests/bpf: Compile bpf_syscall_macro test also with user headers

[Ilya Leoshkevich]

Verify that using linux/ptrace.h instead of vmlinux.h works fine.
Since without vmlinux.h and with CO-RE it's not possible to access the
first syscall argument on arm64 and s390x, and any syscall arguments on
Intel, skip the corresponding checks.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 ...call_macro.c => test_bpf_syscall_macro_common.h} |  8 +++++++-
 .../bpf/prog_tests/test_bpf_syscall_macro_kernel.c  | 13 +++++++++++++
 .../bpf/prog_tests/test_bpf_syscall_macro_user.c    | 13 +++++++++++++
 ...f_syscall_macro.c => bpf_syscall_macro_common.h} |  8 ++++++--
 .../selftests/bpf/progs/bpf_syscall_macro_kernel.c  |  4 ++++
 .../selftests/bpf/progs/bpf_syscall_macro_user.c    | 10 ++++++++++
 6 files changed, 53 insertions(+), 3 deletions(-)
 rename tools/testing/selftests/bpf/prog_tests/{test_bpf_syscall_macro.c => test_bpf_syscall_macro_common.h} (89%)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/test_bpf_syscall_macro_kernel.c
 create mode 100644 tools/testing/selftests/bpf/prog_tests/test_bpf_syscall_macro_user.c
 rename tools/testing/selftests/bpf/progs/{bpf_syscall_macro.c => bpf_syscall_macro_common.h} (87%)
 create mode 100644 tools/testing/selftests/bpf/progs/bpf_syscall_macro_kernel.c
 create mode 100644 tools/testing/selftests/bpf/progs/bpf_syscall_macro_user.c

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_common.h
similarity index 89%
rename from tools/testing/selftests/bpf/prog_tests/test_bpf_syscall_macro.c
rename to tools/testing/selftests/bpf/prog_tests/test_bpf_syscall_macro_common.h
index f5f4c8adf539..9f2a395abff7 100644
--- a/tools/testing/selftests/bpf/prog_tests/test_bpf_syscall_macro.c
+++ b/tools/testing/selftests/bpf/prog_tests/test_bpf_syscall_macro_common.h
@@ -2,7 +2,6 @@
 /* Copyright 2022 Sony Group Corporation */
 #include <sys/prctl.h>
 #include <test_progs.h>
-#include "bpf_syscall_macro.skel.h"
 
 void test_bpf_syscall_macro(void)
 {
@@ -46,7 +45,13 @@ void test_bpf_syscall_macro(void)
 	ASSERT_EQ(skel->bss->arg5, exp_arg5, "syscall_arg5");
 
 	/* check whether args of syscall are copied correctly for CORE variants */
+#if defined(__BPF_SYSCALL_MACRO_KERNEL_SKEL_H__) || \
+		(!defined(__s390__) && !defined(__aarch64__) && \
+		 !defined(__i386__) && !defined(__x86_64__))
 	ASSERT_EQ(skel->bss->arg1_core, exp_arg1, "syscall_arg1_core_variant");
+#endif
+#if defined(__BPF_SYSCALL_MACRO_KERNEL_SKEL_H__) || \
+		(!defined(__i386__) && !defined(__x86_64__))
 	ASSERT_EQ(skel->bss->arg2_core, exp_arg2, "syscall_arg2_core_variant");
 	ASSERT_EQ(skel->bss->arg3_core, exp_arg3, "syscall_arg3_core_variant");
 	/* it cannot copy arg4 when uses PT_REGS_PARM4_CORE on x86_64 */
@@ -57,6 +62,7 @@ void test_bpf_syscall_macro(void)
 #endif
 	ASSERT_EQ(skel->bss->arg4_core, exp_arg4, "syscall_arg4_core_variant");
 	ASSERT_EQ(skel->bss->arg5_core, exp_arg5, "syscall_arg5_core_variant");
+#endif
 
 cleanup:
 	bpf_syscall_macro__destroy(skel);
diff --git a/tools/testing/selftests/bpf/prog_tests/test_bpf_syscall_macro_kernel.c b/tools/testing/selftests/bpf/prog_tests/test_bpf_syscall_macro_kernel.c
new file mode 100644
index 000000000000..7ceabd62bb0f
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/test_bpf_syscall_macro_kernel.c
@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: GPL-2.0
+#include "bpf_syscall_macro_kernel.skel.h"
+
+void test_bpf_syscall_macro_kernel(void);
+
+#define test_bpf_syscall_macro test_bpf_syscall_macro_kernel
+#define bpf_syscall_macro bpf_syscall_macro_kernel
+#define bpf_syscall_macro__open bpf_syscall_macro_kernel__open
+#define bpf_syscall_macro__load bpf_syscall_macro_kernel__load
+#define bpf_syscall_macro__attach bpf_syscall_macro_kernel__attach
+#define bpf_syscall_macro__destroy bpf_syscall_macro_kernel__destroy
+
+#include "test_bpf_syscall_macro_common.h"
diff --git a/tools/testing/selftests/bpf/prog_tests/test_bpf_syscall_macro_user.c b/tools/testing/selftests/bpf/prog_tests/test_bpf_syscall_macro_user.c
new file mode 100644
index 000000000000..f31558f14e7e
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/test_bpf_syscall_macro_user.c
@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: GPL-2.0
+#include "bpf_syscall_macro_user.skel.h"
+
+void test_bpf_syscall_macro_user(void);
+
+#define test_bpf_syscall_macro test_bpf_syscall_macro_user
+#define bpf_syscall_macro bpf_syscall_macro_user
+#define bpf_syscall_macro__open bpf_syscall_macro_user__open
+#define bpf_syscall_macro__load bpf_syscall_macro_user__load
+#define bpf_syscall_macro__attach bpf_syscall_macro_user__attach
+#define bpf_syscall_macro__destroy bpf_syscall_macro_user__destroy
+
+#include "test_bpf_syscall_macro_common.h"
diff --git a/tools/testing/selftests/bpf/progs/bpf_syscall_macro.c b/tools/testing/selftests/bpf/progs/bpf_syscall_macro_common.h
similarity index 87%
rename from tools/testing/selftests/bpf/progs/bpf_syscall_macro.c
rename to tools/testing/selftests/bpf/progs/bpf_syscall_macro_common.h
index f5c6ef2ff6d1..8717605358d3 100644
--- a/tools/testing/selftests/bpf/progs/bpf_syscall_macro.c
+++ b/tools/testing/selftests/bpf/progs/bpf_syscall_macro_common.h
@@ -1,7 +1,5 @@
 // SPDX-License-Identifier: GPL-2.0
 /* Copyright 2022 Sony Group Corporation */
-#include <vmlinux.h>
-
 #include <bpf/bpf_core_read.h>
 #include <bpf/bpf_helpers.h>
 #include <bpf/bpf_tracing.h>
@@ -46,12 +44,18 @@ int BPF_KPROBE(handle_sys_prctl)
 	bpf_probe_read_kernel(&arg5, sizeof(arg5), &PT_REGS_PARM5_SYSCALL(real_regs));
 
 	/* test for the CORE variant of PT_REGS_PARM */
+#if defined(__KERNEL__) || defined(__VMLINUX_H__) || \
+		(!defined(bpf_target_s390) && !defined(bpf_target_arm64) && \
+		 !defined(bpf_target_x86))
 	arg1_core = PT_REGS_PARM1_CORE_SYSCALL(real_regs);
+#endif
+#if defined(__KERNEL__) || defined(__VMLINUX_H__) || !defined(bpf_target_x86)
 	arg2_core = PT_REGS_PARM2_CORE_SYSCALL(real_regs);
 	arg3_core = PT_REGS_PARM3_CORE_SYSCALL(real_regs);
 	arg4_core_cx = PT_REGS_PARM4_CORE(real_regs);
 	arg4_core = PT_REGS_PARM4_CORE_SYSCALL(real_regs);
 	arg5_core = PT_REGS_PARM5_CORE_SYSCALL(real_regs);
+#endif
 
 	return 0;
 }
diff --git a/tools/testing/selftests/bpf/progs/bpf_syscall_macro_kernel.c b/tools/testing/selftests/bpf/progs/bpf_syscall_macro_kernel.c
new file mode 100644
index 000000000000..1affac21266d
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/bpf_syscall_macro_kernel.c
@@ -0,0 +1,4 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <vmlinux.h>
+
+#include "bpf_syscall_macro_common.h"
diff --git a/tools/testing/selftests/bpf/progs/bpf_syscall_macro_user.c b/tools/testing/selftests/bpf/progs/bpf_syscall_macro_user.c
new file mode 100644
index 000000000000..1c078d528e8c
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/bpf_syscall_macro_user.c
@@ -0,0 +1,10 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/ptrace.h>
+#include <linux/types.h>
+#include <sys/types.h>
+
+#include "bpf_syscall_macro_common.h"
+
+#if defined(__KERNEL__) || defined(__VMLINUX_H__)
+#error This test must be compiled with userspace headers
+#endif
-- 
2.34.1

[PATCH bpf-next v4 04/14] libbpf: Fix a typo in bpf_tracing.h

[Ilya Leoshkevich]

s/architecutres/architectures/

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/lib/bpf/bpf_tracing.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
index 032ba809f3e5..da7e8d5c939c 100644
--- a/tools/lib/bpf/bpf_tracing.h
+++ b/tools/lib/bpf/bpf_tracing.h
@@ -219,7 +219,7 @@
 
 struct pt_regs;
 
-/* allow some architecutres to override `struct pt_regs` */
+/* allow some architectures to override `struct pt_regs` */
 #ifndef __PT_REGS_CAST
 #define __PT_REGS_CAST(x) (x)
 #endif
-- 
2.34.1

[PATCH bpf-next v4 05/14] libbpf: Generalize overriding syscall parameter access macros

[Ilya Leoshkevich]

Instead of conditionally overriding PT_REGS_PARM4_SYSCALL, provide
default fallbacks for all __PT_PARMn_REG_SYSCALL macros, so that
architectures can simply override a specific syscall parameter macro.
Also allow completely overriding PT_REGS_PARM1_SYSCALL for
non-trivial access sequences.

Co-developed-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/lib/bpf/bpf_tracing.h | 48 +++++++++++++++++++++++++------------
 1 file changed, 33 insertions(+), 15 deletions(-)

diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
index da7e8d5c939c..82f1e935d549 100644
--- a/tools/lib/bpf/bpf_tracing.h
+++ b/tools/lib/bpf/bpf_tracing.h
@@ -265,25 +265,43 @@ struct pt_regs;
 
 #endif
 
-#define PT_REGS_PARM1_SYSCALL(x) PT_REGS_PARM1(x)
-#define PT_REGS_PARM2_SYSCALL(x) PT_REGS_PARM2(x)
-#define PT_REGS_PARM3_SYSCALL(x) PT_REGS_PARM3(x)
-#ifdef __PT_PARM4_REG_SYSCALL
+#ifndef __PT_PARM1_REG_SYSCALL
+#define __PT_PARM1_REG_SYSCALL __PT_PARM1_REG
+#endif
+#ifndef __PT_PARM2_REG_SYSCALL
+#define __PT_PARM2_REG_SYSCALL __PT_PARM2_REG
+#endif
+#ifndef __PT_PARM3_REG_SYSCALL
+#define __PT_PARM3_REG_SYSCALL __PT_PARM3_REG
+#endif
+#ifndef __PT_PARM4_REG_SYSCALL
+#define __PT_PARM4_REG_SYSCALL __PT_PARM4_REG
+#endif
+#ifndef __PT_PARM5_REG_SYSCALL
+#define __PT_PARM5_REG_SYSCALL __PT_PARM5_REG
+#endif
+
+#ifndef PT_REGS_PARM1_SYSCALL
+#define PT_REGS_PARM1_SYSCALL(x) (__PT_REGS_CAST(x)->__PT_PARM1_REG_SYSCALL)
+#endif
+#ifndef PT_REGS_PARM2_SYSCALL
+#define PT_REGS_PARM2_SYSCALL(x) (__PT_REGS_CAST(x)->__PT_PARM2_REG_SYSCALL)
+#endif
+#ifndef PT_REGS_PARM3_SYSCALL
+#define PT_REGS_PARM3_SYSCALL(x) (__PT_REGS_CAST(x)->__PT_PARM3_REG_SYSCALL)
+#endif
+#ifndef PT_REGS_PARM4_SYSCALL
 #define PT_REGS_PARM4_SYSCALL(x) (__PT_REGS_CAST(x)->__PT_PARM4_REG_SYSCALL)
-#else /* __PT_PARM4_REG_SYSCALL */
-#define PT_REGS_PARM4_SYSCALL(x) PT_REGS_PARM4(x)
 #endif
-#define PT_REGS_PARM5_SYSCALL(x) PT_REGS_PARM5(x)
+#ifndef PT_REGS_PARM5_SYSCALL
+#define PT_REGS_PARM5_SYSCALL(x) (__PT_REGS_CAST(x)->__PT_PARM5_REG_SYSCALL)
+#endif
 
-#define PT_REGS_PARM1_CORE_SYSCALL(x) PT_REGS_PARM1_CORE(x)
-#define PT_REGS_PARM2_CORE_SYSCALL(x) PT_REGS_PARM2_CORE(x)
-#define PT_REGS_PARM3_CORE_SYSCALL(x) PT_REGS_PARM3_CORE(x)
-#ifdef __PT_PARM4_REG_SYSCALL
+#define PT_REGS_PARM1_CORE_SYSCALL(x) BPF_CORE_READ(__PT_REGS_CAST(x), __PT_PARM1_REG_SYSCALL)
+#define PT_REGS_PARM2_CORE_SYSCALL(x) BPF_CORE_READ(__PT_REGS_CAST(x), __PT_PARM2_REG_SYSCALL)
+#define PT_REGS_PARM3_CORE_SYSCALL(x) BPF_CORE_READ(__PT_REGS_CAST(x), __PT_PARM3_REG_SYSCALL)
 #define PT_REGS_PARM4_CORE_SYSCALL(x) BPF_CORE_READ(__PT_REGS_CAST(x), __PT_PARM4_REG_SYSCALL)
-#else /* __PT_PARM4_REG_SYSCALL */
-#define PT_REGS_PARM4_CORE_SYSCALL(x) PT_REGS_PARM4_CORE(x)
-#endif
-#define PT_REGS_PARM5_CORE_SYSCALL(x) PT_REGS_PARM5_CORE(x)
+#define PT_REGS_PARM5_CORE_SYSCALL(x) BPF_CORE_READ(__PT_REGS_CAST(x), __PT_PARM5_REG_SYSCALL)
 
 #else /* defined(bpf_target_defined) */
 
-- 
2.34.1

[PATCH bpf-next v4 06/14] libbpf: Add PT_REGS_SYSCALL_REGS macro

[Ilya Leoshkevich]

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@linux.ibm.com>
Co-developed-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 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().
+ */
+#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

[PATCH bpf-next v4 07/14] selftests/bpf: Use PT_REGS_SYSCALL_REGS in bpf_syscall_macro

[Ilya Leoshkevich]

Ensure that PT_REGS_SYSCALL_REGS works correctly, and also remove some
duplication.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/testing/selftests/bpf/progs/bpf_misc.h              | 4 ----
 .../selftests/bpf/progs/bpf_syscall_macro_common.h        | 2 +-
 tools/testing/selftests/bpf/progs/test_probe_user.c       | 8 +-------
 3 files changed, 2 insertions(+), 12 deletions(-)

diff --git a/tools/testing/selftests/bpf/progs/bpf_misc.h b/tools/testing/selftests/bpf/progs/bpf_misc.h
index 5bb11fe595a4..9f2937b5e819 100644
--- a/tools/testing/selftests/bpf/progs/bpf_misc.h
+++ b/tools/testing/selftests/bpf/progs/bpf_misc.h
@@ -3,16 +3,12 @@
 #define __BPF_MISC_H__
 
 #if defined(__TARGET_ARCH_x86)
-#define SYSCALL_WRAPPER 1
 #define SYS_PREFIX "__x64_"
 #elif defined(__TARGET_ARCH_s390)
-#define SYSCALL_WRAPPER 1
 #define SYS_PREFIX "__s390x_"
 #elif defined(__TARGET_ARCH_arm64)
-#define SYSCALL_WRAPPER 1
 #define SYS_PREFIX "__arm64_"
 #else
-#define SYSCALL_WRAPPER 0
 #define SYS_PREFIX "__se_"
 #endif
 
diff --git a/tools/testing/selftests/bpf/progs/bpf_syscall_macro_common.h b/tools/testing/selftests/bpf/progs/bpf_syscall_macro_common.h
index 8717605358d3..95c2f1904f81 100644
--- a/tools/testing/selftests/bpf/progs/bpf_syscall_macro_common.h
+++ b/tools/testing/selftests/bpf/progs/bpf_syscall_macro_common.h
@@ -31,7 +31,7 @@ int BPF_KPROBE(handle_sys_prctl)
 	if (pid != filter_pid)
 		return 0;
 
-	real_regs = (struct pt_regs *)PT_REGS_PARM1(ctx);
+	real_regs = PT_REGS_SYSCALL_REGS(ctx);
 
 	/* test for PT_REGS_PARM */
 
diff --git a/tools/testing/selftests/bpf/progs/test_probe_user.c b/tools/testing/selftests/bpf/progs/test_probe_user.c
index 702578a5e496..b2531f587c87 100644
--- a/tools/testing/selftests/bpf/progs/test_probe_user.c
+++ b/tools/testing/selftests/bpf/progs/test_probe_user.c
@@ -14,18 +14,12 @@ static struct sockaddr_in old;
 SEC("kprobe/" SYS_PREFIX "sys_connect")
 int BPF_KPROBE(handle_sys_connect)
 {
-#if SYSCALL_WRAPPER == 1
 	struct pt_regs *real_regs;
-#endif
 	struct sockaddr_in new;
 	void *ptr;
 
-#if SYSCALL_WRAPPER == 0
-	ptr = (void *)PT_REGS_PARM2(ctx);
-#else
-	real_regs = (struct pt_regs *)PT_REGS_PARM1(ctx);
+	real_regs = PT_REGS_SYSCALL_REGS(ctx);
 	bpf_probe_read_kernel(&ptr, sizeof(ptr), &PT_REGS_PARM2(real_regs));
-#endif
 
 	bpf_probe_read_user(&old, sizeof(old), ptr);
 	__builtin_memset(&new, 0xab, sizeof(new));
-- 
2.34.1

[PATCH bpf-next v4 08/14] libbpf: Use struct pt_regs when compiling with kernel headers

[Ilya Leoshkevich]

Andrii says: "... with CO-RE and vmlinux.h it would be more reliable
and straightforward to just stick to kernel-internal struct pt_regs
everywhere ...".

Actually, if vmlinux.h is available, then it's ok to do so for both
CO-RE and non-CO-RE cases, since the beginning of struct pt_regs must
match (struct) user_pt_regs, which must never change.

Implement this by not defining __PT_REGS_CAST if the user included
vmlinux.h.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/lib/bpf/bpf_tracing.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
index 7a015ee8fb11..07e291d77e83 100644
--- a/tools/lib/bpf/bpf_tracing.h
+++ b/tools/lib/bpf/bpf_tracing.h
@@ -118,8 +118,11 @@
 
 #define __BPF_ARCH_HAS_SYSCALL_WRAPPER
 
+#if !defined(__KERNEL__) && !defined(__VMLINUX_H__)
 /* s390 provides user_pt_regs instead of struct pt_regs to userspace */
 #define __PT_REGS_CAST(x) ((const user_pt_regs *)(x))
+#endif
+
 #define __PT_PARM1_REG gprs[2]
 #define __PT_PARM2_REG gprs[3]
 #define __PT_PARM3_REG gprs[4]
@@ -148,8 +151,11 @@
 
 #define __BPF_ARCH_HAS_SYSCALL_WRAPPER
 
+#if !defined(__KERNEL__) && !defined(__VMLINUX_H__)
 /* arm64 provides struct user_pt_regs instead of struct pt_regs to userspace */
 #define __PT_REGS_CAST(x) ((const struct user_pt_regs *)(x))
+#endif
+
 #define __PT_PARM1_REG regs[0]
 #define __PT_PARM2_REG regs[1]
 #define __PT_PARM3_REG regs[2]
@@ -207,7 +213,10 @@
 
 #elif defined(bpf_target_riscv)
 
+#if !defined(__KERNEL__) && !defined(__VMLINUX_H__)
 #define __PT_REGS_CAST(x) ((const struct user_regs_struct *)(x))
+#endif
+
 #define __PT_PARM1_REG a0
 #define __PT_PARM2_REG a1
 #define __PT_PARM3_REG a2
-- 
2.34.1

[PATCH bpf-next v4 09/14] libbpf: Fix riscv register names

[Ilya Leoshkevich]

struct pt_regs and struct user_regs_struct have different names for the
program counter: epc and pc respectively. Also, the frame pointer is
called s0, not fp.

Reported-by: Heiko Carstens <hca@linux.ibm.com>
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/lib/bpf/bpf_tracing.h | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
index 07e291d77e83..88ed5ba9510c 100644
--- a/tools/lib/bpf/bpf_tracing.h
+++ b/tools/lib/bpf/bpf_tracing.h
@@ -213,8 +213,11 @@
 
 #elif defined(bpf_target_riscv)
 
-#if !defined(__KERNEL__) && !defined(__VMLINUX_H__)
+#if defined(__KERNEL__) || defined(__VMLINUX_H__)
+#define __PT_IP_REG epc
+#else
 #define __PT_REGS_CAST(x) ((const struct user_regs_struct *)(x))
+#define __PT_IP_REG pc
 #endif
 
 #define __PT_PARM1_REG a0
@@ -223,10 +226,9 @@
 #define __PT_PARM4_REG a3
 #define __PT_PARM5_REG a4
 #define __PT_RET_REG ra
-#define __PT_FP_REG fp
+#define __PT_FP_REG s0
 #define __PT_RC_REG a5
 #define __PT_SP_REG sp
-#define __PT_IP_REG epc
 
 #endif
 
-- 
2.34.1

[PATCH bpf-next v4 10/14] libbpf: Move data structure manipulation macros to bpf_common_helpers.h

[Ilya Leoshkevich]

These macros are useful for both libbpf and bpf progs, so put them into
a separate header dedicated to this use case.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/lib/bpf/Makefile                 |  2 +-
 tools/lib/bpf/bpf_common_helpers.h     | 30 ++++++++++++++++++++++++++
 tools/lib/bpf/bpf_helpers.h            | 15 +------------
 tools/testing/selftests/bpf/bpf_util.h | 10 +--------
 4 files changed, 33 insertions(+), 24 deletions(-)
 create mode 100644 tools/lib/bpf/bpf_common_helpers.h

diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
index b8b37fe76006..60b06c22e0a1 100644
--- a/tools/lib/bpf/Makefile
+++ b/tools/lib/bpf/Makefile
@@ -239,7 +239,7 @@ install_lib: all_cmd
 
 SRC_HDRS := bpf.h libbpf.h btf.h libbpf_common.h libbpf_legacy.h xsk.h	     \
 	    bpf_helpers.h bpf_tracing.h bpf_endian.h bpf_core_read.h	     \
-	    skel_internal.h libbpf_version.h
+	    skel_internal.h libbpf_version.h bpf_common_helpers.h
 GEN_HDRS := $(BPF_GENERATED)
 
 INSTALL_PFX := $(DESTDIR)$(prefix)/include/bpf
diff --git a/tools/lib/bpf/bpf_common_helpers.h b/tools/lib/bpf/bpf_common_helpers.h
new file mode 100644
index 000000000000..79db303b6ae2
--- /dev/null
+++ b/tools/lib/bpf/bpf_common_helpers.h
@@ -0,0 +1,30 @@
+/* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */
+#ifndef __BPF_COMMON_HELPERS__
+#define __BPF_COMMON_HELPERS__
+
+/*
+ * Helper macros that can be used both by libbpf and bpf progs.
+ */
+
+#ifndef offsetof
+#define offsetof(TYPE, MEMBER) ((unsigned long)&((TYPE *)0)->MEMBER)
+#endif
+
+#ifndef sizeof_field
+#define sizeof_field(TYPE, MEMBER) sizeof((((TYPE *)0)->MEMBER))
+#endif
+
+#ifndef offsetofend
+#define offsetofend(TYPE, MEMBER) \
+	(offsetof(TYPE, MEMBER) + sizeof_field(TYPE, MEMBER))
+#endif
+
+#ifndef container_of
+#define container_of(ptr, type, member)				\
+	({							\
+		void *__mptr = (void *)(ptr);			\
+		((type *)(__mptr - offsetof(type, member)));	\
+	})
+#endif
+
+#endif
diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
index 44df982d2a5c..1e8b609c1000 100644
--- a/tools/lib/bpf/bpf_helpers.h
+++ b/tools/lib/bpf/bpf_helpers.h
@@ -2,6 +2,7 @@
 #ifndef __BPF_HELPERS__
 #define __BPF_HELPERS__
 
+#include "bpf_common_helpers.h"
 /*
  * Note that bpf programs need to include either
  * vmlinux.h (auto-generated from BTF) or linux/types.h
@@ -61,20 +62,6 @@
 #define KERNEL_VERSION(a, b, c) (((a) << 16) + ((b) << 8) + ((c) > 255 ? 255 : (c)))
 #endif
 
-/*
- * Helper macros to manipulate data structures
- */
-#ifndef offsetof
-#define offsetof(TYPE, MEMBER)	((unsigned long)&((TYPE *)0)->MEMBER)
-#endif
-#ifndef container_of
-#define container_of(ptr, type, member)				\
-	({							\
-		void *__mptr = (void *)(ptr);			\
-		((type *)(__mptr - offsetof(type, member)));	\
-	})
-#endif
-
 /*
  * Helper macro to throw a compilation error if __bpf_unreachable() gets
  * built into the resulting code. This works given BPF back end does not
diff --git a/tools/testing/selftests/bpf/bpf_util.h b/tools/testing/selftests/bpf/bpf_util.h
index a3352a64c067..bc0b741b1eef 100644
--- a/tools/testing/selftests/bpf/bpf_util.h
+++ b/tools/testing/selftests/bpf/bpf_util.h
@@ -6,6 +6,7 @@
 #include <stdlib.h>
 #include <string.h>
 #include <errno.h>
+#include <bpf/bpf_common_helpers.h>
 #include <bpf/libbpf.h> /* libbpf_num_possible_cpus */
 
 static inline unsigned int bpf_num_possible_cpus(void)
@@ -31,13 +32,4 @@ static inline unsigned int bpf_num_possible_cpus(void)
 # define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
 #endif
 
-#ifndef sizeof_field
-#define sizeof_field(TYPE, MEMBER) sizeof((((TYPE *)0)->MEMBER))
-#endif
-
-#ifndef offsetofend
-#define offsetofend(TYPE, MEMBER) \
-	(offsetof(TYPE, MEMBER)	+ sizeof_field(TYPE, MEMBER))
-#endif
-
 #endif /* __BPF_UTIL__ */
-- 
2.34.1

[PATCH bpf-next v4 11/14] libbpf: Fix accessing the first syscall argument on s390

[Ilya Leoshkevich]

On s390, the first syscall argument should be accessed via orig_gpr2
(see arch/s390/include/asm/syscall.h). Currently gpr[2] is used
instead, leading to bpf_syscall_macro test failure.

Note that this is unfixable for CO-RE when vmlinux.h is not included.
Simply fail the build in this case.

Reported-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/lib/bpf/bpf_tracing.h | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
index 88ed5ba9510c..5911b177728f 100644
--- a/tools/lib/bpf/bpf_tracing.h
+++ b/tools/lib/bpf/bpf_tracing.h
@@ -2,6 +2,8 @@
 #ifndef __BPF_TRACING_H__
 #define __BPF_TRACING_H__
 
+#include <bpf/bpf_common_helpers.h>
+
 /* Scan the ARCH passed in from ARCH env variable (see Makefile) */
 #if defined(__TARGET_ARCH_x86)
 	#define bpf_target_x86
@@ -118,9 +120,20 @@
 
 #define __BPF_ARCH_HAS_SYSCALL_WRAPPER
 
-#if !defined(__KERNEL__) && !defined(__VMLINUX_H__)
+#if defined(__KERNEL__) || defined(__VMLINUX_H__)
+#define __PT_PARM1_REG_SYSCALL orig_gpr2
+#else
 /* s390 provides user_pt_regs instead of struct pt_regs to userspace */
 #define __PT_REGS_CAST(x) ((const user_pt_regs *)(x))
+/*
+ * struct pt_regs.orig_gpr2 is not exposed through user_pt_regs, and the ABI
+ * prohibits extending user_pt_regs. In non-CO-RE case, make use of the fact
+ * that orig_gpr2 comes right after gprs in struct pt_regs. CO-RE does not
+ * allow such hacks, so there is no way to access orig_gpr2.
+ */
+#define PT_REGS_PARM1_SYSCALL(x) \
+	(*(unsigned long *)(((char *)(x) + offsetofend(user_pt_regs, gprs))))
+#define __PT_PARM1_REG_SYSCALL __unsupported__
 #endif
 
 #define __PT_PARM1_REG gprs[2]
-- 
2.34.1

[PATCH bpf-next v4 12/14] s390: add a comment that warns that orig_gpr2 should not be moved

[Ilya Leoshkevich]

orig_gpr2's location is used by libbpf tracing macros, therefore it
should not be moved.

Suggested-by: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 arch/s390/include/asm/ptrace.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/s390/include/asm/ptrace.h b/arch/s390/include/asm/ptrace.h
index 4ffa8e7f0ed3..3c356ec59abc 100644
--- a/arch/s390/include/asm/ptrace.h
+++ b/arch/s390/include/asm/ptrace.h
@@ -85,6 +85,10 @@ struct pt_regs {
 			unsigned long gprs[NUM_GPRS];
 		};
 	};
+	/*
+	 * orig_gpr2 is not exposed via user_pt_regs, but its location is
+	 * assumed by libbpf's tracing macros, so it should not be moved.
+	 */
 	unsigned long orig_gpr2;
 	union {
 		struct {
-- 
2.34.1

[PATCH bpf-next v4 13/14] libbpf: Fix accessing the first syscall argument on arm64

[Ilya Leoshkevich]

On arm64, the first syscall argument should be accessed via orig_x0
(see arch/arm64/include/asm/syscall.h). Currently regs[0] is used
instead, leading to bpf_syscall_macro test failure.

Note that this is unfixable for CO-RE when vmlinux.h is not included.
Simply fail the build in this case.

Reported-by: Heiko Carstens <hca@linux.ibm.com>
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/lib/bpf/bpf_tracing.h | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
index 5911b177728f..f5541add5880 100644
--- a/tools/lib/bpf/bpf_tracing.h
+++ b/tools/lib/bpf/bpf_tracing.h
@@ -164,9 +164,21 @@
 
 #define __BPF_ARCH_HAS_SYSCALL_WRAPPER
 
-#if !defined(__KERNEL__) && !defined(__VMLINUX_H__)
+#if defined(__KERNEL__) || defined(__VMLINUX_H__)
+#define __PT_PARM1_REG_SYSCALL orig_x0
+#else
 /* arm64 provides struct user_pt_regs instead of struct pt_regs to userspace */
 #define __PT_REGS_CAST(x) ((const struct user_pt_regs *)(x))
+/*
+ * struct pt_regs.orig_x0 is not exposed through struct user_pt_regs, and the
+ * ABI prohibits extending struct user_pt_regs. In non-CO-RE case, make use of
+ * the fact that orig_x0 comes right after pstate in struct pt_regs. CO-RE does
+ * not allow such hacks, so there is no way to access orig_x0.
+ */
+#define PT_REGS_PARM1_SYSCALL(x) \
+	(*(unsigned long *)(((char *)(x) + \
+			     offsetofend(struct user_pt_regs, pstate))))
+#define __PT_PARM1_REG_SYSCALL __unsupported__
 #endif
 
 #define __PT_PARM1_REG regs[0]
-- 
2.34.1

[PATCH bpf-next v4 14/14] arm64: add a comment that warns that orig_x0 should not be moved

[Ilya Leoshkevich]

orig_x0's location is used by libbpf tracing macros, therefore it
should not be moved.

Suggested-by: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 arch/arm64/include/asm/ptrace.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
index 41b332c054ab..7e34c3737839 100644
--- a/arch/arm64/include/asm/ptrace.h
+++ b/arch/arm64/include/asm/ptrace.h
@@ -185,6 +185,10 @@ struct pt_regs {
 			u64 pstate;
 		};
 	};
+	/*
+	 * orig_x0 is not exposed via struct user_pt_regs, but its location is
+	 * assumed by libbpf's tracing macros, so it should not be moved.
+	 */
 	u64 orig_x0;
 #ifdef __AARCH64EB__
 	u32 unused2;
-- 
2.34.1

Re: [PATCH bpf-next v4 11/14] libbpf: Fix accessing the first syscall argument on s390

[Ilya Leoshkevich]

On Tue, 2022-02-08 at 06:16 +0100, Ilya Leoshkevich wrote:
> On s390, the first syscall argument should be accessed via orig_gpr2
> (see arch/s390/include/asm/syscall.h). Currently gpr[2] is used
> instead, leading to bpf_syscall_macro test failure.
> 
> Note that this is unfixable for CO-RE when vmlinux.h is not included.
> Simply fail the build in this case.
> 
> Reported-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  tools/lib/bpf/bpf_tracing.h | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/lib/bpf/bpf_tracing.h
> b/tools/lib/bpf/bpf_tracing.h
> index 88ed5ba9510c..5911b177728f 100644
> --- a/tools/lib/bpf/bpf_tracing.h
> +++ b/tools/lib/bpf/bpf_tracing.h
> @@ -2,6 +2,8 @@
>  #ifndef __BPF_TRACING_H__
>  #define __BPF_TRACING_H__
>  
> +#include <bpf/bpf_common_helpers.h>
> +
>  /* Scan the ARCH passed in from ARCH env variable (see Makefile) */
>  #if defined(__TARGET_ARCH_x86)
>         #define bpf_target_x86
> @@ -118,9 +120,20 @@
>  
>  #define __BPF_ARCH_HAS_SYSCALL_WRAPPER
>  
> -#if !defined(__KERNEL__) && !defined(__VMLINUX_H__)
> +#if defined(__KERNEL__) || defined(__VMLINUX_H__)
> +#define __PT_PARM1_REG_SYSCALL orig_gpr2
> +#else
>  /* s390 provides user_pt_regs instead of struct pt_regs to userspace
> */
>  #define __PT_REGS_CAST(x) ((const user_pt_regs *)(x))
> +/*
> + * struct pt_regs.orig_gpr2 is not exposed through user_pt_regs, and
> the ABI
> + * prohibits extending user_pt_regs. In non-CO-RE case, make use of
> the fact
> + * that orig_gpr2 comes right after gprs in struct pt_regs. CO-RE
> does not
> + * allow such hacks, so there is no way to access orig_gpr2.
> + */
> +#define PT_REGS_PARM1_SYSCALL(x) \
> +       (*(unsigned long *)(((char *)(x) + offsetofend(user_pt_regs,
> gprs))))
> +#define __PT_PARM1_REG_SYSCALL __unsupported__
>  #endif
>  
>  #define __PT_PARM1_REG gprs[2]

This might be too pessimistic.

While I don't see a way to add real CO-RE support here, and doing
something like __CORE_RELO(gprs, BYTE_OFFSET) + __CORE_RELO(gprs,
BYTE_SIZE) defeats the purpose of using CO-RE, we promise not to move
orig_gpr2 around. So just this:

#define PT_REGS_PARM1_CORE_SYSCALL(x) ({\
	unsigned long __tmp; \
	bpf_probe_read_kernel(&tmp, sizeof(__tmp),
&PT_REGS_PARM1_SYSCALL(x)); \
	tmp; \
})

does the trick here in a sense that, having been compiled once, it
would run everywhere. What do you think?

Re: [PATCH bpf-next v4 14/14] arm64: add a comment that warns that orig_x0 should not be moved

[Alexei Starovoitov]

On Tue, Feb 08, 2022 at 06:16:35AM +0100, Ilya Leoshkevich wrote:
> orig_x0's location is used by libbpf tracing macros, therefore it
> should not be moved.
> 
> Suggested-by: Andrii Nakryiko <andrii.nakryiko@gmail.com>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  arch/arm64/include/asm/ptrace.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
> index 41b332c054ab..7e34c3737839 100644
> --- a/arch/arm64/include/asm/ptrace.h
> +++ b/arch/arm64/include/asm/ptrace.h
> @@ -185,6 +185,10 @@ struct pt_regs {
>  			u64 pstate;
>  		};
>  	};
> +	/*
> +	 * orig_x0 is not exposed via struct user_pt_regs, but its location is
> +	 * assumed by libbpf's tracing macros, so it should not be moved.
> +	 */

In other words this comment is saying that the layout is ABI.
That's not the case. orig_x0 here and equivalent on s390 can be moved.
It will break bpf progs written without CO-RE and that is expected.
Non CO-RE programs often do all kinds of bpf_probe_read_kernel and
will be breaking when kernel layout is changing.
I suggest to drop this patch and patch 12.

Re: [PATCH bpf-next v4 14/14] arm64: add a comment that warns that orig_x0 should not be moved

[Ilya Leoshkevich]

On Tue, 2022-02-08 at 11:25 -0800, Alexei Starovoitov wrote:
> On Tue, Feb 08, 2022 at 06:16:35AM +0100, Ilya Leoshkevich wrote:
> > orig_x0's location is used by libbpf tracing macros, therefore it
> > should not be moved.
> > 
> > Suggested-by: Andrii Nakryiko <andrii.nakryiko@gmail.com>
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > ---
> >  arch/arm64/include/asm/ptrace.h | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/arch/arm64/include/asm/ptrace.h
> > b/arch/arm64/include/asm/ptrace.h
> > index 41b332c054ab..7e34c3737839 100644
> > --- a/arch/arm64/include/asm/ptrace.h
> > +++ b/arch/arm64/include/asm/ptrace.h
> > @@ -185,6 +185,10 @@ struct pt_regs {
> >                         u64 pstate;
> >                 };
> >         };
> > +       /*
> > +        * orig_x0 is not exposed via struct user_pt_regs, but its
> > location is
> > +        * assumed by libbpf's tracing macros, so it should not be
> > moved.
> > +        */
> 
> In other words this comment is saying that the layout is ABI.
> That's not the case. orig_x0 here and equivalent on s390 can be
> moved.
> It will break bpf progs written without CO-RE and that is expected.
> Non CO-RE programs often do all kinds of bpf_probe_read_kernel and
> will be breaking when kernel layout is changing.
> I suggest to drop this patch and patch 12.

Yeah, that was the intention here: to promote orig_x0 to ABI using a
comment, since doing this by extending user_pt_regs turned out to be
infeasible. I'm actually ok with not doing this, since programs
compiled with kernel headers and using CO-RE macros will be fine.

As you say, we don't care about programs that don't use CO-RE too much
here - if they break after an incompatible kernel change, fine.

The question now is - how much do we care about programs that are
compiled with userspace headers? Andrii suggested to use offsetofend to
make syscall macros work there, however, this now requires this ABI
promotion.

Re: [PATCH bpf-next v4 14/14] arm64: add a comment that warns that orig_x0 should not be moved

[Alexei Starovoitov]

On Tue, Feb 8, 2022 at 11:46 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> On Tue, 2022-02-08 at 11:25 -0800, Alexei Starovoitov wrote:
> > On Tue, Feb 08, 2022 at 06:16:35AM +0100, Ilya Leoshkevich wrote:
> > > orig_x0's location is used by libbpf tracing macros, therefore it
> > > should not be moved.
> > >
> > > Suggested-by: Andrii Nakryiko <andrii.nakryiko@gmail.com>
> > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > > ---
> > >  arch/arm64/include/asm/ptrace.h | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/arch/arm64/include/asm/ptrace.h
> > > b/arch/arm64/include/asm/ptrace.h
> > > index 41b332c054ab..7e34c3737839 100644
> > > --- a/arch/arm64/include/asm/ptrace.h
> > > +++ b/arch/arm64/include/asm/ptrace.h
> > > @@ -185,6 +185,10 @@ struct pt_regs {
> > >                         u64 pstate;
> > >                 };
> > >         };
> > > +       /*
> > > +        * orig_x0 is not exposed via struct user_pt_regs, but its
> > > location is
> > > +        * assumed by libbpf's tracing macros, so it should not be
> > > moved.
> > > +        */
> >
> > In other words this comment is saying that the layout is ABI.
> > That's not the case. orig_x0 here and equivalent on s390 can be
> > moved.
> > It will break bpf progs written without CO-RE and that is expected.
> > Non CO-RE programs often do all kinds of bpf_probe_read_kernel and
> > will be breaking when kernel layout is changing.
> > I suggest to drop this patch and patch 12.
>
> Yeah, that was the intention here: to promote orig_x0 to ABI using a
> comment, since doing this by extending user_pt_regs turned out to be
> infeasible. I'm actually ok with not doing this, since programs
> compiled with kernel headers and using CO-RE macros will be fine.

The comment like this doesn't convert kernel internal struct into ABI.
The comment is just wrong. BPF progs access many kernel data structs.
s390's and arm64's struct pr_regs is not special in that sense.
It's an internal struct.

> As you say, we don't care about programs that don't use CO-RE too much
> here - if they break after an incompatible kernel change, fine.

Before CO-RE was introduced bpf progs included kernel headers
and were breaking when kernel changes. Nothing new here.
See the history of bcc tools. Some of them are littered
with ifdef VERSION ==.

> The question now is - how much do we care about programs that are

> compiled with userspace headers? Andrii suggested to use offsetofend to
> make syscall macros work there, however, this now requires this ABI
> promotion.

Today s390 and arm64 have user_pt_regs as a first field in pt_regs.
That is kernel internal behavior and that part can change if arch
maintainers have a need for that.
bpf progs without CO-RE would have to be adjusted when kernel changes.
Even with CO-RE it's ok to rename pt_regs->orig_gpr2 on s390.
The progs with CO-RE will break too. The authors of tracing bpf progs
have to expect that sooner or later their progs will break and they
would have to adjust them.

Re: [PATCH bpf-next v4 14/14] arm64: add a comment that warns that orig_x0 should not be moved

[Ilya Leoshkevich]

On Tue, 2022-02-08 at 13:11 -0800, Alexei Starovoitov wrote:
> On Tue, Feb 8, 2022 at 11:46 AM Ilya Leoshkevich <iii@linux.ibm.com>
> wrote:
> > 
> > On Tue, 2022-02-08 at 11:25 -0800, Alexei Starovoitov wrote:
> > > On Tue, Feb 08, 2022 at 06:16:35AM +0100, Ilya Leoshkevich wrote:
> > > > orig_x0's location is used by libbpf tracing macros, therefore
> > > > it
> > > > should not be moved.
> > > > 
> > > > Suggested-by: Andrii Nakryiko <andrii.nakryiko@gmail.com>
> > > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > > > ---
> > > >  arch/arm64/include/asm/ptrace.h | 4 ++++
> > > >  1 file changed, 4 insertions(+)
> > > > 
> > > > diff --git a/arch/arm64/include/asm/ptrace.h
> > > > b/arch/arm64/include/asm/ptrace.h
> > > > index 41b332c054ab..7e34c3737839 100644
> > > > --- a/arch/arm64/include/asm/ptrace.h
> > > > +++ b/arch/arm64/include/asm/ptrace.h
> > > > @@ -185,6 +185,10 @@ struct pt_regs {
> > > >                         u64 pstate;
> > > >                 };
> > > >         };
> > > > +       /*
> > > > +        * orig_x0 is not exposed via struct user_pt_regs, but
> > > > its
> > > > location is
> > > > +        * assumed by libbpf's tracing macros, so it should not
> > > > be
> > > > moved.
> > > > +        */
> > > 
> > > In other words this comment is saying that the layout is ABI.
> > > That's not the case. orig_x0 here and equivalent on s390 can be
> > > moved.
> > > It will break bpf progs written without CO-RE and that is
> > > expected.
> > > Non CO-RE programs often do all kinds of bpf_probe_read_kernel
> > > and
> > > will be breaking when kernel layout is changing.
> > > I suggest to drop this patch and patch 12.
> > 
> > Yeah, that was the intention here: to promote orig_x0 to ABI using
> > a
> > comment, since doing this by extending user_pt_regs turned out to
> > be
> > infeasible. I'm actually ok with not doing this, since programs
> > compiled with kernel headers and using CO-RE macros will be fine.
> 
> The comment like this doesn't convert kernel internal struct into
> ABI.
> The comment is just wrong. BPF progs access many kernel data structs.
> s390's and arm64's struct pr_regs is not special in that sense.
> It's an internal struct.
> 
> > As you say, we don't care about programs that don't use CO-RE too
> > much
> > here - if they break after an incompatible kernel change, fine.
> 
> Before CO-RE was introduced bpf progs included kernel headers
> and were breaking when kernel changes. Nothing new here.
> See the history of bcc tools. Some of them are littered
> with ifdef VERSION ==.
> 
> > The question now is - how much do we care about programs that are
> 
> > compiled with userspace headers? Andrii suggested to use
> > offsetofend to
> > make syscall macros work there, however, this now requires this ABI
> > promotion.
> 
> Today s390 and arm64 have user_pt_regs as a first field in pt_regs.
> That is kernel internal behavior and that part can change if arch
> maintainers have a need for that.
> bpf progs without CO-RE would have to be adjusted when kernel
> changes.
> Even with CO-RE it's ok to rename pt_regs->orig_gpr2 on s390.
> The progs with CO-RE will break too. The authors of tracing bpf progs
> have to expect that sooner or later their progs will break and they
> would have to adjust them.

When it comes to authors of tracing bpf progs, I agree that eventually
they will have to adjust their code, CO-RE or not. However, in patch 13
I introduce the following libbpf macro:

#if defined(__KERNEL__) || defined(__VMLINUX_H__)
...
#else
#define PT_REGS_PARM1_SYSCALL(x) \
	(*(unsigned long *)(((char *)(x) + \
			     offsetofend(struct user_pt_regs,
pstate))))

If we merge this series without freezing orig_x0's offset, in case of
an incompatible kernel change the users of PT_REGS_PARMn_SYSCALL and
BPF_KPROBE_SYSCALL, who build against userspace headers, will not
simply have to update their code - they will have to upgrade libbpf.
It's also not clear how PT_REGS_PARM1_SYSCALL in the upgraded libbpf 
will even look like, given that it would need to work on both old and
new kernels.

I've also briefly looked into MIPS' ptrace.h, and it looks as if their
user_pt_regs has no relationship to kernel pt_regs. IIUC this means
that it's not possible to correctly implement PT_REGS_PARMn_SYSCALL
using MIPS userspace headers.

So I wonder whether we should allow using PT_REGS_PARMn_SYSCALL and
BPF_KPROBE_SYSCALL with userspace headers at all? Would it make sense
to simply fail the compilation if PT_REGS_PARMn_SYSCALL is used without
including kernel headers?

Re: [PATCH bpf-next v4 05/14] libbpf: Generalize overriding syscall parameter access macros

[Andrii Nakryiko]

On Mon, Feb 7, 2022 at 9:16 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> Instead of conditionally overriding PT_REGS_PARM4_SYSCALL, provide
> default fallbacks for all __PT_PARMn_REG_SYSCALL macros, so that
> architectures can simply override a specific syscall parameter macro.
> Also allow completely overriding PT_REGS_PARM1_SYSCALL for
> non-trivial access sequences.
>
> Co-developed-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  tools/lib/bpf/bpf_tracing.h | 48 +++++++++++++++++++++++++------------
>  1 file changed, 33 insertions(+), 15 deletions(-)
>
> diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
> index da7e8d5c939c..82f1e935d549 100644
> --- a/tools/lib/bpf/bpf_tracing.h
> +++ b/tools/lib/bpf/bpf_tracing.h
> @@ -265,25 +265,43 @@ struct pt_regs;
>
>  #endif
>
> -#define PT_REGS_PARM1_SYSCALL(x) PT_REGS_PARM1(x)
> -#define PT_REGS_PARM2_SYSCALL(x) PT_REGS_PARM2(x)
> -#define PT_REGS_PARM3_SYSCALL(x) PT_REGS_PARM3(x)
> -#ifdef __PT_PARM4_REG_SYSCALL
> +#ifndef __PT_PARM1_REG_SYSCALL
> +#define __PT_PARM1_REG_SYSCALL __PT_PARM1_REG
> +#endif
> +#ifndef __PT_PARM2_REG_SYSCALL
> +#define __PT_PARM2_REG_SYSCALL __PT_PARM2_REG
> +#endif
> +#ifndef __PT_PARM3_REG_SYSCALL
> +#define __PT_PARM3_REG_SYSCALL __PT_PARM3_REG
> +#endif
> +#ifndef __PT_PARM4_REG_SYSCALL
> +#define __PT_PARM4_REG_SYSCALL __PT_PARM4_REG
> +#endif
> +#ifndef __PT_PARM5_REG_SYSCALL
> +#define __PT_PARM5_REG_SYSCALL __PT_PARM5_REG
> +#endif
> +
> +#ifndef PT_REGS_PARM1_SYSCALL
> +#define PT_REGS_PARM1_SYSCALL(x) (__PT_REGS_CAST(x)->__PT_PARM1_REG_SYSCALL)
> +#endif
> +#ifndef PT_REGS_PARM2_SYSCALL
> +#define PT_REGS_PARM2_SYSCALL(x) (__PT_REGS_CAST(x)->__PT_PARM2_REG_SYSCALL)
> +#endif
> +#ifndef PT_REGS_PARM3_SYSCALL
> +#define PT_REGS_PARM3_SYSCALL(x) (__PT_REGS_CAST(x)->__PT_PARM3_REG_SYSCALL)
> +#endif
> +#ifndef PT_REGS_PARM4_SYSCALL
>  #define PT_REGS_PARM4_SYSCALL(x) (__PT_REGS_CAST(x)->__PT_PARM4_REG_SYSCALL)
> -#else /* __PT_PARM4_REG_SYSCALL */
> -#define PT_REGS_PARM4_SYSCALL(x) PT_REGS_PARM4(x)
>  #endif
> -#define PT_REGS_PARM5_SYSCALL(x) PT_REGS_PARM5(x)
> +#ifndef PT_REGS_PARM5_SYSCALL
> +#define PT_REGS_PARM5_SYSCALL(x) (__PT_REGS_CAST(x)->__PT_PARM5_REG_SYSCALL)
> +#endif
>
> -#define PT_REGS_PARM1_CORE_SYSCALL(x) PT_REGS_PARM1_CORE(x)
> -#define PT_REGS_PARM2_CORE_SYSCALL(x) PT_REGS_PARM2_CORE(x)
> -#define PT_REGS_PARM3_CORE_SYSCALL(x) PT_REGS_PARM3_CORE(x)
> -#ifdef __PT_PARM4_REG_SYSCALL
> +#define PT_REGS_PARM1_CORE_SYSCALL(x) BPF_CORE_READ(__PT_REGS_CAST(x), __PT_PARM1_REG_SYSCALL)
> +#define PT_REGS_PARM2_CORE_SYSCALL(x) BPF_CORE_READ(__PT_REGS_CAST(x), __PT_PARM2_REG_SYSCALL)
> +#define PT_REGS_PARM3_CORE_SYSCALL(x) BPF_CORE_READ(__PT_REGS_CAST(x), __PT_PARM3_REG_SYSCALL)
>  #define PT_REGS_PARM4_CORE_SYSCALL(x) BPF_CORE_READ(__PT_REGS_CAST(x), __PT_PARM4_REG_SYSCALL)
> -#else /* __PT_PARM4_REG_SYSCALL */
> -#define PT_REGS_PARM4_CORE_SYSCALL(x) PT_REGS_PARM4_CORE(x)
> -#endif
> -#define PT_REGS_PARM5_CORE_SYSCALL(x) PT_REGS_PARM5_CORE(x)
> +#define PT_REGS_PARM5_CORE_SYSCALL(x) BPF_CORE_READ(__PT_REGS_CAST(x), __PT_PARM5_REG_SYSCALL)
>

No, please don't do it. It makes CORE variants too rigid. We agreed w/
Naveen that the way you did it in v2 is better and more flexible and
in v3 you did it the other way. Why?

>  #else /* defined(bpf_target_defined) */
>
> --
> 2.34.1
>

Re: [PATCH bpf-next v4 03/14] selftests/bpf: Compile bpf_syscall_macro test also with user headers

[Andrii Nakryiko]

On Mon, Feb 7, 2022 at 9:16 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> Verify that using linux/ptrace.h instead of vmlinux.h works fine.
> Since without vmlinux.h and with CO-RE it's not possible to access the
> first syscall argument on arm64 and s390x, and any syscall arguments on
> Intel, skip the corresponding checks.
>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  ...call_macro.c => test_bpf_syscall_macro_common.h} |  8 +++++++-
>  .../bpf/prog_tests/test_bpf_syscall_macro_kernel.c  | 13 +++++++++++++
>  .../bpf/prog_tests/test_bpf_syscall_macro_user.c    | 13 +++++++++++++
>  ...f_syscall_macro.c => bpf_syscall_macro_common.h} |  8 ++++++--
>  .../selftests/bpf/progs/bpf_syscall_macro_kernel.c  |  4 ++++
>  .../selftests/bpf/progs/bpf_syscall_macro_user.c    | 10 ++++++++++
>  6 files changed, 53 insertions(+), 3 deletions(-)
>  rename tools/testing/selftests/bpf/prog_tests/{test_bpf_syscall_macro.c => test_bpf_syscall_macro_common.h} (89%)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/test_bpf_syscall_macro_kernel.c
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/test_bpf_syscall_macro_user.c
>  rename tools/testing/selftests/bpf/progs/{bpf_syscall_macro.c => bpf_syscall_macro_common.h} (87%)
>  create mode 100644 tools/testing/selftests/bpf/progs/bpf_syscall_macro_kernel.c
>  create mode 100644 tools/testing/selftests/bpf/progs/bpf_syscall_macro_user.c
>
> 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_common.h
> similarity index 89%
> rename from tools/testing/selftests/bpf/prog_tests/test_bpf_syscall_macro.c
> rename to tools/testing/selftests/bpf/prog_tests/test_bpf_syscall_macro_common.h
> index f5f4c8adf539..9f2a395abff7 100644
> --- a/tools/testing/selftests/bpf/prog_tests/test_bpf_syscall_macro.c
> +++ b/tools/testing/selftests/bpf/prog_tests/test_bpf_syscall_macro_common.h
> @@ -2,7 +2,6 @@
>  /* Copyright 2022 Sony Group Corporation */
>  #include <sys/prctl.h>
>  #include <test_progs.h>
> -#include "bpf_syscall_macro.skel.h"
>
>  void test_bpf_syscall_macro(void)
>  {
> @@ -46,7 +45,13 @@ void test_bpf_syscall_macro(void)
>         ASSERT_EQ(skel->bss->arg5, exp_arg5, "syscall_arg5");
>
>         /* check whether args of syscall are copied correctly for CORE variants */
> +#if defined(__BPF_SYSCALL_MACRO_KERNEL_SKEL_H__) || \
> +               (!defined(__s390__) && !defined(__aarch64__) && \
> +                !defined(__i386__) && !defined(__x86_64__))

All this is horrible, please no. I think we have better ways to do it
with CO-RE.

>         ASSERT_EQ(skel->bss->arg1_core, exp_arg1, "syscall_arg1_core_variant");
> +#endif
> +#if defined(__BPF_SYSCALL_MACRO_KERNEL_SKEL_H__) || \
> +               (!defined(__i386__) && !defined(__x86_64__))
>         ASSERT_EQ(skel->bss->arg2_core, exp_arg2, "syscall_arg2_core_variant");
>         ASSERT_EQ(skel->bss->arg3_core, exp_arg3, "syscall_arg3_core_variant");
>         /* it cannot copy arg4 when uses PT_REGS_PARM4_CORE on x86_64 */
> @@ -57,6 +62,7 @@ void test_bpf_syscall_macro(void)
>  #endif
>         ASSERT_EQ(skel->bss->arg4_core, exp_arg4, "syscall_arg4_core_variant");
>         ASSERT_EQ(skel->bss->arg5_core, exp_arg5, "syscall_arg5_core_variant");
> +#endif
>
>  cleanup:
>         bpf_syscall_macro__destroy(skel);
> diff --git a/tools/testing/selftests/bpf/prog_tests/test_bpf_syscall_macro_kernel.c b/tools/testing/selftests/bpf/prog_tests/test_bpf_syscall_macro_kernel.c
> new file mode 100644
> index 000000000000..7ceabd62bb0f
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/test_bpf_syscall_macro_kernel.c
> @@ -0,0 +1,13 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include "bpf_syscall_macro_kernel.skel.h"
> +
> +void test_bpf_syscall_macro_kernel(void);
> +
> +#define test_bpf_syscall_macro test_bpf_syscall_macro_kernel
> +#define bpf_syscall_macro bpf_syscall_macro_kernel
> +#define bpf_syscall_macro__open bpf_syscall_macro_kernel__open
> +#define bpf_syscall_macro__load bpf_syscall_macro_kernel__load
> +#define bpf_syscall_macro__attach bpf_syscall_macro_kernel__attach
> +#define bpf_syscall_macro__destroy bpf_syscall_macro_kernel__destroy
> +
> +#include "test_bpf_syscall_macro_common.h"
> diff --git a/tools/testing/selftests/bpf/prog_tests/test_bpf_syscall_macro_user.c b/tools/testing/selftests/bpf/prog_tests/test_bpf_syscall_macro_user.c
> new file mode 100644
> index 000000000000..f31558f14e7e
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/test_bpf_syscall_macro_user.c
> @@ -0,0 +1,13 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include "bpf_syscall_macro_user.skel.h"
> +
> +void test_bpf_syscall_macro_user(void);
> +
> +#define test_bpf_syscall_macro test_bpf_syscall_macro_user
> +#define bpf_syscall_macro bpf_syscall_macro_user
> +#define bpf_syscall_macro__open bpf_syscall_macro_user__open
> +#define bpf_syscall_macro__load bpf_syscall_macro_user__load
> +#define bpf_syscall_macro__attach bpf_syscall_macro_user__attach
> +#define bpf_syscall_macro__destroy bpf_syscall_macro_user__destroy
> +
> +#include "test_bpf_syscall_macro_common.h"
> diff --git a/tools/testing/selftests/bpf/progs/bpf_syscall_macro.c b/tools/testing/selftests/bpf/progs/bpf_syscall_macro_common.h
> similarity index 87%
> rename from tools/testing/selftests/bpf/progs/bpf_syscall_macro.c
> rename to tools/testing/selftests/bpf/progs/bpf_syscall_macro_common.h
> index f5c6ef2ff6d1..8717605358d3 100644
> --- a/tools/testing/selftests/bpf/progs/bpf_syscall_macro.c
> +++ b/tools/testing/selftests/bpf/progs/bpf_syscall_macro_common.h
> @@ -1,7 +1,5 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /* Copyright 2022 Sony Group Corporation */
> -#include <vmlinux.h>
> -
>  #include <bpf/bpf_core_read.h>
>  #include <bpf/bpf_helpers.h>
>  #include <bpf/bpf_tracing.h>
> @@ -46,12 +44,18 @@ int BPF_KPROBE(handle_sys_prctl)
>         bpf_probe_read_kernel(&arg5, sizeof(arg5), &PT_REGS_PARM5_SYSCALL(real_regs));
>
>         /* test for the CORE variant of PT_REGS_PARM */
> +#if defined(__KERNEL__) || defined(__VMLINUX_H__) || \
> +               (!defined(bpf_target_s390) && !defined(bpf_target_arm64) && \
> +                !defined(bpf_target_x86))
>         arg1_core = PT_REGS_PARM1_CORE_SYSCALL(real_regs);
> +#endif
> +#if defined(__KERNEL__) || defined(__VMLINUX_H__) || !defined(bpf_target_x86)
>         arg2_core = PT_REGS_PARM2_CORE_SYSCALL(real_regs);
>         arg3_core = PT_REGS_PARM3_CORE_SYSCALL(real_regs);
>         arg4_core_cx = PT_REGS_PARM4_CORE(real_regs);
>         arg4_core = PT_REGS_PARM4_CORE_SYSCALL(real_regs);
>         arg5_core = PT_REGS_PARM5_CORE_SYSCALL(real_regs);
> +#endif
>
>         return 0;
>  }
> diff --git a/tools/testing/selftests/bpf/progs/bpf_syscall_macro_kernel.c b/tools/testing/selftests/bpf/progs/bpf_syscall_macro_kernel.c
> new file mode 100644
> index 000000000000..1affac21266d
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/bpf_syscall_macro_kernel.c
> @@ -0,0 +1,4 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <vmlinux.h>
> +
> +#include "bpf_syscall_macro_common.h"
> diff --git a/tools/testing/selftests/bpf/progs/bpf_syscall_macro_user.c b/tools/testing/selftests/bpf/progs/bpf_syscall_macro_user.c
> new file mode 100644
> index 000000000000..1c078d528e8c
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/bpf_syscall_macro_user.c
> @@ -0,0 +1,10 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/ptrace.h>
> +#include <linux/types.h>
> +#include <sys/types.h>
> +
> +#include "bpf_syscall_macro_common.h"
> +
> +#if defined(__KERNEL__) || defined(__VMLINUX_H__)
> +#error This test must be compiled with userspace headers
> +#endif
> --
> 2.34.1
>

Re: [PATCH bpf-next v4 06/14] libbpf: Add PT_REGS_SYSCALL_REGS macro

[Andrii Nakryiko]

On Mon, Feb 7, 2022 at 9:16 PM Ilya Leoshkevich <iii@linux.ibm.com> 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@linux.ibm.com>
> Co-developed-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---

Again, there was nothing wrong with the way you did it in v3, please
revert to that one.

>  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().
> + */
> +#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
>

Re: [PATCH bpf-next v4 08/14] libbpf: Use struct pt_regs when compiling with kernel headers

[Andrii Nakryiko]

On Mon, Feb 7, 2022 at 9:16 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> Andrii says: "... with CO-RE and vmlinux.h it would be more reliable
> and straightforward to just stick to kernel-internal struct pt_regs
> everywhere ...".
>
> Actually, if vmlinux.h is available, then it's ok to do so for both
> CO-RE and non-CO-RE cases, since the beginning of struct pt_regs must
> match (struct) user_pt_regs, which must never change.
>
> Implement this by not defining __PT_REGS_CAST if the user included
> vmlinux.h.
>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---

If we are using CO-RE we don't have to assume vmlinux.h, we can define
our own definition of pt_regs with custom "flavor":

struct pt_regs___s390x {
    long gprs[10];
    long orig_gpr2; /* whatever the right types and names, but order
doesn't matter */
} __attribute__((preserve_access_index));


And then use `struct pt_regs__s390x` for s390x macros. That way we
don't assume any specific included header, we have minimal definition
we need (and it can be different for each architecture. It's still
CO-RE, still relocatable, and we don't need all these ugly #if
defined() checks.

>  tools/lib/bpf/bpf_tracing.h | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
> index 7a015ee8fb11..07e291d77e83 100644
> --- a/tools/lib/bpf/bpf_tracing.h
> +++ b/tools/lib/bpf/bpf_tracing.h
> @@ -118,8 +118,11 @@
>
>  #define __BPF_ARCH_HAS_SYSCALL_WRAPPER
>
> +#if !defined(__KERNEL__) && !defined(__VMLINUX_H__)
>  /* s390 provides user_pt_regs instead of struct pt_regs to userspace */
>  #define __PT_REGS_CAST(x) ((const user_pt_regs *)(x))
> +#endif
> +
>  #define __PT_PARM1_REG gprs[2]
>  #define __PT_PARM2_REG gprs[3]
>  #define __PT_PARM3_REG gprs[4]
> @@ -148,8 +151,11 @@
>
>  #define __BPF_ARCH_HAS_SYSCALL_WRAPPER
>
> +#if !defined(__KERNEL__) && !defined(__VMLINUX_H__)
>  /* arm64 provides struct user_pt_regs instead of struct pt_regs to userspace */
>  #define __PT_REGS_CAST(x) ((const struct user_pt_regs *)(x))
> +#endif
> +
>  #define __PT_PARM1_REG regs[0]
>  #define __PT_PARM2_REG regs[1]
>  #define __PT_PARM3_REG regs[2]
> @@ -207,7 +213,10 @@
>
>  #elif defined(bpf_target_riscv)
>
> +#if !defined(__KERNEL__) && !defined(__VMLINUX_H__)
>  #define __PT_REGS_CAST(x) ((const struct user_regs_struct *)(x))
> +#endif
> +
>  #define __PT_PARM1_REG a0
>  #define __PT_PARM2_REG a1
>  #define __PT_PARM3_REG a2
> --
> 2.34.1
>

Re: [PATCH bpf-next v4 10/14] libbpf: Move data structure manipulation macros to bpf_common_helpers.h

[Andrii Nakryiko]

On Mon, Feb 7, 2022 at 9:16 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> These macros are useful for both libbpf and bpf progs, so put them into
> a separate header dedicated to this use case.
>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  tools/lib/bpf/Makefile                 |  2 +-
>  tools/lib/bpf/bpf_common_helpers.h     | 30 ++++++++++++++++++++++++++
>  tools/lib/bpf/bpf_helpers.h            | 15 +------------
>  tools/testing/selftests/bpf/bpf_util.h | 10 +--------
>  4 files changed, 33 insertions(+), 24 deletions(-)
>  create mode 100644 tools/lib/bpf/bpf_common_helpers.h
>
> diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
> index b8b37fe76006..60b06c22e0a1 100644
> --- a/tools/lib/bpf/Makefile
> +++ b/tools/lib/bpf/Makefile
> @@ -239,7 +239,7 @@ install_lib: all_cmd
>
>  SRC_HDRS := bpf.h libbpf.h btf.h libbpf_common.h libbpf_legacy.h xsk.h      \
>             bpf_helpers.h bpf_tracing.h bpf_endian.h bpf_core_read.h         \
> -           skel_internal.h libbpf_version.h
> +           skel_internal.h libbpf_version.h bpf_common_helpers.h

Wait, how did we get from fixing s390x syscall arg fetching to
exposing a new public API header from libbpf?... I feel like I missed
a few revisions and discussion threads.

>  GEN_HDRS := $(BPF_GENERATED)
>
>  INSTALL_PFX := $(DESTDIR)$(prefix)/include/bpf
> diff --git a/tools/lib/bpf/bpf_common_helpers.h b/tools/lib/bpf/bpf_common_helpers.h
> new file mode 100644
> index 000000000000..79db303b6ae2
> --- /dev/null
> +++ b/tools/lib/bpf/bpf_common_helpers.h
> @@ -0,0 +1,30 @@
> +/* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */
> +#ifndef __BPF_COMMON_HELPERS__
> +#define __BPF_COMMON_HELPERS__
> +
> +/*
> + * Helper macros that can be used both by libbpf and bpf progs.
> + */
> +
> +#ifndef offsetof
> +#define offsetof(TYPE, MEMBER) ((unsigned long)&((TYPE *)0)->MEMBER)
> +#endif
> +
> +#ifndef sizeof_field
> +#define sizeof_field(TYPE, MEMBER) sizeof((((TYPE *)0)->MEMBER))
> +#endif
> +
> +#ifndef offsetofend
> +#define offsetofend(TYPE, MEMBER) \
> +       (offsetof(TYPE, MEMBER) + sizeof_field(TYPE, MEMBER))
> +#endif
> +
> +#ifndef container_of
> +#define container_of(ptr, type, member)                                \
> +       ({                                                      \
> +               void *__mptr = (void *)(ptr);                   \
> +               ((type *)(__mptr - offsetof(type, member)));    \
> +       })
> +#endif
> +
> +#endif
> diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
> index 44df982d2a5c..1e8b609c1000 100644
> --- a/tools/lib/bpf/bpf_helpers.h
> +++ b/tools/lib/bpf/bpf_helpers.h
> @@ -2,6 +2,7 @@
>  #ifndef __BPF_HELPERS__
>  #define __BPF_HELPERS__
>
> +#include "bpf_common_helpers.h"
>  /*
>   * Note that bpf programs need to include either
>   * vmlinux.h (auto-generated from BTF) or linux/types.h
> @@ -61,20 +62,6 @@
>  #define KERNEL_VERSION(a, b, c) (((a) << 16) + ((b) << 8) + ((c) > 255 ? 255 : (c)))
>  #endif
>
> -/*
> - * Helper macros to manipulate data structures
> - */
> -#ifndef offsetof
> -#define offsetof(TYPE, MEMBER) ((unsigned long)&((TYPE *)0)->MEMBER)
> -#endif
> -#ifndef container_of
> -#define container_of(ptr, type, member)                                \
> -       ({                                                      \
> -               void *__mptr = (void *)(ptr);                   \
> -               ((type *)(__mptr - offsetof(type, member)));    \
> -       })
> -#endif
> -
>  /*
>   * Helper macro to throw a compilation error if __bpf_unreachable() gets
>   * built into the resulting code. This works given BPF back end does not
> diff --git a/tools/testing/selftests/bpf/bpf_util.h b/tools/testing/selftests/bpf/bpf_util.h
> index a3352a64c067..bc0b741b1eef 100644
> --- a/tools/testing/selftests/bpf/bpf_util.h
> +++ b/tools/testing/selftests/bpf/bpf_util.h
> @@ -6,6 +6,7 @@
>  #include <stdlib.h>
>  #include <string.h>
>  #include <errno.h>
> +#include <bpf/bpf_common_helpers.h>
>  #include <bpf/libbpf.h> /* libbpf_num_possible_cpus */
>
>  static inline unsigned int bpf_num_possible_cpus(void)
> @@ -31,13 +32,4 @@ static inline unsigned int bpf_num_possible_cpus(void)
>  # define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
>  #endif
>
> -#ifndef sizeof_field
> -#define sizeof_field(TYPE, MEMBER) sizeof((((TYPE *)0)->MEMBER))
> -#endif
> -
> -#ifndef offsetofend
> -#define offsetofend(TYPE, MEMBER) \
> -       (offsetof(TYPE, MEMBER) + sizeof_field(TYPE, MEMBER))
> -#endif
> -
>  #endif /* __BPF_UTIL__ */
> --
> 2.34.1
>

Re: [PATCH bpf-next v4 14/14] arm64: add a comment that warns that orig_x0 should not be moved

[Andrii Nakryiko]

On Tue, Feb 8, 2022 at 1:46 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> On Tue, 2022-02-08 at 13:11 -0800, Alexei Starovoitov wrote:
> > On Tue, Feb 8, 2022 at 11:46 AM Ilya Leoshkevich <iii@linux.ibm.com>
> > wrote:
> > >
> > > On Tue, 2022-02-08 at 11:25 -0800, Alexei Starovoitov wrote:
> > > > On Tue, Feb 08, 2022 at 06:16:35AM +0100, Ilya Leoshkevich wrote:
> > > > > orig_x0's location is used by libbpf tracing macros, therefore
> > > > > it
> > > > > should not be moved.
> > > > >
> > > > > Suggested-by: Andrii Nakryiko <andrii.nakryiko@gmail.com>
> > > > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > > > > ---
> > > > >  arch/arm64/include/asm/ptrace.h | 4 ++++
> > > > >  1 file changed, 4 insertions(+)
> > > > >
> > > > > diff --git a/arch/arm64/include/asm/ptrace.h
> > > > > b/arch/arm64/include/asm/ptrace.h
> > > > > index 41b332c054ab..7e34c3737839 100644
> > > > > --- a/arch/arm64/include/asm/ptrace.h
> > > > > +++ b/arch/arm64/include/asm/ptrace.h
> > > > > @@ -185,6 +185,10 @@ struct pt_regs {
> > > > >                         u64 pstate;
> > > > >                 };
> > > > >         };
> > > > > +       /*
> > > > > +        * orig_x0 is not exposed via struct user_pt_regs, but
> > > > > its
> > > > > location is
> > > > > +        * assumed by libbpf's tracing macros, so it should not
> > > > > be
> > > > > moved.
> > > > > +        */
> > > >
> > > > In other words this comment is saying that the layout is ABI.
> > > > That's not the case. orig_x0 here and equivalent on s390 can be
> > > > moved.
> > > > It will break bpf progs written without CO-RE and that is
> > > > expected.
> > > > Non CO-RE programs often do all kinds of bpf_probe_read_kernel
> > > > and
> > > > will be breaking when kernel layout is changing.
> > > > I suggest to drop this patch and patch 12.
> > >
> > > Yeah, that was the intention here: to promote orig_x0 to ABI using
> > > a
> > > comment, since doing this by extending user_pt_regs turned out to
> > > be
> > > infeasible. I'm actually ok with not doing this, since programs
> > > compiled with kernel headers and using CO-RE macros will be fine.
> >
> > The comment like this doesn't convert kernel internal struct into
> > ABI.
> > The comment is just wrong. BPF progs access many kernel data structs.
> > s390's and arm64's struct pr_regs is not special in that sense.
> > It's an internal struct.
> >
> > > As you say, we don't care about programs that don't use CO-RE too
> > > much
> > > here - if they break after an incompatible kernel change, fine.
> >
> > Before CO-RE was introduced bpf progs included kernel headers
> > and were breaking when kernel changes. Nothing new here.
> > See the history of bcc tools. Some of them are littered
> > with ifdef VERSION ==.
> >
> > > The question now is - how much do we care about programs that are
> >
> > > compiled with userspace headers? Andrii suggested to use
> > > offsetofend to
> > > make syscall macros work there, however, this now requires this ABI
> > > promotion.
> >
> > Today s390 and arm64 have user_pt_regs as a first field in pt_regs.
> > That is kernel internal behavior and that part can change if arch
> > maintainers have a need for that.
> > bpf progs without CO-RE would have to be adjusted when kernel
> > changes.
> > Even with CO-RE it's ok to rename pt_regs->orig_gpr2 on s390.
> > The progs with CO-RE will break too. The authors of tracing bpf progs
> > have to expect that sooner or later their progs will break and they
> > would have to adjust them.
>
> When it comes to authors of tracing bpf progs, I agree that eventually
> they will have to adjust their code, CO-RE or not. However, in patch 13
> I introduce the following libbpf macro:
>
> #if defined(__KERNEL__) || defined(__VMLINUX_H__)
> ...
> #else
> #define PT_REGS_PARM1_SYSCALL(x) \
>         (*(unsigned long *)(((char *)(x) + \
>                              offsetofend(struct user_pt_regs,
> pstate))))
>
> If we merge this series without freezing orig_x0's offset, in case of
> an incompatible kernel change the users of PT_REGS_PARMn_SYSCALL and
> BPF_KPROBE_SYSCALL, who build against userspace headers, will not
> simply have to update their code - they will have to upgrade libbpf.
> It's also not clear how PT_REGS_PARM1_SYSCALL in the upgraded libbpf
> will even look like, given that it would need to work on both old and
> new kernels.
>
> I've also briefly looked into MIPS' ptrace.h, and it looks as if their
> user_pt_regs has no relationship to kernel pt_regs. IIUC this means
> that it's not possible to correctly implement PT_REGS_PARMn_SYSCALL
> using MIPS userspace headers.
>
> So I wonder whether we should allow using PT_REGS_PARMn_SYSCALL and
> BPF_KPROBE_SYSCALL with userspace headers at all? Would it make sense
> to simply fail the compilation if PT_REGS_PARMn_SYSCALL is used without
> including kernel headers?

Ok, my bad for suggesting those comments, I didn't realize the
consequences of making anything into a stable ABI. Let's not add any
comments, we don't need that.

I think we should just come to terms that for some architectures this
syscall argument access won't work at least for some architectures
without CO-RE. For uniformity let's still have those
PT_REGS_PARM1_SYSCALL macro defined, but we should use
__bpf_unreachable or something like that to make sure it won't compile
if someone tries to use it.

But it's an entirely different story for CORE variants and there (as I
explained on one of the previous patches) we can fabricate our own
definitions of pt_regs (architecture specific, using CO-RE struct
flavors) without any unnecessary assumptions about which include
headers the user is going to use. Hengqi's BPF_KPROBE_SYSCALL() macro
is always going to use CORE variants and will "just work"(tm).

And because this asymmetry of CORE and non-CORE PT_REGS_PARM_SYSCALL
definitions, your changes in v4 are a regression from the ones in v3
which were absolutely fine (I still don't get why you changed all of
that, I've previously landed v3, which means it was 100% acceptable as
is...), because v4 establishes more rigid relation between CORE and
non-CORE variants.

Anyways, let's get back to v3, drop UAPI changes, add struct
pt_regs__s390x and whatever fields we need, use those with
BPF_CORE_READ() and it should be ok with no ABI changes whatsoever.

Re: [PATCH bpf-next v4 05/14] libbpf: Generalize overriding syscall parameter access macros

[Ilya Leoshkevich]

On Tue, 2022-02-08 at 14:05 -0800, Andrii Nakryiko wrote:
> On Mon, Feb 7, 2022 at 9:16 PM Ilya Leoshkevich <iii@linux.ibm.com>
> wrote:
> > 
> > Instead of conditionally overriding PT_REGS_PARM4_SYSCALL, provide
> > default fallbacks for all __PT_PARMn_REG_SYSCALL macros, so that
> > architectures can simply override a specific syscall parameter
> > macro.
> > Also allow completely overriding PT_REGS_PARM1_SYSCALL for
> > non-trivial access sequences.
> > 
> > Co-developed-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > ---
> >  tools/lib/bpf/bpf_tracing.h | 48 +++++++++++++++++++++++++--------
> > ----
> >  1 file changed, 33 insertions(+), 15 deletions(-)
> > 
> > diff --git a/tools/lib/bpf/bpf_tracing.h
> > b/tools/lib/bpf/bpf_tracing.h
> > index da7e8d5c939c..82f1e935d549 100644
> > --- a/tools/lib/bpf/bpf_tracing.h
> > +++ b/tools/lib/bpf/bpf_tracing.h
> > @@ -265,25 +265,43 @@ struct pt_regs;
> > 
> >  #endif
> > 
> > -#define PT_REGS_PARM1_SYSCALL(x) PT_REGS_PARM1(x)
> > -#define PT_REGS_PARM2_SYSCALL(x) PT_REGS_PARM2(x)
> > -#define PT_REGS_PARM3_SYSCALL(x) PT_REGS_PARM3(x)
> > -#ifdef __PT_PARM4_REG_SYSCALL
> > +#ifndef __PT_PARM1_REG_SYSCALL
> > +#define __PT_PARM1_REG_SYSCALL __PT_PARM1_REG
> > +#endif
> > +#ifndef __PT_PARM2_REG_SYSCALL
> > +#define __PT_PARM2_REG_SYSCALL __PT_PARM2_REG
> > +#endif
> > +#ifndef __PT_PARM3_REG_SYSCALL
> > +#define __PT_PARM3_REG_SYSCALL __PT_PARM3_REG
> > +#endif
> > +#ifndef __PT_PARM4_REG_SYSCALL
> > +#define __PT_PARM4_REG_SYSCALL __PT_PARM4_REG
> > +#endif
> > +#ifndef __PT_PARM5_REG_SYSCALL
> > +#define __PT_PARM5_REG_SYSCALL __PT_PARM5_REG
> > +#endif
> > +
> > +#ifndef PT_REGS_PARM1_SYSCALL
> > +#define PT_REGS_PARM1_SYSCALL(x) (__PT_REGS_CAST(x)-
> > >__PT_PARM1_REG_SYSCALL)
> > +#endif
> > +#ifndef PT_REGS_PARM2_SYSCALL
> > +#define PT_REGS_PARM2_SYSCALL(x) (__PT_REGS_CAST(x)-
> > >__PT_PARM2_REG_SYSCALL)
> > +#endif
> > +#ifndef PT_REGS_PARM3_SYSCALL
> > +#define PT_REGS_PARM3_SYSCALL(x) (__PT_REGS_CAST(x)-
> > >__PT_PARM3_REG_SYSCALL)
> > +#endif
> > +#ifndef PT_REGS_PARM4_SYSCALL
> >  #define PT_REGS_PARM4_SYSCALL(x) (__PT_REGS_CAST(x)-
> > >__PT_PARM4_REG_SYSCALL)
> > -#else /* __PT_PARM4_REG_SYSCALL */
> > -#define PT_REGS_PARM4_SYSCALL(x) PT_REGS_PARM4(x)
> >  #endif
> > -#define PT_REGS_PARM5_SYSCALL(x) PT_REGS_PARM5(x)
> > +#ifndef PT_REGS_PARM5_SYSCALL
> > +#define PT_REGS_PARM5_SYSCALL(x) (__PT_REGS_CAST(x)-
> > >__PT_PARM5_REG_SYSCALL)
> > +#endif
> > 
> > -#define PT_REGS_PARM1_CORE_SYSCALL(x) PT_REGS_PARM1_CORE(x)
> > -#define PT_REGS_PARM2_CORE_SYSCALL(x) PT_REGS_PARM2_CORE(x)
> > -#define PT_REGS_PARM3_CORE_SYSCALL(x) PT_REGS_PARM3_CORE(x)
> > -#ifdef __PT_PARM4_REG_SYSCALL
> > +#define PT_REGS_PARM1_CORE_SYSCALL(x)
> > BPF_CORE_READ(__PT_REGS_CAST(x), __PT_PARM1_REG_SYSCALL)
> > +#define PT_REGS_PARM2_CORE_SYSCALL(x)
> > BPF_CORE_READ(__PT_REGS_CAST(x), __PT_PARM2_REG_SYSCALL)
> > +#define PT_REGS_PARM3_CORE_SYSCALL(x)
> > BPF_CORE_READ(__PT_REGS_CAST(x), __PT_PARM3_REG_SYSCALL)
> >  #define PT_REGS_PARM4_CORE_SYSCALL(x)
> > BPF_CORE_READ(__PT_REGS_CAST(x), __PT_PARM4_REG_SYSCALL)
> > -#else /* __PT_PARM4_REG_SYSCALL */
> > -#define PT_REGS_PARM4_CORE_SYSCALL(x) PT_REGS_PARM4_CORE(x)
> > -#endif
> > -#define PT_REGS_PARM5_CORE_SYSCALL(x) PT_REGS_PARM5_CORE(x)
> > +#define PT_REGS_PARM5_CORE_SYSCALL(x)
> > BPF_CORE_READ(__PT_REGS_CAST(x), __PT_PARM5_REG_SYSCALL)
> > 
> 
> No, please don't do it. It makes CORE variants too rigid. We agreed
> w/
> Naveen that the way you did it in v2 is better and more flexible and
> in v3 you did it the other way. Why?

As far as I remember we didn't discuss this proposal from Naveen [1] -
there was another one about moving SYS_PREFIX to libbpf, where
we agreed that it would have bad consequences.

Isn't this patch essentially equivalent to the one from my v3 [2],
but with the added ability to override more things and better-looking? 
I.e.: if we define __PT_PARMn_REG_SYSCALL, then PT_REGS_PARMn_SYSCALL
and PT_REGS_PARMn_CORE_SYSCALL use that, and __PT_PARMn_REG otherwise.

[1]
https://lore.kernel.org/bpf/1643990954.fs9q9mrdxt.naveen@linux.ibm.com/
[2]
https://lore.kernel.org/bpf/20220204145018.1983773-5-iii@linux.ibm.com/

> 
> >  #else /* defined(bpf_target_defined) */
> > 
> > --
> > 2.34.1
> > 

Re: [PATCH bpf-next v4 03/14] selftests/bpf: Compile bpf_syscall_macro test also with user headers

[Ilya Leoshkevich]

On Tue, 2022-02-08 at 14:06 -0800, Andrii Nakryiko wrote:
> On Mon, Feb 7, 2022 at 9:16 PM Ilya Leoshkevich <iii@linux.ibm.com>
> wrote:
> > 
> > Verify that using linux/ptrace.h instead of vmlinux.h works fine.
> > Since without vmlinux.h and with CO-RE it's not possible to access
> > the
> > first syscall argument on arm64 and s390x, and any syscall
> > arguments on
> > Intel, skip the corresponding checks.
> > 
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > ---
> >  ...call_macro.c => test_bpf_syscall_macro_common.h} |  8 +++++++-
> >  .../bpf/prog_tests/test_bpf_syscall_macro_kernel.c  | 13
> > +++++++++++++
> >  .../bpf/prog_tests/test_bpf_syscall_macro_user.c    | 13
> > +++++++++++++
> >  ...f_syscall_macro.c => bpf_syscall_macro_common.h} |  8 ++++++--
> >  .../selftests/bpf/progs/bpf_syscall_macro_kernel.c  |  4 ++++
> >  .../selftests/bpf/progs/bpf_syscall_macro_user.c    | 10
> > ++++++++++
> >  6 files changed, 53 insertions(+), 3 deletions(-)
> >  rename
> > tools/testing/selftests/bpf/prog_tests/{test_bpf_syscall_macro.c =>
> > test_bpf_syscall_macro_common.h} (89%)
> >  create mode 100644
> > tools/testing/selftests/bpf/prog_tests/test_bpf_syscall_macro_kerne
> > l.c
> >  create mode 100644
> > tools/testing/selftests/bpf/prog_tests/test_bpf_syscall_macro_user.
> > c
> >  rename tools/testing/selftests/bpf/progs/{bpf_syscall_macro.c =>
> > bpf_syscall_macro_common.h} (87%)
> >  create mode 100644
> > tools/testing/selftests/bpf/progs/bpf_syscall_macro_kernel.c
> >  create mode 100644
> > tools/testing/selftests/bpf/progs/bpf_syscall_macro_user.c
> > 
> > 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_com
> > mon.h
> > similarity index 89%
> > rename from
> > tools/testing/selftests/bpf/prog_tests/test_bpf_syscall_macro.c
> > rename to
> > tools/testing/selftests/bpf/prog_tests/test_bpf_syscall_macro_commo
> > n.h
> > index f5f4c8adf539..9f2a395abff7 100644
> > ---
> > a/tools/testing/selftests/bpf/prog_tests/test_bpf_syscall_macro.c
> > +++
> > b/tools/testing/selftests/bpf/prog_tests/test_bpf_syscall_macro_com
> > mon.h
> > @@ -2,7 +2,6 @@
> >  /* Copyright 2022 Sony Group Corporation */
> >  #include <sys/prctl.h>
> >  #include <test_progs.h>
> > -#include "bpf_syscall_macro.skel.h"
> > 
> >  void test_bpf_syscall_macro(void)
> >  {
> > @@ -46,7 +45,13 @@ void test_bpf_syscall_macro(void)
> >         ASSERT_EQ(skel->bss->arg5, exp_arg5, "syscall_arg5");
> > 
> >         /* check whether args of syscall are copied correctly for
> > CORE variants */
> > +#if defined(__BPF_SYSCALL_MACRO_KERNEL_SKEL_H__) || \
> > +               (!defined(__s390__) && !defined(__aarch64__) && \
> > +                !defined(__i386__) && !defined(__x86_64__))
> 
> All this is horrible, please no. I think we have better ways to do it
> with CO-RE.

Agreed, with introduction of pt_regs___<ARCH> this part of the test
should always work.

> 
> >         ASSERT_EQ(skel->bss->arg1_core, exp_arg1,
> > "syscall_arg1_core_variant");
> > +#endif
> > +#if defined(__BPF_SYSCALL_MACRO_KERNEL_SKEL_H__) || \
> > +               (!defined(__i386__) && !defined(__x86_64__))
> >         ASSERT_EQ(skel->bss->arg2_core, exp_arg2,
> > "syscall_arg2_core_variant");
> >         ASSERT_EQ(skel->bss->arg3_core, exp_arg3,
> > "syscall_arg3_core_variant");
> >         /* it cannot copy arg4 when uses PT_REGS_PARM4_CORE on
> > x86_64 */
> > @@ -57,6 +62,7 @@ void test_bpf_syscall_macro(void)
> >  #endif
> >         ASSERT_EQ(skel->bss->arg4_core, exp_arg4,
> > "syscall_arg4_core_variant");
> >         ASSERT_EQ(skel->bss->arg5_core, exp_arg5,
> > "syscall_arg5_core_variant");
> > +#endif
> > 
> >  cleanup:
> >         bpf_syscall_macro__destroy(skel);
> > diff --git
> > a/tools/testing/selftests/bpf/prog_tests/test_bpf_syscall_macro_ker
> > nel.c
> > b/tools/testing/selftests/bpf/prog_tests/test_bpf_syscall_macro_ker
> > nel.c
> > new file mode 100644
> > index 000000000000..7ceabd62bb0f
> > --- /dev/null
> > +++
> > b/tools/testing/selftests/bpf/prog_tests/test_bpf_syscall_macro_ker
> > nel.c
> > @@ -0,0 +1,13 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include "bpf_syscall_macro_kernel.skel.h"
> > +
> > +void test_bpf_syscall_macro_kernel(void);
> > +
> > +#define test_bpf_syscall_macro test_bpf_syscall_macro_kernel
> > +#define bpf_syscall_macro bpf_syscall_macro_kernel
> > +#define bpf_syscall_macro__open bpf_syscall_macro_kernel__open
> > +#define bpf_syscall_macro__load bpf_syscall_macro_kernel__load
> > +#define bpf_syscall_macro__attach bpf_syscall_macro_kernel__attach
> > +#define bpf_syscall_macro__destroy
> > bpf_syscall_macro_kernel__destroy
> > +
> > +#include "test_bpf_syscall_macro_common.h"
> > diff --git
> > a/tools/testing/selftests/bpf/prog_tests/test_bpf_syscall_macro_use
> > r.c
> > b/tools/testing/selftests/bpf/prog_tests/test_bpf_syscall_macro_use
> > r.c
> > new file mode 100644
> > index 000000000000..f31558f14e7e
> > --- /dev/null
> > +++
> > b/tools/testing/selftests/bpf/prog_tests/test_bpf_syscall_macro_use
> > r.c
> > @@ -0,0 +1,13 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include "bpf_syscall_macro_user.skel.h"
> > +
> > +void test_bpf_syscall_macro_user(void);
> > +
> > +#define test_bpf_syscall_macro test_bpf_syscall_macro_user
> > +#define bpf_syscall_macro bpf_syscall_macro_user
> > +#define bpf_syscall_macro__open bpf_syscall_macro_user__open
> > +#define bpf_syscall_macro__load bpf_syscall_macro_user__load
> > +#define bpf_syscall_macro__attach bpf_syscall_macro_user__attach
> > +#define bpf_syscall_macro__destroy bpf_syscall_macro_user__destroy
> > +
> > +#include "test_bpf_syscall_macro_common.h"
> > diff --git a/tools/testing/selftests/bpf/progs/bpf_syscall_macro.c
> > b/tools/testing/selftests/bpf/progs/bpf_syscall_macro_common.h
> > similarity index 87%
> > rename from tools/testing/selftests/bpf/progs/bpf_syscall_macro.c
> > rename to
> > tools/testing/selftests/bpf/progs/bpf_syscall_macro_common.h
> > index f5c6ef2ff6d1..8717605358d3 100644
> > --- a/tools/testing/selftests/bpf/progs/bpf_syscall_macro.c
> > +++ b/tools/testing/selftests/bpf/progs/bpf_syscall_macro_common.h
> > @@ -1,7 +1,5 @@
> >  // SPDX-License-Identifier: GPL-2.0
> >  /* Copyright 2022 Sony Group Corporation */
> > -#include <vmlinux.h>
> > -
> >  #include <bpf/bpf_core_read.h>
> >  #include <bpf/bpf_helpers.h>
> >  #include <bpf/bpf_tracing.h>
> > @@ -46,12 +44,18 @@ int BPF_KPROBE(handle_sys_prctl)
> >         bpf_probe_read_kernel(&arg5, sizeof(arg5),
> > &PT_REGS_PARM5_SYSCALL(real_regs));
> > 
> >         /* test for the CORE variant of PT_REGS_PARM */
> > +#if defined(__KERNEL__) || defined(__VMLINUX_H__) || \
> > +               (!defined(bpf_target_s390) &&
> > !defined(bpf_target_arm64) && \
> > +                !defined(bpf_target_x86))
> >         arg1_core = PT_REGS_PARM1_CORE_SYSCALL(real_regs);
> > +#endif
> > +#if defined(__KERNEL__) || defined(__VMLINUX_H__) ||
> > !defined(bpf_target_x86)
> >         arg2_core = PT_REGS_PARM2_CORE_SYSCALL(real_regs);
> >         arg3_core = PT_REGS_PARM3_CORE_SYSCALL(real_regs);
> >         arg4_core_cx = PT_REGS_PARM4_CORE(real_regs);
> >         arg4_core = PT_REGS_PARM4_CORE_SYSCALL(real_regs);
> >         arg5_core = PT_REGS_PARM5_CORE_SYSCALL(real_regs);
> > +#endif
> > 
> >         return 0;
> >  }
> > diff --git
> > a/tools/testing/selftests/bpf/progs/bpf_syscall_macro_kernel.c
> > b/tools/testing/selftests/bpf/progs/bpf_syscall_macro_kernel.c
> > new file mode 100644
> > index 000000000000..1affac21266d
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/bpf_syscall_macro_kernel.c
> > @@ -0,0 +1,4 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include <vmlinux.h>
> > +
> > +#include "bpf_syscall_macro_common.h"
> > diff --git
> > a/tools/testing/selftests/bpf/progs/bpf_syscall_macro_user.c
> > b/tools/testing/selftests/bpf/progs/bpf_syscall_macro_user.c
> > new file mode 100644
> > index 000000000000..1c078d528e8c
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/bpf_syscall_macro_user.c
> > @@ -0,0 +1,10 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include <linux/ptrace.h>
> > +#include <linux/types.h>
> > +#include <sys/types.h>
> > +
> > +#include "bpf_syscall_macro_common.h"
> > +
> > +#if defined(__KERNEL__) || defined(__VMLINUX_H__)
> > +#error This test must be compiled with userspace headers
> > +#endif
> > --
> > 2.34.1
> > 

Re: [PATCH bpf-next v4 05/14] libbpf: Generalize overriding syscall parameter access macros

[Andrii Nakryiko]

On Tue, Feb 8, 2022 at 3:09 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> On Tue, 2022-02-08 at 14:05 -0800, Andrii Nakryiko wrote:
> > On Mon, Feb 7, 2022 at 9:16 PM Ilya Leoshkevich <iii@linux.ibm.com>
> > wrote:
> > >
> > > Instead of conditionally overriding PT_REGS_PARM4_SYSCALL, provide
> > > default fallbacks for all __PT_PARMn_REG_SYSCALL macros, so that
> > > architectures can simply override a specific syscall parameter
> > > macro.
> > > Also allow completely overriding PT_REGS_PARM1_SYSCALL for
> > > non-trivial access sequences.
> > >
> > > Co-developed-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> > > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > > ---
> > >  tools/lib/bpf/bpf_tracing.h | 48 +++++++++++++++++++++++++--------
> > > ----
> > >  1 file changed, 33 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/tools/lib/bpf/bpf_tracing.h
> > > b/tools/lib/bpf/bpf_tracing.h
> > > index da7e8d5c939c..82f1e935d549 100644
> > > --- a/tools/lib/bpf/bpf_tracing.h
> > > +++ b/tools/lib/bpf/bpf_tracing.h
> > > @@ -265,25 +265,43 @@ struct pt_regs;
> > >
> > >  #endif
> > >
> > > -#define PT_REGS_PARM1_SYSCALL(x) PT_REGS_PARM1(x)
> > > -#define PT_REGS_PARM2_SYSCALL(x) PT_REGS_PARM2(x)
> > > -#define PT_REGS_PARM3_SYSCALL(x) PT_REGS_PARM3(x)
> > > -#ifdef __PT_PARM4_REG_SYSCALL
> > > +#ifndef __PT_PARM1_REG_SYSCALL
> > > +#define __PT_PARM1_REG_SYSCALL __PT_PARM1_REG
> > > +#endif
> > > +#ifndef __PT_PARM2_REG_SYSCALL
> > > +#define __PT_PARM2_REG_SYSCALL __PT_PARM2_REG
> > > +#endif
> > > +#ifndef __PT_PARM3_REG_SYSCALL
> > > +#define __PT_PARM3_REG_SYSCALL __PT_PARM3_REG
> > > +#endif
> > > +#ifndef __PT_PARM4_REG_SYSCALL
> > > +#define __PT_PARM4_REG_SYSCALL __PT_PARM4_REG
> > > +#endif
> > > +#ifndef __PT_PARM5_REG_SYSCALL
> > > +#define __PT_PARM5_REG_SYSCALL __PT_PARM5_REG
> > > +#endif
> > > +
> > > +#ifndef PT_REGS_PARM1_SYSCALL
> > > +#define PT_REGS_PARM1_SYSCALL(x) (__PT_REGS_CAST(x)-
> > > >__PT_PARM1_REG_SYSCALL)
> > > +#endif
> > > +#ifndef PT_REGS_PARM2_SYSCALL
> > > +#define PT_REGS_PARM2_SYSCALL(x) (__PT_REGS_CAST(x)-
> > > >__PT_PARM2_REG_SYSCALL)
> > > +#endif
> > > +#ifndef PT_REGS_PARM3_SYSCALL
> > > +#define PT_REGS_PARM3_SYSCALL(x) (__PT_REGS_CAST(x)-
> > > >__PT_PARM3_REG_SYSCALL)
> > > +#endif
> > > +#ifndef PT_REGS_PARM4_SYSCALL
> > >  #define PT_REGS_PARM4_SYSCALL(x) (__PT_REGS_CAST(x)-
> > > >__PT_PARM4_REG_SYSCALL)
> > > -#else /* __PT_PARM4_REG_SYSCALL */
> > > -#define PT_REGS_PARM4_SYSCALL(x) PT_REGS_PARM4(x)
> > >  #endif
> > > -#define PT_REGS_PARM5_SYSCALL(x) PT_REGS_PARM5(x)
> > > +#ifndef PT_REGS_PARM5_SYSCALL
> > > +#define PT_REGS_PARM5_SYSCALL(x) (__PT_REGS_CAST(x)-
> > > >__PT_PARM5_REG_SYSCALL)
> > > +#endif
> > >
> > > -#define PT_REGS_PARM1_CORE_SYSCALL(x) PT_REGS_PARM1_CORE(x)
> > > -#define PT_REGS_PARM2_CORE_SYSCALL(x) PT_REGS_PARM2_CORE(x)
> > > -#define PT_REGS_PARM3_CORE_SYSCALL(x) PT_REGS_PARM3_CORE(x)
> > > -#ifdef __PT_PARM4_REG_SYSCALL
> > > +#define PT_REGS_PARM1_CORE_SYSCALL(x)
> > > BPF_CORE_READ(__PT_REGS_CAST(x), __PT_PARM1_REG_SYSCALL)
> > > +#define PT_REGS_PARM2_CORE_SYSCALL(x)
> > > BPF_CORE_READ(__PT_REGS_CAST(x), __PT_PARM2_REG_SYSCALL)
> > > +#define PT_REGS_PARM3_CORE_SYSCALL(x)
> > > BPF_CORE_READ(__PT_REGS_CAST(x), __PT_PARM3_REG_SYSCALL)
> > >  #define PT_REGS_PARM4_CORE_SYSCALL(x)
> > > BPF_CORE_READ(__PT_REGS_CAST(x), __PT_PARM4_REG_SYSCALL)
> > > -#else /* __PT_PARM4_REG_SYSCALL */
> > > -#define PT_REGS_PARM4_CORE_SYSCALL(x) PT_REGS_PARM4_CORE(x)
> > > -#endif
> > > -#define PT_REGS_PARM5_CORE_SYSCALL(x) PT_REGS_PARM5_CORE(x)
> > > +#define PT_REGS_PARM5_CORE_SYSCALL(x)
> > > BPF_CORE_READ(__PT_REGS_CAST(x), __PT_PARM5_REG_SYSCALL)
> > >
> >
> > No, please don't do it. It makes CORE variants too rigid. We agreed
> > w/
> > Naveen that the way you did it in v2 is better and more flexible and
> > in v3 you did it the other way. Why?
>
> As far as I remember we didn't discuss this proposal from Naveen [1] -
> there was another one about moving SYS_PREFIX to libbpf, where
> we agreed that it would have bad consequences.

Alright, I guess I never submitted my opposition to what Naveen
proposed. But I did land the v3 version of that patch, didn't I? Why
change something that's already accepted?

>
> Isn't this patch essentially equivalent to the one from my v3 [2],
> but with the added ability to override more things and better-looking?

No, it's not. We want to override entire PT_REGS_PARM1_CORE_SYSCALL
definition to be something like BPF_CORE_READ((struct pt_regs___s390x
*)x, orig_gpr2), while you are making  PT_REGS_PARM1_CORE_SYSCALL
definition very rigid.


> I.e.: if we define __PT_PARMn_REG_SYSCALL, then PT_REGS_PARMn_SYSCALL
> and PT_REGS_PARMn_CORE_SYSCALL use that, and __PT_PARMn_REG otherwise.
>
> [1]
> https://lore.kernel.org/bpf/1643990954.fs9q9mrdxt.naveen@linux.ibm.com/
> [2]
> https://lore.kernel.org/bpf/20220204145018.1983773-5-iii@linux.ibm.com/
>
> >
> > >  #else /* defined(bpf_target_defined) */
> > >
> > > --
> > > 2.34.1
> > >
>

Re: [PATCH bpf-next v4 06/14] libbpf: Add PT_REGS_SYSCALL_REGS macro

[Ilya Leoshkevich]

On Tue, 2022-02-08 at 14:08 -0800, Andrii Nakryiko wrote:
> On Mon, Feb 7, 2022 at 9:16 PM Ilya Leoshkevich <iii@linux.ibm.com>
> 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@linux.ibm.com>
> > Co-developed-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > ---
> 
> 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@linux.ibm.com/#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().
> > + */
> > +#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
> > 

Re: [PATCH bpf-next v4 05/14] libbpf: Generalize overriding syscall parameter access macros

[Ilya Leoshkevich]

On 2/9/22 00:21, Andrii Nakryiko wrote:
> On Tue, Feb 8, 2022 at 3:09 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>>
>> On Tue, 2022-02-08 at 14:05 -0800, Andrii Nakryiko wrote:
>>> On Mon, Feb 7, 2022 at 9:16 PM Ilya Leoshkevich <iii@linux.ibm.com>
>>> wrote:
>>>>
>>>> Instead of conditionally overriding PT_REGS_PARM4_SYSCALL, provide
>>>> default fallbacks for all __PT_PARMn_REG_SYSCALL macros, so that
>>>> architectures can simply override a specific syscall parameter
>>>> macro.
>>>> Also allow completely overriding PT_REGS_PARM1_SYSCALL for
>>>> non-trivial access sequences.
>>>>
>>>> Co-developed-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
>>>> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
>>>> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
>>>> ---
>>>>   tools/lib/bpf/bpf_tracing.h | 48 +++++++++++++++++++++++++--------
>>>> ----
>>>>   1 file changed, 33 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/tools/lib/bpf/bpf_tracing.h
>>>> b/tools/lib/bpf/bpf_tracing.h
>>>> index da7e8d5c939c..82f1e935d549 100644
>>>> --- a/tools/lib/bpf/bpf_tracing.h
>>>> +++ b/tools/lib/bpf/bpf_tracing.h
>>>> @@ -265,25 +265,43 @@ struct pt_regs;
>>>>
>>>>   #endif
>>>>
>>>> -#define PT_REGS_PARM1_SYSCALL(x) PT_REGS_PARM1(x)
>>>> -#define PT_REGS_PARM2_SYSCALL(x) PT_REGS_PARM2(x)
>>>> -#define PT_REGS_PARM3_SYSCALL(x) PT_REGS_PARM3(x)
>>>> -#ifdef __PT_PARM4_REG_SYSCALL
>>>> +#ifndef __PT_PARM1_REG_SYSCALL
>>>> +#define __PT_PARM1_REG_SYSCALL __PT_PARM1_REG
>>>> +#endif
>>>> +#ifndef __PT_PARM2_REG_SYSCALL
>>>> +#define __PT_PARM2_REG_SYSCALL __PT_PARM2_REG
>>>> +#endif
>>>> +#ifndef __PT_PARM3_REG_SYSCALL
>>>> +#define __PT_PARM3_REG_SYSCALL __PT_PARM3_REG
>>>> +#endif
>>>> +#ifndef __PT_PARM4_REG_SYSCALL
>>>> +#define __PT_PARM4_REG_SYSCALL __PT_PARM4_REG
>>>> +#endif
>>>> +#ifndef __PT_PARM5_REG_SYSCALL
>>>> +#define __PT_PARM5_REG_SYSCALL __PT_PARM5_REG
>>>> +#endif
>>>> +
>>>> +#ifndef PT_REGS_PARM1_SYSCALL
>>>> +#define PT_REGS_PARM1_SYSCALL(x) (__PT_REGS_CAST(x)-
>>>>> __PT_PARM1_REG_SYSCALL)
>>>> +#endif
>>>> +#ifndef PT_REGS_PARM2_SYSCALL
>>>> +#define PT_REGS_PARM2_SYSCALL(x) (__PT_REGS_CAST(x)-
>>>>> __PT_PARM2_REG_SYSCALL)
>>>> +#endif
>>>> +#ifndef PT_REGS_PARM3_SYSCALL
>>>> +#define PT_REGS_PARM3_SYSCALL(x) (__PT_REGS_CAST(x)-
>>>>> __PT_PARM3_REG_SYSCALL)
>>>> +#endif
>>>> +#ifndef PT_REGS_PARM4_SYSCALL
>>>>   #define PT_REGS_PARM4_SYSCALL(x) (__PT_REGS_CAST(x)-
>>>>> __PT_PARM4_REG_SYSCALL)
>>>> -#else /* __PT_PARM4_REG_SYSCALL */
>>>> -#define PT_REGS_PARM4_SYSCALL(x) PT_REGS_PARM4(x)
>>>>   #endif
>>>> -#define PT_REGS_PARM5_SYSCALL(x) PT_REGS_PARM5(x)
>>>> +#ifndef PT_REGS_PARM5_SYSCALL
>>>> +#define PT_REGS_PARM5_SYSCALL(x) (__PT_REGS_CAST(x)-
>>>>> __PT_PARM5_REG_SYSCALL)
>>>> +#endif
>>>>
>>>> -#define PT_REGS_PARM1_CORE_SYSCALL(x) PT_REGS_PARM1_CORE(x)
>>>> -#define PT_REGS_PARM2_CORE_SYSCALL(x) PT_REGS_PARM2_CORE(x)
>>>> -#define PT_REGS_PARM3_CORE_SYSCALL(x) PT_REGS_PARM3_CORE(x)
>>>> -#ifdef __PT_PARM4_REG_SYSCALL
>>>> +#define PT_REGS_PARM1_CORE_SYSCALL(x)
>>>> BPF_CORE_READ(__PT_REGS_CAST(x), __PT_PARM1_REG_SYSCALL)
>>>> +#define PT_REGS_PARM2_CORE_SYSCALL(x)
>>>> BPF_CORE_READ(__PT_REGS_CAST(x), __PT_PARM2_REG_SYSCALL)
>>>> +#define PT_REGS_PARM3_CORE_SYSCALL(x)
>>>> BPF_CORE_READ(__PT_REGS_CAST(x), __PT_PARM3_REG_SYSCALL)
>>>>   #define PT_REGS_PARM4_CORE_SYSCALL(x)
>>>> BPF_CORE_READ(__PT_REGS_CAST(x), __PT_PARM4_REG_SYSCALL)
>>>> -#else /* __PT_PARM4_REG_SYSCALL */
>>>> -#define PT_REGS_PARM4_CORE_SYSCALL(x) PT_REGS_PARM4_CORE(x)
>>>> -#endif
>>>> -#define PT_REGS_PARM5_CORE_SYSCALL(x) PT_REGS_PARM5_CORE(x)
>>>> +#define PT_REGS_PARM5_CORE_SYSCALL(x)
>>>> BPF_CORE_READ(__PT_REGS_CAST(x), __PT_PARM5_REG_SYSCALL)
>>>>
>>>
>>> No, please don't do it. It makes CORE variants too rigid. We agreed
>>> w/
>>> Naveen that the way you did it in v2 is better and more flexible and
>>> in v3 you did it the other way. Why?
>>
>> As far as I remember we didn't discuss this proposal from Naveen [1] -
>> there was another one about moving SYS_PREFIX to libbpf, where
>> we agreed that it would have bad consequences.
> 
> Alright, I guess I never submitted my opposition to what Naveen
> proposed. But I did land the v3 version of that patch, didn't I? Why
> change something that's already accepted?

Right. Sorry, I just wanted to use this opportunity to clean up things a
little.

> 
>>
>> Isn't this patch essentially equivalent to the one from my v3 [2],
>> but with the added ability to override more things and better-looking?
> 
> No, it's not. We want to override entire PT_REGS_PARM1_CORE_SYSCALL
> definition to be something like BPF_CORE_READ((struct pt_regs___s390x
> *)x, orig_gpr2), while you are making  PT_REGS_PARM1_CORE_SYSCALL
> definition very rigid.

Right, now that we've decided to use flavors, this is no longer useful.
I'll drop it for v5.

> 
> 
>> I.e.: if we define __PT_PARMn_REG_SYSCALL, then PT_REGS_PARMn_SYSCALL
>> and PT_REGS_PARMn_CORE_SYSCALL use that, and __PT_PARMn_REG otherwise.
>>
>> [1]
>> https://lore.kernel.org/bpf/1643990954.fs9q9mrdxt.naveen@linux.ibm.com/
>> [2]
>> https://lore.kernel.org/bpf/20220204145018.1983773-5-iii@linux.ibm.com/
>>
>>>
>>>>   #else /* defined(bpf_target_defined) */
>>>>
>>>> --
>>>> 2.34.1
>>>>
>>

Re: [PATCH bpf-next v4 08/14] libbpf: Use struct pt_regs when compiling with kernel headers

[Ilya Leoshkevich]

On Tue, 2022-02-08 at 14:12 -0800, Andrii Nakryiko wrote:
> On Mon, Feb 7, 2022 at 9:16 PM Ilya Leoshkevich <iii@linux.ibm.com>
> wrote:
> > 
> > Andrii says: "... with CO-RE and vmlinux.h it would be more
> > reliable
> > and straightforward to just stick to kernel-internal struct pt_regs
> > everywhere ...".
> > 
> > Actually, if vmlinux.h is available, then it's ok to do so for both
> > CO-RE and non-CO-RE cases, since the beginning of struct pt_regs
> > must
> > match (struct) user_pt_regs, which must never change.
> > 
> > Implement this by not defining __PT_REGS_CAST if the user included
> > vmlinux.h.
> > 
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > ---
> 
> If we are using CO-RE we don't have to assume vmlinux.h, we can
> define
> our own definition of pt_regs with custom "flavor":
> 
> struct pt_regs___s390x {
>     long gprs[10];
>     long orig_gpr2; /* whatever the right types and names, but order
> doesn't matter */
> } __attribute__((preserve_access_index));
> 
> 
> And then use `struct pt_regs__s390x` for s390x macros. That way we
> don't assume any specific included header, we have minimal definition
> we need (and it can be different for each architecture. It's still
> CO-RE, still relocatable, and we don't need all these ugly #if
> defined() checks.

I haven't considered using flavors - this will indeed improve the CO-RE
side of things, thanks!

> 
> >  tools/lib/bpf/bpf_tracing.h | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/tools/lib/bpf/bpf_tracing.h
> > b/tools/lib/bpf/bpf_tracing.h
> > index 7a015ee8fb11..07e291d77e83 100644
> > --- a/tools/lib/bpf/bpf_tracing.h
> > +++ b/tools/lib/bpf/bpf_tracing.h
> > @@ -118,8 +118,11 @@
> > 
> >  #define __BPF_ARCH_HAS_SYSCALL_WRAPPER
> > 
> > +#if !defined(__KERNEL__) && !defined(__VMLINUX_H__)
> >  /* s390 provides user_pt_regs instead of struct pt_regs to
> > userspace */
> >  #define __PT_REGS_CAST(x) ((const user_pt_regs *)(x))
> > +#endif
> > +
> >  #define __PT_PARM1_REG gprs[2]
> >  #define __PT_PARM2_REG gprs[3]
> >  #define __PT_PARM3_REG gprs[4]
> > @@ -148,8 +151,11 @@
> > 
> >  #define __BPF_ARCH_HAS_SYSCALL_WRAPPER
> > 
> > +#if !defined(__KERNEL__) && !defined(__VMLINUX_H__)
> >  /* arm64 provides struct user_pt_regs instead of struct pt_regs to
> > userspace */
> >  #define __PT_REGS_CAST(x) ((const struct user_pt_regs *)(x))
> > +#endif
> > +
> >  #define __PT_PARM1_REG regs[0]
> >  #define __PT_PARM2_REG regs[1]
> >  #define __PT_PARM3_REG regs[2]
> > @@ -207,7 +213,10 @@
> > 
> >  #elif defined(bpf_target_riscv)
> > 
> > +#if !defined(__KERNEL__) && !defined(__VMLINUX_H__)
> >  #define __PT_REGS_CAST(x) ((const struct user_regs_struct *)(x))
> > +#endif
> > +
> >  #define __PT_PARM1_REG a0
> >  #define __PT_PARM2_REG a1
> >  #define __PT_PARM3_REG a2
> > --
> > 2.34.1
> > 

Re: [PATCH bpf-next v4 10/14] libbpf: Move data structure manipulation macros to bpf_common_helpers.h

[Ilya Leoshkevich]

On Tue, 2022-02-08 at 14:14 -0800, Andrii Nakryiko wrote:
> On Mon, Feb 7, 2022 at 9:16 PM Ilya Leoshkevich <iii@linux.ibm.com>
> wrote:
> > 
> > These macros are useful for both libbpf and bpf progs, so put them
> > into
> > a separate header dedicated to this use case.
> > 
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > ---
> >  tools/lib/bpf/Makefile                 |  2 +-
> >  tools/lib/bpf/bpf_common_helpers.h     | 30
> > ++++++++++++++++++++++++++
> >  tools/lib/bpf/bpf_helpers.h            | 15 +------------
> >  tools/testing/selftests/bpf/bpf_util.h | 10 +--------
> >  4 files changed, 33 insertions(+), 24 deletions(-)
> >  create mode 100644 tools/lib/bpf/bpf_common_helpers.h
> > 
> > diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
> > index b8b37fe76006..60b06c22e0a1 100644
> > --- a/tools/lib/bpf/Makefile
> > +++ b/tools/lib/bpf/Makefile
> > @@ -239,7 +239,7 @@ install_lib: all_cmd
> > 
> >  SRC_HDRS := bpf.h libbpf.h btf.h libbpf_common.h libbpf_legacy.h
> > xsk.h      \
> >             bpf_helpers.h bpf_tracing.h bpf_endian.h
> > bpf_core_read.h         \
> > -           skel_internal.h libbpf_version.h
> > +           skel_internal.h libbpf_version.h bpf_common_helpers.h
> 
> Wait, how did we get from fixing s390x syscall arg fetching to
> exposing a new public API header from libbpf?... I feel like I missed
> a few revisions and discussion threads.

I didn't want to add yet another copy of
offsetof/sizeof_field/offsetofend to the code. However, since we don't
need offsetofend anymore, this patch will be dropped.

> 
> >  GEN_HDRS := $(BPF_GENERATED)
> > 
> >  INSTALL_PFX := $(DESTDIR)$(prefix)/include/bpf
> > diff --git a/tools/lib/bpf/bpf_common_helpers.h
> > b/tools/lib/bpf/bpf_common_helpers.h
> > new file mode 100644
> > index 000000000000..79db303b6ae2
> > --- /dev/null
> > +++ b/tools/lib/bpf/bpf_common_helpers.h
> > @@ -0,0 +1,30 @@
> > +/* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */
> > +#ifndef __BPF_COMMON_HELPERS__
> > +#define __BPF_COMMON_HELPERS__
> > +
> > +/*
> > + * Helper macros that can be used both by libbpf and bpf progs.
> > + */
> > +
> > +#ifndef offsetof
> > +#define offsetof(TYPE, MEMBER) ((unsigned long)&((TYPE *)0)-
> > >MEMBER)
> > +#endif
> > +
> > +#ifndef sizeof_field
> > +#define sizeof_field(TYPE, MEMBER) sizeof((((TYPE *)0)->MEMBER))
> > +#endif
> > +
> > +#ifndef offsetofend
> > +#define offsetofend(TYPE, MEMBER) \
> > +       (offsetof(TYPE, MEMBER) + sizeof_field(TYPE, MEMBER))
> > +#endif
> > +
> > +#ifndef container_of
> > +#define container_of(ptr, type,
> > member)                                \
> > +       ({                                                      \
> > +               void *__mptr = (void *)(ptr);                   \
> > +               ((type *)(__mptr - offsetof(type, member)));    \
> > +       })
> > +#endif
> > +
> > +#endif
> > diff --git a/tools/lib/bpf/bpf_helpers.h
> > b/tools/lib/bpf/bpf_helpers.h
> > index 44df982d2a5c..1e8b609c1000 100644
> > --- a/tools/lib/bpf/bpf_helpers.h
> > +++ b/tools/lib/bpf/bpf_helpers.h
> > @@ -2,6 +2,7 @@
> >  #ifndef __BPF_HELPERS__
> >  #define __BPF_HELPERS__
> > 
> > +#include "bpf_common_helpers.h"
> >  /*
> >   * Note that bpf programs need to include either
> >   * vmlinux.h (auto-generated from BTF) or linux/types.h
> > @@ -61,20 +62,6 @@
> >  #define KERNEL_VERSION(a, b, c) (((a) << 16) + ((b) << 8) + ((c) >
> > 255 ? 255 : (c)))
> >  #endif
> > 
> > -/*
> > - * Helper macros to manipulate data structures
> > - */
> > -#ifndef offsetof
> > -#define offsetof(TYPE, MEMBER) ((unsigned long)&((TYPE *)0)-
> > >MEMBER)
> > -#endif
> > -#ifndef container_of
> > -#define container_of(ptr, type,
> > member)                                \
> > -       ({                                                      \
> > -               void *__mptr = (void *)(ptr);                   \
> > -               ((type *)(__mptr - offsetof(type, member)));    \
> > -       })
> > -#endif
> > -
> >  /*
> >   * Helper macro to throw a compilation error if
> > __bpf_unreachable() gets
> >   * built into the resulting code. This works given BPF back end
> > does not
> > diff --git a/tools/testing/selftests/bpf/bpf_util.h
> > b/tools/testing/selftests/bpf/bpf_util.h
> > index a3352a64c067..bc0b741b1eef 100644
> > --- a/tools/testing/selftests/bpf/bpf_util.h
> > +++ b/tools/testing/selftests/bpf/bpf_util.h
> > @@ -6,6 +6,7 @@
> >  #include <stdlib.h>
> >  #include <string.h>
> >  #include <errno.h>
> > +#include <bpf/bpf_common_helpers.h>
> >  #include <bpf/libbpf.h> /* libbpf_num_possible_cpus */
> > 
> >  static inline unsigned int bpf_num_possible_cpus(void)
> > @@ -31,13 +32,4 @@ static inline unsigned int
> > bpf_num_possible_cpus(void)
> >  # define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> >  #endif
> > 
> > -#ifndef sizeof_field
> > -#define sizeof_field(TYPE, MEMBER) sizeof((((TYPE *)0)->MEMBER))
> > -#endif
> > -
> > -#ifndef offsetofend
> > -#define offsetofend(TYPE, MEMBER) \
> > -       (offsetof(TYPE, MEMBER) + sizeof_field(TYPE, MEMBER))
> > -#endif
> > -
> >  #endif /* __BPF_UTIL__ */
> > --
> > 2.34.1
> > 

Re: [PATCH bpf-next v4 14/14] arm64: add a comment that warns that orig_x0 should not be moved

[Ilya Leoshkevich]

On Tue, 2022-02-08 at 14:23 -0800, Andrii Nakryiko wrote:
> On Tue, Feb 8, 2022 at 1:46 PM Ilya Leoshkevich <iii@linux.ibm.com>
> wrote:
> > 
> > On Tue, 2022-02-08 at 13:11 -0800, Alexei Starovoitov wrote:
> > > On Tue, Feb 8, 2022 at 11:46 AM Ilya Leoshkevich
> > > <iii@linux.ibm.com>
> > > wrote:
> > > > 
> > > > On Tue, 2022-02-08 at 11:25 -0800, Alexei Starovoitov wrote:
> > > > > On Tue, Feb 08, 2022 at 06:16:35AM +0100, Ilya Leoshkevich
> > > > > wrote:
> > > > > > orig_x0's location is used by libbpf tracing macros,
> > > > > > therefore
> > > > > > it
> > > > > > should not be moved.
> > > > > > 
> > > > > > Suggested-by: Andrii Nakryiko <andrii.nakryiko@gmail.com>
> > > > > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > > > > > ---
> > > > > >  arch/arm64/include/asm/ptrace.h | 4 ++++
> > > > > >  1 file changed, 4 insertions(+)
> > > > > > 
> > > > > > diff --git a/arch/arm64/include/asm/ptrace.h
> > > > > > b/arch/arm64/include/asm/ptrace.h
> > > > > > index 41b332c054ab..7e34c3737839 100644
> > > > > > --- a/arch/arm64/include/asm/ptrace.h
> > > > > > +++ b/arch/arm64/include/asm/ptrace.h
> > > > > > @@ -185,6 +185,10 @@ struct pt_regs {
> > > > > >                         u64 pstate;
> > > > > >                 };
> > > > > >         };
> > > > > > +       /*
> > > > > > +        * orig_x0 is not exposed via struct user_pt_regs,
> > > > > > but
> > > > > > its
> > > > > > location is
> > > > > > +        * assumed by libbpf's tracing macros, so it should
> > > > > > not
> > > > > > be
> > > > > > moved.
> > > > > > +        */
> > > > > 
> > > > > In other words this comment is saying that the layout is ABI.
> > > > > That's not the case. orig_x0 here and equivalent on s390 can
> > > > > be
> > > > > moved.
> > > > > It will break bpf progs written without CO-RE and that is
> > > > > expected.
> > > > > Non CO-RE programs often do all kinds of
> > > > > bpf_probe_read_kernel
> > > > > and
> > > > > will be breaking when kernel layout is changing.
> > > > > I suggest to drop this patch and patch 12.
> > > > 
> > > > Yeah, that was the intention here: to promote orig_x0 to ABI
> > > > using
> > > > a
> > > > comment, since doing this by extending user_pt_regs turned out
> > > > to
> > > > be
> > > > infeasible. I'm actually ok with not doing this, since programs
> > > > compiled with kernel headers and using CO-RE macros will be
> > > > fine.
> > > 
> > > The comment like this doesn't convert kernel internal struct into
> > > ABI.
> > > The comment is just wrong. BPF progs access many kernel data
> > > structs.
> > > s390's and arm64's struct pr_regs is not special in that sense.
> > > It's an internal struct.
> > > 
> > > > As you say, we don't care about programs that don't use CO-RE
> > > > too
> > > > much
> > > > here - if they break after an incompatible kernel change, fine.
> > > 
> > > Before CO-RE was introduced bpf progs included kernel headers
> > > and were breaking when kernel changes. Nothing new here.
> > > See the history of bcc tools. Some of them are littered
> > > with ifdef VERSION ==.
> > > 
> > > > The question now is - how much do we care about programs that
> > > > are
> > > 
> > > > compiled with userspace headers? Andrii suggested to use
> > > > offsetofend to
> > > > make syscall macros work there, however, this now requires this
> > > > ABI
> > > > promotion.
> > > 
> > > Today s390 and arm64 have user_pt_regs as a first field in
> > > pt_regs.
> > > That is kernel internal behavior and that part can change if arch
> > > maintainers have a need for that.
> > > bpf progs without CO-RE would have to be adjusted when kernel
> > > changes.
> > > Even with CO-RE it's ok to rename pt_regs->orig_gpr2 on s390.
> > > The progs with CO-RE will break too. The authors of tracing bpf
> > > progs
> > > have to expect that sooner or later their progs will break and
> > > they
> > > would have to adjust them.
> > 
> > When it comes to authors of tracing bpf progs, I agree that
> > eventually
> > they will have to adjust their code, CO-RE or not. However, in
> > patch 13
> > I introduce the following libbpf macro:
> > 
> > #if defined(__KERNEL__) || defined(__VMLINUX_H__)
> > ...
> > #else
> > #define PT_REGS_PARM1_SYSCALL(x) \
> >         (*(unsigned long *)(((char *)(x) + \
> >                              offsetofend(struct user_pt_regs,
> > pstate))))
> > 
> > If we merge this series without freezing orig_x0's offset, in case
> > of
> > an incompatible kernel change the users of PT_REGS_PARMn_SYSCALL
> > and
> > BPF_KPROBE_SYSCALL, who build against userspace headers, will not
> > simply have to update their code - they will have to upgrade
> > libbpf.
> > It's also not clear how PT_REGS_PARM1_SYSCALL in the upgraded
> > libbpf
> > will even look like, given that it would need to work on both old
> > and
> > new kernels.
> > 
> > I've also briefly looked into MIPS' ptrace.h, and it looks as if
> > their
> > user_pt_regs has no relationship to kernel pt_regs. IIUC this means
> > that it's not possible to correctly implement PT_REGS_PARMn_SYSCALL
> > using MIPS userspace headers.
> > 
> > So I wonder whether we should allow using PT_REGS_PARMn_SYSCALL and
> > BPF_KPROBE_SYSCALL with userspace headers at all? Would it make
> > sense
> > to simply fail the compilation if PT_REGS_PARMn_SYSCALL is used
> > without
> > including kernel headers?
> 
> Ok, my bad for suggesting those comments, I didn't realize the
> consequences of making anything into a stable ABI. Let's not add any
> comments, we don't need that.
> 
> I think we should just come to terms that for some architectures this
> syscall argument access won't work at least for some architectures
> without CO-RE. For uniformity let's still have those
> PT_REGS_PARM1_SYSCALL macro defined, but we should use
> __bpf_unreachable or something like that to make sure it won't
> compile
> if someone tries to use it.

Awesome, this would make quite a few headaches go away.

> 
> But it's an entirely different story for CORE variants and there (as
> I
> explained on one of the previous patches) we can fabricate our own
> definitions of pt_regs (architecture specific, using CO-RE struct
> flavors) without any unnecessary assumptions about which include
> headers the user is going to use. Hengqi's BPF_KPROBE_SYSCALL() macro
> is always going to use CORE variants and will "just work"(tm).
> 
> And because this asymmetry of CORE and non-CORE PT_REGS_PARM_SYSCALL
> definitions, your changes in v4 are a regression from the ones in v3
> which were absolutely fine (I still don't get why you changed all of
> that, I've previously landed v3, which means it was 100% acceptable
> as
> is...), because v4 establishes more rigid relation between CORE and
> non-CORE variants.
> 
> Anyways, let's get back to v3, drop UAPI changes, add struct
> pt_regs__s390x and whatever fields we need, use those with
> BPF_CORE_READ() and it should be ok with no ABI changes whatsoever.

Ok.

Re: [PATCH bpf-next v4 06/14] libbpf: Add PT_REGS_SYSCALL_REGS macro

[Andrii Nakryiko]

On Tue, Feb 8, 2022 at 3:26 PM Ilya Leoshkevich <iii@linux.ibm.com> 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@linux.ibm.com>
> > 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@linux.ibm.com>
> > > Co-developed-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> > > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > > ---
> >
> > 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@linux.ibm.com/#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
> > >
>