* [PATCH bpf-next 0/2] libbpf: Add syscall-specific variants of BPF_KPROBE/BPF_KRETPROBE
@ 2021-12-21 5:53 Hengqi Chen
2021-12-21 5:53 ` [PATCH bpf-next 1/2] libbpf: Add BPF_KPROBE_SYSCALL/BPF_KRETPROBE_SYSCALL macros Hengqi Chen
2021-12-21 5:53 ` [PATCH bpf-next 2/2] selftests/bpf: Test " Hengqi Chen
0 siblings, 2 replies; 10+ messages in thread
From: Hengqi Chen @ 2021-12-21 5:53 UTC (permalink / raw)
To: bpf, andrii, hengqi.chen
Add new macros BPF_KPROBE_SYSCALL/BPF_KRETPROBE_SYSCALL, which provides
easy access to syscall input arguments and return values. See [0] and [1]
for background.
[0]: https://github.com/libbpf/libbpf-bootstrap/issues/57
[1]: https://github.com/libbpf/libbpf/issues/425
Hengqi Chen (2):
libbpf: Add BPF_KPROBE_SYSCALL/BPF_KRETPROBE_SYSCALL macros
selftests/bpf: Test BPF_KPROBE_SYSCALL/BPF_KRETPROBE_SYSCALL macros
tools/lib/bpf/bpf_tracing.h | 45 +++++++++++++++++++
.../selftests/bpf/prog_tests/kprobe_syscall.c | 40 +++++++++++++++++
.../selftests/bpf/progs/test_kprobe_syscall.c | 41 +++++++++++++++++
3 files changed, 126 insertions(+)
create mode 100644 tools/testing/selftests/bpf/prog_tests/kprobe_syscall.c
create mode 100644 tools/testing/selftests/bpf/progs/test_kprobe_syscall.c
--
2.30.2
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH bpf-next 1/2] libbpf: Add BPF_KPROBE_SYSCALL/BPF_KRETPROBE_SYSCALL macros
2021-12-21 5:53 [PATCH bpf-next 0/2] libbpf: Add syscall-specific variants of BPF_KPROBE/BPF_KRETPROBE Hengqi Chen
@ 2021-12-21 5:53 ` Hengqi Chen
2021-12-21 15:53 ` Yonghong Song
2021-12-22 0:18 ` Andrii Nakryiko
2021-12-21 5:53 ` [PATCH bpf-next 2/2] selftests/bpf: Test " Hengqi Chen
1 sibling, 2 replies; 10+ messages in thread
From: Hengqi Chen @ 2021-12-21 5:53 UTC (permalink / raw)
To: bpf, andrii, hengqi.chen
Add syscall-specific variants of BPF_KPROBE/BPF_KRETPROBE named
BPF_KPROBE_SYSCALL/BPF_KRETPROBE_SYSCALL ([0]). These new macros
hide the underlying way of getting syscall input arguments and
return values. With these new macros, the following code:
SEC("kprobe/__x64_sys_close")
int BPF_KPROBE(do_sys_close, struct pt_regs *regs)
{
int fd;
fd = PT_REGS_PARM1_CORE(regs);
/* do something with fd */
}
can be written as:
SEC("kprobe/__x64_sys_close")
int BPF_KPROBE_SYSCALL(do_sys_close, int fd)
{
/* do something with fd */
}
[0] Closes: https://github.com/libbpf/libbpf/issues/425
Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
---
tools/lib/bpf/bpf_tracing.h | 45 +++++++++++++++++++++++++++++++++++++
1 file changed, 45 insertions(+)
diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
index db05a5937105..eb4b567e443f 100644
--- a/tools/lib/bpf/bpf_tracing.h
+++ b/tools/lib/bpf/bpf_tracing.h
@@ -489,4 +489,49 @@ typeof(name(0)) name(struct pt_regs *ctx) \
} \
static __always_inline typeof(name(0)) ____##name(struct pt_regs *ctx, ##args)
+#define ___bpf_syscall_args0() ctx, regs
+#define ___bpf_syscall_args1(x) \
+ ___bpf_syscall_args0(), (void *)PT_REGS_PARM1_CORE(regs)
+#define ___bpf_syscall_args2(x, args...) \
+ ___bpf_syscall_args1(args), (void *)PT_REGS_PARM2_CORE(regs)
+#define ___bpf_syscall_args3(x, args...) \
+ ___bpf_syscall_args2(args), (void *)PT_REGS_PARM3_CORE(regs)
+#define ___bpf_syscall_args4(x, args...) \
+ ___bpf_syscall_args3(args), (void *)PT_REGS_PARM4_CORE(regs)
+#define ___bpf_syscall_args5(x, args...) \
+ ___bpf_syscall_args4(args), (void *)PT_REGS_PARM5_CORE(regs)
+#define ___bpf_syscall_args(args...) \
+ ___bpf_apply(___bpf_syscall_args, ___bpf_narg(args))(args)
+
+/*
+ * BPF_KPROBE_SYSCALL is a variant of BPF_KPROBE, which is intended for
+ * tracing syscall functions. It hides the underlying platform-specific
+ * low-level way of getting syscall input arguments from struct pt_regs, and
+ * provides a familiar typed and named function arguments syntax and
+ * semantics of accessing syscall input paremeters.
+ *
+ * Original struct pt_regs* context is preserved as 'ctx' argument. This might
+ * be necessary when using BPF helpers like bpf_perf_event_output().
+ */
+#define BPF_KPROBE_SYSCALL(name, args...) \
+name(struct pt_regs *ctx); \
+static __attribute__((always_inline)) typeof(name(0)) \
+____##name(struct pt_regs *ctx, struct pt_regs *regs, ##args); \
+typeof(name(0)) name(struct pt_regs *ctx) \
+{ \
+ _Pragma("GCC diagnostic push") \
+ _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
+ struct pt_regs *regs = PT_REGS_PARM1(ctx); \
+ return ____##name(___bpf_syscall_args(args)); \
+ _Pragma("GCC diagnostic pop") \
+} \
+static __attribute__((always_inline)) typeof(name(0)) \
+____##name(struct pt_regs *ctx, struct pt_regs *regs, ##args)
+
+/*
+ * BPF_KRETPROBE_SYSCALL is just an alias to BPF_KRETPROBE,
+ * it provides optional return value (in addition to `struct pt_regs *ctx`)
+ */
+#define BPF_KRETPROBE_SYSCALL BPF_KRETPROBE
+
#endif
--
2.30.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH bpf-next 2/2] selftests/bpf: Test BPF_KPROBE_SYSCALL/BPF_KRETPROBE_SYSCALL macros
2021-12-21 5:53 [PATCH bpf-next 0/2] libbpf: Add syscall-specific variants of BPF_KPROBE/BPF_KRETPROBE Hengqi Chen
2021-12-21 5:53 ` [PATCH bpf-next 1/2] libbpf: Add BPF_KPROBE_SYSCALL/BPF_KRETPROBE_SYSCALL macros Hengqi Chen
@ 2021-12-21 5:53 ` Hengqi Chen
2021-12-22 0:34 ` Andrii Nakryiko
2021-12-22 0:37 ` Andrii Nakryiko
1 sibling, 2 replies; 10+ messages in thread
From: Hengqi Chen @ 2021-12-21 5:53 UTC (permalink / raw)
To: bpf, andrii, hengqi.chen
Add tests for the newly added BPF_KPROBE_SYSCALL/BPF_KRETPROBE_SYSCALL macros.
Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
---
.../selftests/bpf/prog_tests/kprobe_syscall.c | 40 ++++++++++++++++++
.../selftests/bpf/progs/test_kprobe_syscall.c | 41 +++++++++++++++++++
2 files changed, 81 insertions(+)
create mode 100644 tools/testing/selftests/bpf/prog_tests/kprobe_syscall.c
create mode 100644 tools/testing/selftests/bpf/progs/test_kprobe_syscall.c
diff --git a/tools/testing/selftests/bpf/prog_tests/kprobe_syscall.c b/tools/testing/selftests/bpf/prog_tests/kprobe_syscall.c
new file mode 100644
index 000000000000..a1fad70bbb69
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/kprobe_syscall.c
@@ -0,0 +1,40 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2021 Hengqi Chen */
+
+#include <test_progs.h>
+#include <sys/types.h>
+#include <sys/socket.h>
+#include "test_kprobe_syscall.skel.h"
+
+void test_kprobe_syscall(void)
+{
+ struct test_kprobe_syscall *skel;
+ int err, fd = 0;
+
+ skel = test_kprobe_syscall__open();
+ if (!ASSERT_OK_PTR(skel, "could not open BPF object"))
+ return;
+
+ skel->rodata->my_pid = getpid();
+
+ err = test_kprobe_syscall__load(skel);
+ if (!ASSERT_OK(err, "could not load BPF object"))
+ goto cleanup;
+
+ err = test_kprobe_syscall__attach(skel);
+ if (!ASSERT_OK(err, "could not attach BPF object"))
+ goto cleanup;
+
+ fd = socket(AF_UNIX, SOCK_STREAM, 0);
+
+ ASSERT_GT(fd, 0, "socket failed");
+ ASSERT_EQ(skel->bss->domain, AF_UNIX, "BPF_KPROBE_SYSCALL failed");
+ ASSERT_EQ(skel->bss->type, SOCK_STREAM, "BPF_KPROBE_SYSCALL failed");
+ ASSERT_EQ(skel->bss->protocol, 0, "BPF_KPROBE_SYSCALL failed");
+ ASSERT_EQ(skel->bss->fd, fd, "BPF_KRETPROBE_SYSCALL failed");
+
+cleanup:
+ if (fd)
+ close(fd);
+ test_kprobe_syscall__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/progs/test_kprobe_syscall.c b/tools/testing/selftests/bpf/progs/test_kprobe_syscall.c
new file mode 100644
index 000000000000..ecef9d19007c
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_kprobe_syscall.c
@@ -0,0 +1,41 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2021 Hengqi Chen */
+
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include <bpf/bpf_core_read.h>
+
+const volatile pid_t my_pid = 0;
+int domain = 0;
+int type = 0;
+int protocol = 0;
+int fd = 0;
+
+SEC("kprobe/__x64_sys_socket")
+int BPF_KPROBE_SYSCALL(socket_enter, int d, int t, int p)
+{
+ pid_t pid = bpf_get_current_pid_tgid() >> 32;
+
+ if (pid != my_pid)
+ return 0;
+
+ domain = d;
+ type = t;
+ protocol = p;
+ return 0;
+}
+
+SEC("kretprobe/__x64_sys_socket")
+int BPF_KRETPROBE_SYSCALL(socket_exit, int ret)
+{
+ pid_t pid = bpf_get_current_pid_tgid() >> 32;
+
+ if (pid != my_pid)
+ return 0;
+
+ fd = ret;
+ return 0;
+}
+
+char _license[] SEC("license") = "GPL";
--
2.30.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next 1/2] libbpf: Add BPF_KPROBE_SYSCALL/BPF_KRETPROBE_SYSCALL macros
2021-12-21 5:53 ` [PATCH bpf-next 1/2] libbpf: Add BPF_KPROBE_SYSCALL/BPF_KRETPROBE_SYSCALL macros Hengqi Chen
@ 2021-12-21 15:53 ` Yonghong Song
2021-12-22 0:18 ` Andrii Nakryiko
1 sibling, 0 replies; 10+ messages in thread
From: Yonghong Song @ 2021-12-21 15:53 UTC (permalink / raw)
To: Hengqi Chen, bpf, andrii
On 12/20/21 9:53 PM, Hengqi Chen wrote:
> Add syscall-specific variants of BPF_KPROBE/BPF_KRETPROBE named
> BPF_KPROBE_SYSCALL/BPF_KRETPROBE_SYSCALL ([0]). These new macros
> hide the underlying way of getting syscall input arguments and
> return values. With these new macros, the following code:
>
> SEC("kprobe/__x64_sys_close")
> int BPF_KPROBE(do_sys_close, struct pt_regs *regs)
> {
> int fd;
>
> fd = PT_REGS_PARM1_CORE(regs);
> /* do something with fd */
> }
>
> can be written as:
>
> SEC("kprobe/__x64_sys_close")
> int BPF_KPROBE_SYSCALL(do_sys_close, int fd)
> {
> /* do something with fd */
> }
>
> [0] Closes: https://github.com/libbpf/libbpf/issues/425
>
> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
> ---
> tools/lib/bpf/bpf_tracing.h | 45 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 45 insertions(+)
>
> diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
> index db05a5937105..eb4b567e443f 100644
> --- a/tools/lib/bpf/bpf_tracing.h
> +++ b/tools/lib/bpf/bpf_tracing.h
> @@ -489,4 +489,49 @@ typeof(name(0)) name(struct pt_regs *ctx) \
> } \
> static __always_inline typeof(name(0)) ____##name(struct pt_regs *ctx, ##args)
>
> +#define ___bpf_syscall_args0() ctx, regs
> +#define ___bpf_syscall_args1(x) \
> + ___bpf_syscall_args0(), (void *)PT_REGS_PARM1_CORE(regs)
> +#define ___bpf_syscall_args2(x, args...) \
> + ___bpf_syscall_args1(args), (void *)PT_REGS_PARM2_CORE(regs)
> +#define ___bpf_syscall_args3(x, args...) \
> + ___bpf_syscall_args2(args), (void *)PT_REGS_PARM3_CORE(regs)
> +#define ___bpf_syscall_args4(x, args...) \
> + ___bpf_syscall_args3(args), (void *)PT_REGS_PARM4_CORE(regs)
We probably need to use a syscall variant of PT_REGS_PARAM4 here, see
https://lore.kernel.org/bpf/TYCPR01MB59360988D96E23FBA97DAE0AF57C9@TYCPR01MB5936.jpnprd01.prod.outlook.com/
> +#define ___bpf_syscall_args5(x, args...) \
> + ___bpf_syscall_args4(args), (void *)PT_REGS_PARM5_CORE(regs)
> +#define ___bpf_syscall_args(args...) \
> + ___bpf_apply(___bpf_syscall_args, ___bpf_narg(args))(args)
> +
> +/*
> + * BPF_KPROBE_SYSCALL is a variant of BPF_KPROBE, which is intended for
> + * tracing syscall functions. It hides the underlying platform-specific
> + * low-level way of getting syscall input arguments from struct pt_regs, and
> + * provides a familiar typed and named function arguments syntax and
> + * semantics of accessing syscall input paremeters.
> + *
> + * Original struct pt_regs* context is preserved as 'ctx' argument. This might
> + * be necessary when using BPF helpers like bpf_perf_event_output().
> + */
> +#define BPF_KPROBE_SYSCALL(name, args...) \
> +name(struct pt_regs *ctx); \
> +static __attribute__((always_inline)) typeof(name(0)) \
> +____##name(struct pt_regs *ctx, struct pt_regs *regs, ##args); \
> +typeof(name(0)) name(struct pt_regs *ctx) \
> +{ \
> + _Pragma("GCC diagnostic push") \
> + _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
> + struct pt_regs *regs = PT_REGS_PARM1(ctx); \
> + return ____##name(___bpf_syscall_args(args)); \
> + _Pragma("GCC diagnostic pop") \
> +} \
> +static __attribute__((always_inline)) typeof(name(0)) \
> +____##name(struct pt_regs *ctx, struct pt_regs *regs, ##args)
> +
> +/*
> + * BPF_KRETPROBE_SYSCALL is just an alias to BPF_KRETPROBE,
> + * it provides optional return value (in addition to `struct pt_regs *ctx`)
> + */
> +#define BPF_KRETPROBE_SYSCALL BPF_KRETPROBE
> +
> #endif
> --
> 2.30.2
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next 1/2] libbpf: Add BPF_KPROBE_SYSCALL/BPF_KRETPROBE_SYSCALL macros
2021-12-21 5:53 ` [PATCH bpf-next 1/2] libbpf: Add BPF_KPROBE_SYSCALL/BPF_KRETPROBE_SYSCALL macros Hengqi Chen
2021-12-21 15:53 ` Yonghong Song
@ 2021-12-22 0:18 ` Andrii Nakryiko
2021-12-23 12:11 ` Hengqi Chen
1 sibling, 1 reply; 10+ messages in thread
From: Andrii Nakryiko @ 2021-12-22 0:18 UTC (permalink / raw)
To: Hengqi Chen; +Cc: bpf, Andrii Nakryiko
On Mon, Dec 20, 2021 at 9:53 PM Hengqi Chen <hengqi.chen@gmail.com> wrote:
>
> Add syscall-specific variants of BPF_KPROBE/BPF_KRETPROBE named
> BPF_KPROBE_SYSCALL/BPF_KRETPROBE_SYSCALL ([0]). These new macros
> hide the underlying way of getting syscall input arguments and
> return values. With these new macros, the following code:
>
> SEC("kprobe/__x64_sys_close")
> int BPF_KPROBE(do_sys_close, struct pt_regs *regs)
> {
> int fd;
>
> fd = PT_REGS_PARM1_CORE(regs);
> /* do something with fd */
> }
>
> can be written as:
>
> SEC("kprobe/__x64_sys_close")
> int BPF_KPROBE_SYSCALL(do_sys_close, int fd)
> {
> /* do something with fd */
> }
>
> [0] Closes: https://github.com/libbpf/libbpf/issues/425
>
> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
> ---
As Yonghong mentioned, let's wait for PT_REGS_PARMx_SYSCALL macros to
land and use those (due to 4th argument quirkiness on x86 arches).
> tools/lib/bpf/bpf_tracing.h | 45 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 45 insertions(+)
>
> diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
> index db05a5937105..eb4b567e443f 100644
> --- a/tools/lib/bpf/bpf_tracing.h
> +++ b/tools/lib/bpf/bpf_tracing.h
> @@ -489,4 +489,49 @@ typeof(name(0)) name(struct pt_regs *ctx) \
> } \
> static __always_inline typeof(name(0)) ____##name(struct pt_regs *ctx, ##args)
>
> +#define ___bpf_syscall_args0() ctx, regs
> +#define ___bpf_syscall_args1(x) \
> + ___bpf_syscall_args0(), (void *)PT_REGS_PARM1_CORE(regs)
> +#define ___bpf_syscall_args2(x, args...) \
> + ___bpf_syscall_args1(args), (void *)PT_REGS_PARM2_CORE(regs)
> +#define ___bpf_syscall_args3(x, args...) \
> + ___bpf_syscall_args2(args), (void *)PT_REGS_PARM3_CORE(regs)
> +#define ___bpf_syscall_args4(x, args...) \
> + ___bpf_syscall_args3(args), (void *)PT_REGS_PARM4_CORE(regs)
> +#define ___bpf_syscall_args5(x, args...) \
> + ___bpf_syscall_args4(args), (void *)PT_REGS_PARM5_CORE(regs)
> +#define ___bpf_syscall_args(args...) \
> + ___bpf_apply(___bpf_syscall_args, ___bpf_narg(args))(args)
try keeping each definition on a single line, make them much more
readable and I think still fits in 100 character limit
> +
> +/*
> + * BPF_KPROBE_SYSCALL is a variant of BPF_KPROBE, which is intended for
> + * tracing syscall functions. It hides the underlying platform-specific
let's add a simple example to explain what kind of tracing syscall
functions we mean.
"tracing syscall functions, like __x64_sys_close." ?
> + * low-level way of getting syscall input arguments from struct pt_regs, and
> + * provides a familiar typed and named function arguments syntax and
> + * semantics of accessing syscall input paremeters.
typo: parameters
> + *
> + * Original struct pt_regs* context is preserved as 'ctx' argument. This might
> + * be necessary when using BPF helpers like bpf_perf_event_output().
> + */
> +#define BPF_KPROBE_SYSCALL(name, args...) \
> +name(struct pt_regs *ctx); \
> +static __attribute__((always_inline)) typeof(name(0)) \
> +____##name(struct pt_regs *ctx, struct pt_regs *regs, ##args); \
> +typeof(name(0)) name(struct pt_regs *ctx) \
> +{ \
> + _Pragma("GCC diagnostic push") \
> + _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
> + struct pt_regs *regs = PT_REGS_PARM1(ctx); \
please move it out of _Pragma region, no need to guard it
> + return ____##name(___bpf_syscall_args(args)); \
> + _Pragma("GCC diagnostic pop") \
> +} \
> +static __attribute__((always_inline)) typeof(name(0)) \
> +____##name(struct pt_regs *ctx, struct pt_regs *regs, ##args)
I don't think we need to add another magical hidden argument "regs".
Anyone who will need it for something can get it from the hidden ctx
with PT_REGS_PARM1(ctx) anyways.
> +
> +/*
> + * BPF_KRETPROBE_SYSCALL is just an alias to BPF_KRETPROBE,
> + * it provides optional return value (in addition to `struct pt_regs *ctx`)
> + */
> +#define BPF_KRETPROBE_SYSCALL BPF_KRETPROBE
> +
hm... do we even need BPF_KRETPROBE_SYSCALL then? Let's drop it, it
doesn't provide much value, just creates a confusion.
> #endif
> --
> 2.30.2
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next 2/2] selftests/bpf: Test BPF_KPROBE_SYSCALL/BPF_KRETPROBE_SYSCALL macros
2021-12-21 5:53 ` [PATCH bpf-next 2/2] selftests/bpf: Test " Hengqi Chen
@ 2021-12-22 0:34 ` Andrii Nakryiko
2021-12-22 0:37 ` Andrii Nakryiko
1 sibling, 0 replies; 10+ messages in thread
From: Andrii Nakryiko @ 2021-12-22 0:34 UTC (permalink / raw)
To: Hengqi Chen; +Cc: bpf, Andrii Nakryiko
On Mon, Dec 20, 2021 at 9:54 PM Hengqi Chen <hengqi.chen@gmail.com> wrote:
>
> Add tests for the newly added BPF_KPROBE_SYSCALL/BPF_KRETPROBE_SYSCALL macros.
>
> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
> ---
> .../selftests/bpf/prog_tests/kprobe_syscall.c | 40 ++++++++++++++++++
> .../selftests/bpf/progs/test_kprobe_syscall.c | 41 +++++++++++++++++++
> 2 files changed, 81 insertions(+)
> create mode 100644 tools/testing/selftests/bpf/prog_tests/kprobe_syscall.c
> create mode 100644 tools/testing/selftests/bpf/progs/test_kprobe_syscall.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/kprobe_syscall.c b/tools/testing/selftests/bpf/prog_tests/kprobe_syscall.c
> new file mode 100644
> index 000000000000..a1fad70bbb69
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/kprobe_syscall.c
> @@ -0,0 +1,40 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2021 Hengqi Chen */
> +
> +#include <test_progs.h>
> +#include <sys/types.h>
> +#include <sys/socket.h>
> +#include "test_kprobe_syscall.skel.h"
> +
> +void test_kprobe_syscall(void)
> +{
> + struct test_kprobe_syscall *skel;
> + int err, fd = 0;
> +
> + skel = test_kprobe_syscall__open();
> + if (!ASSERT_OK_PTR(skel, "could not open BPF object"))
"could not open BPF object" is not an error message, it's an
identifier of what you are checking (skel here). If assertion fails,
we'll see something like: "<identifier> is not a valid pointer". So
please pick it properly here and below.
> + return;
> +
> + skel->rodata->my_pid = getpid();
> +
> + err = test_kprobe_syscall__load(skel);
> + if (!ASSERT_OK(err, "could not load BPF object"))
> + goto cleanup;
> +
> + err = test_kprobe_syscall__attach(skel);
> + if (!ASSERT_OK(err, "could not attach BPF object"))
> + goto cleanup;
> +
> + fd = socket(AF_UNIX, SOCK_STREAM, 0);
maybe use something non-zero for the 3rd argument? Also see discussion
on previous patch, let's test something that has at least 4 arguments.
> +
> + ASSERT_GT(fd, 0, "socket failed");
see comment below, it should be GE
> + ASSERT_EQ(skel->bss->domain, AF_UNIX, "BPF_KPROBE_SYSCALL failed");
> + ASSERT_EQ(skel->bss->type, SOCK_STREAM, "BPF_KPROBE_SYSCALL failed");
> + ASSERT_EQ(skel->bss->protocol, 0, "BPF_KPROBE_SYSCALL failed");
> + ASSERT_EQ(skel->bss->fd, fd, "BPF_KRETPROBE_SYSCALL failed");
> +
> +cleanup:
> + if (fd)
it's highly unlikely, but for FDs the check should be >= 0
> + close(fd);
> + test_kprobe_syscall__destroy(skel);
> +}
[...]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next 2/2] selftests/bpf: Test BPF_KPROBE_SYSCALL/BPF_KRETPROBE_SYSCALL macros
2021-12-21 5:53 ` [PATCH bpf-next 2/2] selftests/bpf: Test " Hengqi Chen
2021-12-22 0:34 ` Andrii Nakryiko
@ 2021-12-22 0:37 ` Andrii Nakryiko
2021-12-23 12:16 ` Hengqi Chen
1 sibling, 1 reply; 10+ messages in thread
From: Andrii Nakryiko @ 2021-12-22 0:37 UTC (permalink / raw)
To: Hengqi Chen; +Cc: bpf, Andrii Nakryiko
On Mon, Dec 20, 2021 at 9:54 PM Hengqi Chen <hengqi.chen@gmail.com> wrote:
>
> Add tests for the newly added BPF_KPROBE_SYSCALL/BPF_KRETPROBE_SYSCALL macros.
>
> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
> ---
> .../selftests/bpf/prog_tests/kprobe_syscall.c | 40 ++++++++++++++++++
> .../selftests/bpf/progs/test_kprobe_syscall.c | 41 +++++++++++++++++++
> 2 files changed, 81 insertions(+)
> create mode 100644 tools/testing/selftests/bpf/prog_tests/kprobe_syscall.c
> create mode 100644 tools/testing/selftests/bpf/progs/test_kprobe_syscall.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/kprobe_syscall.c b/tools/testing/selftests/bpf/prog_tests/kprobe_syscall.c
> new file mode 100644
> index 000000000000..a1fad70bbb69
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/kprobe_syscall.c
> @@ -0,0 +1,40 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2021 Hengqi Chen */
> +
> +#include <test_progs.h>
> +#include <sys/types.h>
> +#include <sys/socket.h>
> +#include "test_kprobe_syscall.skel.h"
> +
> +void test_kprobe_syscall(void)
> +{
> + struct test_kprobe_syscall *skel;
> + int err, fd = 0;
> +
> + skel = test_kprobe_syscall__open();
> + if (!ASSERT_OK_PTR(skel, "could not open BPF object"))
> + return;
> +
> + skel->rodata->my_pid = getpid();
> +
> + err = test_kprobe_syscall__load(skel);
> + if (!ASSERT_OK(err, "could not load BPF object"))
> + goto cleanup;
> +
> + err = test_kprobe_syscall__attach(skel);
> + if (!ASSERT_OK(err, "could not attach BPF object"))
> + goto cleanup;
> +
> + fd = socket(AF_UNIX, SOCK_STREAM, 0);
> +
> + ASSERT_GT(fd, 0, "socket failed");
> + ASSERT_EQ(skel->bss->domain, AF_UNIX, "BPF_KPROBE_SYSCALL failed");
> + ASSERT_EQ(skel->bss->type, SOCK_STREAM, "BPF_KPROBE_SYSCALL failed");
> + ASSERT_EQ(skel->bss->protocol, 0, "BPF_KPROBE_SYSCALL failed");
> + ASSERT_EQ(skel->bss->fd, fd, "BPF_KRETPROBE_SYSCALL failed");
> +
> +cleanup:
> + if (fd)
> + close(fd);
> + test_kprobe_syscall__destroy(skel);
> +}
> diff --git a/tools/testing/selftests/bpf/progs/test_kprobe_syscall.c b/tools/testing/selftests/bpf/progs/test_kprobe_syscall.c
> new file mode 100644
> index 000000000000..ecef9d19007c
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_kprobe_syscall.c
> @@ -0,0 +1,41 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2021 Hengqi Chen */
> +
> +#include "vmlinux.h"
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +#include <bpf/bpf_core_read.h>
> +
> +const volatile pid_t my_pid = 0;
> +int domain = 0;
> +int type = 0;
> +int protocol = 0;
> +int fd = 0;
> +
> +SEC("kprobe/__x64_sys_socket")
> +int BPF_KPROBE_SYSCALL(socket_enter, int d, int t, int p)
> +{
> + pid_t pid = bpf_get_current_pid_tgid() >> 32;
> +
> + if (pid != my_pid)
> + return 0;
> +
> + domain = d;
> + type = t;
> + protocol = p;
> + return 0;
> +}
> +
> +SEC("kretprobe/__x64_sys_socket")
oh, please also use SYS_PREFIX instead of hard-coding __x64. This is
very x86-64-specific and we have other architectures tested by
selftests, so this makes it automatically fail on non-x86_64.
If you get a chance, try also cleaning up other __x64_ uses in the
selftests as a separate patch. Thank you!
> +int BPF_KRETPROBE_SYSCALL(socket_exit, int ret)
> +{
> + pid_t pid = bpf_get_current_pid_tgid() >> 32;
> +
> + if (pid != my_pid)
> + return 0;
> +
> + fd = ret;
> + return 0;
> +}
> +
> +char _license[] SEC("license") = "GPL";
> --
> 2.30.2
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next 1/2] libbpf: Add BPF_KPROBE_SYSCALL/BPF_KRETPROBE_SYSCALL macros
2021-12-22 0:18 ` Andrii Nakryiko
@ 2021-12-23 12:11 ` Hengqi Chen
2022-01-06 20:27 ` Andrii Nakryiko
0 siblings, 1 reply; 10+ messages in thread
From: Hengqi Chen @ 2021-12-23 12:11 UTC (permalink / raw)
To: Andrii Nakryiko; +Cc: bpf, Andrii Nakryiko
Hi, Andrii
On 2021/12/22 8:18 AM, Andrii Nakryiko wrote:
> On Mon, Dec 20, 2021 at 9:53 PM Hengqi Chen <hengqi.chen@gmail.com> wrote:
>>
>> Add syscall-specific variants of BPF_KPROBE/BPF_KRETPROBE named
>> BPF_KPROBE_SYSCALL/BPF_KRETPROBE_SYSCALL ([0]). These new macros
>> hide the underlying way of getting syscall input arguments and
>> return values. With these new macros, the following code:
>>
>> SEC("kprobe/__x64_sys_close")
>> int BPF_KPROBE(do_sys_close, struct pt_regs *regs)
>> {
>> int fd;
>>
>> fd = PT_REGS_PARM1_CORE(regs);
>> /* do something with fd */
>> }
>>
>> can be written as:
>>
>> SEC("kprobe/__x64_sys_close")
>> int BPF_KPROBE_SYSCALL(do_sys_close, int fd)
>> {
>> /* do something with fd */
>> }
>>
>> [0] Closes: https://github.com/libbpf/libbpf/issues/425
>>
>> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
>> ---
>
> As Yonghong mentioned, let's wait for PT_REGS_PARMx_SYSCALL macros to
> land and use those (due to 4th argument quirkiness on x86 arches).
>
I see those patches, will wait.
>> tools/lib/bpf/bpf_tracing.h | 45 +++++++++++++++++++++++++++++++++++++
>> 1 file changed, 45 insertions(+)
>>
>> diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
>> index db05a5937105..eb4b567e443f 100644
>> --- a/tools/lib/bpf/bpf_tracing.h
>> +++ b/tools/lib/bpf/bpf_tracing.h
>> @@ -489,4 +489,49 @@ typeof(name(0)) name(struct pt_regs *ctx) \
>> } \
>> static __always_inline typeof(name(0)) ____##name(struct pt_regs *ctx, ##args)
>>
>> +#define ___bpf_syscall_args0() ctx, regs
>> +#define ___bpf_syscall_args1(x) \
>> + ___bpf_syscall_args0(), (void *)PT_REGS_PARM1_CORE(regs)
>> +#define ___bpf_syscall_args2(x, args...) \
>> + ___bpf_syscall_args1(args), (void *)PT_REGS_PARM2_CORE(regs)
>> +#define ___bpf_syscall_args3(x, args...) \
>> + ___bpf_syscall_args2(args), (void *)PT_REGS_PARM3_CORE(regs)
>> +#define ___bpf_syscall_args4(x, args...) \
>> + ___bpf_syscall_args3(args), (void *)PT_REGS_PARM4_CORE(regs)
>> +#define ___bpf_syscall_args5(x, args...) \
>> + ___bpf_syscall_args4(args), (void *)PT_REGS_PARM5_CORE(regs)
>> +#define ___bpf_syscall_args(args...) \
>> + ___bpf_apply(___bpf_syscall_args, ___bpf_narg(args))(args)
>
> try keeping each definition on a single line, make them much more
> readable and I think still fits in 100 character limit
>
This should be addressed by your patch, will build on top of it.
>> +
>> +/*
>> + * BPF_KPROBE_SYSCALL is a variant of BPF_KPROBE, which is intended for
>> + * tracing syscall functions. It hides the underlying platform-specific
>
> let's add a simple example to explain what kind of tracing syscall
> functions we mean.
>
> "tracing syscall functions, like __x64_sys_close." ?
>
>> + * low-level way of getting syscall input arguments from struct pt_regs, and
>> + * provides a familiar typed and named function arguments syntax and
>> + * semantics of accessing syscall input paremeters.
>
> typo: parameters
>
Ack.
>> + *
>> + * Original struct pt_regs* context is preserved as 'ctx' argument. This might
>> + * be necessary when using BPF helpers like bpf_perf_event_output().
>> + */
>> +#define BPF_KPROBE_SYSCALL(name, args...) \
>> +name(struct pt_regs *ctx); \
>> +static __attribute__((always_inline)) typeof(name(0)) \
>> +____##name(struct pt_regs *ctx, struct pt_regs *regs, ##args); \
>> +typeof(name(0)) name(struct pt_regs *ctx) \
>> +{ \
>> + _Pragma("GCC diagnostic push") \
>> + _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
>> + struct pt_regs *regs = PT_REGS_PARM1(ctx); \
>
> please move it out of _Pragma region, no need to guard it
>
Ack.
>> + return ____##name(___bpf_syscall_args(args)); \
>> + _Pragma("GCC diagnostic pop") \
>> +} \
>> +static __attribute__((always_inline)) typeof(name(0)) \
>> +____##name(struct pt_regs *ctx, struct pt_regs *regs, ##args)
>
> I don't think we need to add another magical hidden argument "regs".
> Anyone who will need it for something can get it from the hidden ctx
> with PT_REGS_PARM1(ctx) anyways.
>
Yes, this should be removed, otherwise it may conflict with user-defined args.
>> +
>> +/*
>> + * BPF_KRETPROBE_SYSCALL is just an alias to BPF_KRETPROBE,
>> + * it provides optional return value (in addition to `struct pt_regs *ctx`)
>> + */
>> +#define BPF_KRETPROBE_SYSCALL BPF_KRETPROBE
>> +
>
> hm... do we even need BPF_KRETPROBE_SYSCALL then? Let's drop it, it
> doesn't provide much value, just creates a confusion.
>
OK, will drop it.
>
>> #endif
>> --
>> 2.30.2
---
Hengqi
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next 2/2] selftests/bpf: Test BPF_KPROBE_SYSCALL/BPF_KRETPROBE_SYSCALL macros
2021-12-22 0:37 ` Andrii Nakryiko
@ 2021-12-23 12:16 ` Hengqi Chen
0 siblings, 0 replies; 10+ messages in thread
From: Hengqi Chen @ 2021-12-23 12:16 UTC (permalink / raw)
To: Andrii Nakryiko; +Cc: bpf, Andrii Nakryiko
On 2021/12/22 8:37 AM, Andrii Nakryiko wrote:
> On Mon, Dec 20, 2021 at 9:54 PM Hengqi Chen <hengqi.chen@gmail.com> wrote:
>>
>> Add tests for the newly added BPF_KPROBE_SYSCALL/BPF_KRETPROBE_SYSCALL macros.
>>
>> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
>> ---
>> .../selftests/bpf/prog_tests/kprobe_syscall.c | 40 ++++++++++++++++++
>> .../selftests/bpf/progs/test_kprobe_syscall.c | 41 +++++++++++++++++++
>> 2 files changed, 81 insertions(+)
>> create mode 100644 tools/testing/selftests/bpf/prog_tests/kprobe_syscall.c
>> create mode 100644 tools/testing/selftests/bpf/progs/test_kprobe_syscall.c
>>
>> diff --git a/tools/testing/selftests/bpf/prog_tests/kprobe_syscall.c b/tools/testing/selftests/bpf/prog_tests/kprobe_syscall.c
>> new file mode 100644
>> index 000000000000..a1fad70bbb69
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/prog_tests/kprobe_syscall.c
>> @@ -0,0 +1,40 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright (c) 2021 Hengqi Chen */
>> +
>> +#include <test_progs.h>
>> +#include <sys/types.h>
>> +#include <sys/socket.h>
>> +#include "test_kprobe_syscall.skel.h"
>> +
>> +void test_kprobe_syscall(void)
>> +{
>> + struct test_kprobe_syscall *skel;
>> + int err, fd = 0;
>> +
>> + skel = test_kprobe_syscall__open();
>> + if (!ASSERT_OK_PTR(skel, "could not open BPF object"))
>> + return;
>> +
>> + skel->rodata->my_pid = getpid();
>> +
>> + err = test_kprobe_syscall__load(skel);
>> + if (!ASSERT_OK(err, "could not load BPF object"))
>> + goto cleanup;
>> +
>> + err = test_kprobe_syscall__attach(skel);
>> + if (!ASSERT_OK(err, "could not attach BPF object"))
>> + goto cleanup;
>> +
>> + fd = socket(AF_UNIX, SOCK_STREAM, 0);
>> +
>> + ASSERT_GT(fd, 0, "socket failed");
>> + ASSERT_EQ(skel->bss->domain, AF_UNIX, "BPF_KPROBE_SYSCALL failed");
>> + ASSERT_EQ(skel->bss->type, SOCK_STREAM, "BPF_KPROBE_SYSCALL failed");
>> + ASSERT_EQ(skel->bss->protocol, 0, "BPF_KPROBE_SYSCALL failed");
>> + ASSERT_EQ(skel->bss->fd, fd, "BPF_KRETPROBE_SYSCALL failed");
>> +
>> +cleanup:
>> + if (fd)
>> + close(fd);
>> + test_kprobe_syscall__destroy(skel);
>> +}
>> diff --git a/tools/testing/selftests/bpf/progs/test_kprobe_syscall.c b/tools/testing/selftests/bpf/progs/test_kprobe_syscall.c
>> new file mode 100644
>> index 000000000000..ecef9d19007c
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/progs/test_kprobe_syscall.c
>> @@ -0,0 +1,41 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright (c) 2021 Hengqi Chen */
>> +
>> +#include "vmlinux.h"
>> +#include <bpf/bpf_helpers.h>
>> +#include <bpf/bpf_tracing.h>
>> +#include <bpf/bpf_core_read.h>
>> +
>> +const volatile pid_t my_pid = 0;
>> +int domain = 0;
>> +int type = 0;
>> +int protocol = 0;
>> +int fd = 0;
>> +
>> +SEC("kprobe/__x64_sys_socket")
>> +int BPF_KPROBE_SYSCALL(socket_enter, int d, int t, int p)
>> +{
>> + pid_t pid = bpf_get_current_pid_tgid() >> 32;
>> +
>> + if (pid != my_pid)
>> + return 0;
>> +
>> + domain = d;
>> + type = t;
>> + protocol = p;
>> + return 0;
>> +}
>> +
>> +SEC("kretprobe/__x64_sys_socket")
>
> oh, please also use SYS_PREFIX instead of hard-coding __x64. This is
> very x86-64-specific and we have other architectures tested by
> selftests, so this makes it automatically fail on non-x86_64.
>
I just followed some existing selftests, didn't realize SYS_PREFIX.
Will update to use SYS_PREFIX.
> If you get a chance, try also cleaning up other __x64_ uses in the
> selftests as a separate patch. Thank you!
>
OK, will do.
>> +int BPF_KRETPROBE_SYSCALL(socket_exit, int ret)
>> +{
>> + pid_t pid = bpf_get_current_pid_tgid() >> 32;
>> +
>> + if (pid != my_pid)
>> + return 0;
>> +
>> + fd = ret;
>> + return 0;
>> +}
>> +
>> +char _license[] SEC("license") = "GPL";
>> --
>> 2.30.2
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH bpf-next 1/2] libbpf: Add BPF_KPROBE_SYSCALL/BPF_KRETPROBE_SYSCALL macros
2021-12-23 12:11 ` Hengqi Chen
@ 2022-01-06 20:27 ` Andrii Nakryiko
0 siblings, 0 replies; 10+ messages in thread
From: Andrii Nakryiko @ 2022-01-06 20:27 UTC (permalink / raw)
To: Hengqi Chen; +Cc: bpf, Andrii Nakryiko
On Thu, Dec 23, 2021 at 4:11 AM Hengqi Chen <hengqi.chen@gmail.com> wrote:
>
> Hi, Andrii
>
> On 2021/12/22 8:18 AM, Andrii Nakryiko wrote:
> > On Mon, Dec 20, 2021 at 9:53 PM Hengqi Chen <hengqi.chen@gmail.com> wrote:
> >>
> >> Add syscall-specific variants of BPF_KPROBE/BPF_KRETPROBE named
> >> BPF_KPROBE_SYSCALL/BPF_KRETPROBE_SYSCALL ([0]). These new macros
> >> hide the underlying way of getting syscall input arguments and
> >> return values. With these new macros, the following code:
> >>
> >> SEC("kprobe/__x64_sys_close")
> >> int BPF_KPROBE(do_sys_close, struct pt_regs *regs)
> >> {
> >> int fd;
> >>
> >> fd = PT_REGS_PARM1_CORE(regs);
> >> /* do something with fd */
> >> }
> >>
> >> can be written as:
> >>
> >> SEC("kprobe/__x64_sys_close")
> >> int BPF_KPROBE_SYSCALL(do_sys_close, int fd)
> >> {
> >> /* do something with fd */
> >> }
> >>
> >> [0] Closes: https://github.com/libbpf/libbpf/issues/425
> >>
> >> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
> >> ---
> >
> > As Yonghong mentioned, let's wait for PT_REGS_PARMx_SYSCALL macros to
> > land and use those (due to 4th argument quirkiness on x86 arches).
> >
>
> I see those patches, will wait.
They got merged, feel free to resubmit.
>
> >> tools/lib/bpf/bpf_tracing.h | 45 +++++++++++++++++++++++++++++++++++++
> >> 1 file changed, 45 insertions(+)
> >>
> >> diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
> >> index db05a5937105..eb4b567e443f 100644
> >> --- a/tools/lib/bpf/bpf_tracing.h
> >> +++ b/tools/lib/bpf/bpf_tracing.h
> >> @@ -489,4 +489,49 @@ typeof(name(0)) name(struct pt_regs *ctx) \
> >> } \
> >> static __always_inline typeof(name(0)) ____##name(struct pt_regs *ctx, ##args)
> >>
> >> +#define ___bpf_syscall_args0() ctx, regs
> >> +#define ___bpf_syscall_args1(x) \
> >> + ___bpf_syscall_args0(), (void *)PT_REGS_PARM1_CORE(regs)
> >> +#define ___bpf_syscall_args2(x, args...) \
> >> + ___bpf_syscall_args1(args), (void *)PT_REGS_PARM2_CORE(regs)
> >> +#define ___bpf_syscall_args3(x, args...) \
> >> + ___bpf_syscall_args2(args), (void *)PT_REGS_PARM3_CORE(regs)
> >> +#define ___bpf_syscall_args4(x, args...) \
> >> + ___bpf_syscall_args3(args), (void *)PT_REGS_PARM4_CORE(regs)
> >> +#define ___bpf_syscall_args5(x, args...) \
> >> + ___bpf_syscall_args4(args), (void *)PT_REGS_PARM5_CORE(regs)
> >> +#define ___bpf_syscall_args(args...) \
> >> + ___bpf_apply(___bpf_syscall_args, ___bpf_narg(args))(args)
> >
> > try keeping each definition on a single line, make them much more
> > readable and I think still fits in 100 character limit
> >
>
> This should be addressed by your patch, will build on top of it.
>
> >> +
> >> +/*
> >> + * BPF_KPROBE_SYSCALL is a variant of BPF_KPROBE, which is intended for
> >> + * tracing syscall functions. It hides the underlying platform-specific
> >
> > let's add a simple example to explain what kind of tracing syscall
> > functions we mean.
> >
> > "tracing syscall functions, like __x64_sys_close." ?
> >
> >> + * low-level way of getting syscall input arguments from struct pt_regs, and
> >> + * provides a familiar typed and named function arguments syntax and
> >> + * semantics of accessing syscall input paremeters.
> >
> > typo: parameters
> >
>
> Ack.
>
> >> + *
> >> + * Original struct pt_regs* context is preserved as 'ctx' argument. This might
> >> + * be necessary when using BPF helpers like bpf_perf_event_output().
> >> + */
> >> +#define BPF_KPROBE_SYSCALL(name, args...) \
> >> +name(struct pt_regs *ctx); \
> >> +static __attribute__((always_inline)) typeof(name(0)) \
> >> +____##name(struct pt_regs *ctx, struct pt_regs *regs, ##args); \
> >> +typeof(name(0)) name(struct pt_regs *ctx) \
> >> +{ \
> >> + _Pragma("GCC diagnostic push") \
> >> + _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
> >> + struct pt_regs *regs = PT_REGS_PARM1(ctx); \
> >
> > please move it out of _Pragma region, no need to guard it
> >
>
> Ack.
>
> >> + return ____##name(___bpf_syscall_args(args)); \
> >> + _Pragma("GCC diagnostic pop") \
> >> +} \
> >> +static __attribute__((always_inline)) typeof(name(0)) \
> >> +____##name(struct pt_regs *ctx, struct pt_regs *regs, ##args)
> >
> > I don't think we need to add another magical hidden argument "regs".
> > Anyone who will need it for something can get it from the hidden ctx
> > with PT_REGS_PARM1(ctx) anyways.
> >
>
> Yes, this should be removed, otherwise it may conflict with user-defined args.
>
> >> +
> >> +/*
> >> + * BPF_KRETPROBE_SYSCALL is just an alias to BPF_KRETPROBE,
> >> + * it provides optional return value (in addition to `struct pt_regs *ctx`)
> >> + */
> >> +#define BPF_KRETPROBE_SYSCALL BPF_KRETPROBE
> >> +
> >
> > hm... do we even need BPF_KRETPROBE_SYSCALL then? Let's drop it, it
> > doesn't provide much value, just creates a confusion.
> >
>
> OK, will drop it.
>
> >
> >> #endif
> >> --
> >> 2.30.2
>
> ---
> Hengqi
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-01-06 20:27 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-21 5:53 [PATCH bpf-next 0/2] libbpf: Add syscall-specific variants of BPF_KPROBE/BPF_KRETPROBE Hengqi Chen
2021-12-21 5:53 ` [PATCH bpf-next 1/2] libbpf: Add BPF_KPROBE_SYSCALL/BPF_KRETPROBE_SYSCALL macros Hengqi Chen
2021-12-21 15:53 ` Yonghong Song
2021-12-22 0:18 ` Andrii Nakryiko
2021-12-23 12:11 ` Hengqi Chen
2022-01-06 20:27 ` Andrii Nakryiko
2021-12-21 5:53 ` [PATCH bpf-next 2/2] selftests/bpf: Test " Hengqi Chen
2021-12-22 0:34 ` Andrii Nakryiko
2021-12-22 0:37 ` Andrii Nakryiko
2021-12-23 12:16 ` Hengqi Chen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).