These macros are convenient wrappers around the bpf_seq_printf and
bpf_snprintf helpers. They are currently provided by bpf_tracing.h which
targets low level tracing primitives. bpf_helpers.h is a better fit.
The __bpf_narg and __bpf_apply macros are needed in both files so
provided twice and guarded by ifndefs.
Reported-by: Andrii Nakryiko <[email protected]>
Signed-off-by: Florent Revest <[email protected]>
---
kernel/bpf/preload/iterators/iterators.bpf.c | 1 -
tools/lib/bpf/bpf_helpers.h | 70 +++++++++++++++++++
tools/lib/bpf/bpf_tracing.h | 62 +++-------------
.../bpf/progs/bpf_iter_bpf_hash_map.c | 1 -
.../selftests/bpf/progs/bpf_iter_bpf_map.c | 1 -
.../selftests/bpf/progs/bpf_iter_ipv6_route.c | 1 -
.../selftests/bpf/progs/bpf_iter_netlink.c | 1 -
.../selftests/bpf/progs/bpf_iter_task.c | 1 -
.../selftests/bpf/progs/bpf_iter_task_btf.c | 1 -
.../selftests/bpf/progs/bpf_iter_task_file.c | 1 -
.../selftests/bpf/progs/bpf_iter_task_stack.c | 1 -
.../selftests/bpf/progs/bpf_iter_task_vma.c | 1 -
.../selftests/bpf/progs/bpf_iter_tcp4.c | 1 -
.../selftests/bpf/progs/bpf_iter_tcp6.c | 1 -
.../selftests/bpf/progs/bpf_iter_udp4.c | 1 -
.../selftests/bpf/progs/bpf_iter_udp6.c | 1 -
.../selftests/bpf/progs/test_snprintf.c | 1 -
17 files changed, 80 insertions(+), 67 deletions(-)
diff --git a/kernel/bpf/preload/iterators/iterators.bpf.c b/kernel/bpf/preload/iterators/iterators.bpf.c
index 52aa7b38e8b8..03af863314ea 100644
--- a/kernel/bpf/preload/iterators/iterators.bpf.c
+++ b/kernel/bpf/preload/iterators/iterators.bpf.c
@@ -2,7 +2,6 @@
/* Copyright (c) 2020 Facebook */
#include <linux/bpf.h>
#include <bpf/bpf_helpers.h>
-#include <bpf/bpf_tracing.h>
#include <bpf/bpf_core_read.h>
#pragma clang attribute push (__attribute__((preserve_access_index)), apply_to = record)
diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
index 9720dc0b4605..68d992b30f26 100644
--- a/tools/lib/bpf/bpf_helpers.h
+++ b/tools/lib/bpf/bpf_helpers.h
@@ -158,4 +158,74 @@ enum libbpf_tristate {
#define __kconfig __attribute__((section(".kconfig")))
#define __ksym __attribute__((section(".ksyms")))
+#ifndef ___bpf_concat
+#define ___bpf_concat(a, b) a ## b
+#endif
+#ifndef ___bpf_apply
+#define ___bpf_apply(fn, n) ___bpf_concat(fn, n)
+#endif
+#ifndef ___bpf_nth
+#define ___bpf_nth(_, _1, _2, _3, _4, _5, _6, _7, _8, _9, _a, _b, _c, N, ...) N
+#endif
+#ifndef ___bpf_narg
+#define ___bpf_narg(...) \
+ ___bpf_nth(_, ##__VA_ARGS__, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0)
+#endif
+#ifndef ___bpf_empty
+#define ___bpf_empty(...) \
+ ___bpf_nth(_, ##__VA_ARGS__, N, N, N, N, N, N, N, N, N, N, 0)
+#endif
+
+#define ___bpf_fill0(arr, p, x) do {} while (0)
+#define ___bpf_fill1(arr, p, x) arr[p] = x
+#define ___bpf_fill2(arr, p, x, args...) arr[p] = x; ___bpf_fill1(arr, p + 1, args)
+#define ___bpf_fill3(arr, p, x, args...) arr[p] = x; ___bpf_fill2(arr, p + 1, args)
+#define ___bpf_fill4(arr, p, x, args...) arr[p] = x; ___bpf_fill3(arr, p + 1, args)
+#define ___bpf_fill5(arr, p, x, args...) arr[p] = x; ___bpf_fill4(arr, p + 1, args)
+#define ___bpf_fill6(arr, p, x, args...) arr[p] = x; ___bpf_fill5(arr, p + 1, args)
+#define ___bpf_fill7(arr, p, x, args...) arr[p] = x; ___bpf_fill6(arr, p + 1, args)
+#define ___bpf_fill8(arr, p, x, args...) arr[p] = x; ___bpf_fill7(arr, p + 1, args)
+#define ___bpf_fill9(arr, p, x, args...) arr[p] = x; ___bpf_fill8(arr, p + 1, args)
+#define ___bpf_fill10(arr, p, x, args...) arr[p] = x; ___bpf_fill9(arr, p + 1, args)
+#define ___bpf_fill11(arr, p, x, args...) arr[p] = x; ___bpf_fill10(arr, p + 1, args)
+#define ___bpf_fill12(arr, p, x, args...) arr[p] = x; ___bpf_fill11(arr, p + 1, args)
+#define ___bpf_fill(arr, args...) \
+ ___bpf_apply(___bpf_fill, ___bpf_narg(args))(arr, 0, args)
+
+/*
+ * BPF_SEQ_PRINTF to wrap bpf_seq_printf to-be-printed values
+ * in a structure.
+ */
+#define BPF_SEQ_PRINTF(seq, fmt, args...) \
+({ \
+ static const char ___fmt[] = fmt; \
+ unsigned long long ___param[___bpf_narg(args)]; \
+ \
+ _Pragma("GCC diagnostic push") \
+ _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
+ ___bpf_fill(___param, args); \
+ _Pragma("GCC diagnostic pop") \
+ \
+ bpf_seq_printf(seq, ___fmt, sizeof(___fmt), \
+ ___param, sizeof(___param)); \
+})
+
+/*
+ * BPF_SNPRINTF wraps the bpf_snprintf helper with variadic arguments instead of
+ * an array of u64.
+ */
+#define BPF_SNPRINTF(out, out_size, fmt, args...) \
+({ \
+ static const char ___fmt[] = fmt; \
+ unsigned long long ___param[___bpf_narg(args)]; \
+ \
+ _Pragma("GCC diagnostic push") \
+ _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
+ ___bpf_fill(___param, args); \
+ _Pragma("GCC diagnostic pop") \
+ \
+ bpf_snprintf(out, out_size, ___fmt, \
+ ___param, sizeof(___param)); \
+})
+
#endif
diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
index 8c954ebc0c7c..9139ea852f34 100644
--- a/tools/lib/bpf/bpf_tracing.h
+++ b/tools/lib/bpf/bpf_tracing.h
@@ -295,13 +295,23 @@ struct pt_regs;
(void *)(PT_REGS_FP(ctx) + sizeof(ip))); })
#endif
+#ifndef ___bpf_concat
#define ___bpf_concat(a, b) a ## b
+#endif
+#ifndef ___bpf_apply
#define ___bpf_apply(fn, n) ___bpf_concat(fn, n)
+#endif
+#ifndef ___bpf_nth
#define ___bpf_nth(_, _1, _2, _3, _4, _5, _6, _7, _8, _9, _a, _b, _c, N, ...) N
+#endif
+#ifndef ___bpf_narg
#define ___bpf_narg(...) \
___bpf_nth(_, ##__VA_ARGS__, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0)
+#endif
+#ifndef ___bpf_empty
#define ___bpf_empty(...) \
___bpf_nth(_, ##__VA_ARGS__, N, N, N, N, N, N, N, N, N, N, 0)
+#endif
#define ___bpf_ctx_cast0() ctx
#define ___bpf_ctx_cast1(x) ___bpf_ctx_cast0(), (void *)ctx[0]
@@ -413,56 +423,4 @@ typeof(name(0)) name(struct pt_regs *ctx) \
} \
static __always_inline typeof(name(0)) ____##name(struct pt_regs *ctx, ##args)
-#define ___bpf_fill0(arr, p, x) do {} while (0)
-#define ___bpf_fill1(arr, p, x) arr[p] = x
-#define ___bpf_fill2(arr, p, x, args...) arr[p] = x; ___bpf_fill1(arr, p + 1, args)
-#define ___bpf_fill3(arr, p, x, args...) arr[p] = x; ___bpf_fill2(arr, p + 1, args)
-#define ___bpf_fill4(arr, p, x, args...) arr[p] = x; ___bpf_fill3(arr, p + 1, args)
-#define ___bpf_fill5(arr, p, x, args...) arr[p] = x; ___bpf_fill4(arr, p + 1, args)
-#define ___bpf_fill6(arr, p, x, args...) arr[p] = x; ___bpf_fill5(arr, p + 1, args)
-#define ___bpf_fill7(arr, p, x, args...) arr[p] = x; ___bpf_fill6(arr, p + 1, args)
-#define ___bpf_fill8(arr, p, x, args...) arr[p] = x; ___bpf_fill7(arr, p + 1, args)
-#define ___bpf_fill9(arr, p, x, args...) arr[p] = x; ___bpf_fill8(arr, p + 1, args)
-#define ___bpf_fill10(arr, p, x, args...) arr[p] = x; ___bpf_fill9(arr, p + 1, args)
-#define ___bpf_fill11(arr, p, x, args...) arr[p] = x; ___bpf_fill10(arr, p + 1, args)
-#define ___bpf_fill12(arr, p, x, args...) arr[p] = x; ___bpf_fill11(arr, p + 1, args)
-#define ___bpf_fill(arr, args...) \
- ___bpf_apply(___bpf_fill, ___bpf_narg(args))(arr, 0, args)
-
-/*
- * BPF_SEQ_PRINTF to wrap bpf_seq_printf to-be-printed values
- * in a structure.
- */
-#define BPF_SEQ_PRINTF(seq, fmt, args...) \
-({ \
- static const char ___fmt[] = fmt; \
- unsigned long long ___param[___bpf_narg(args)]; \
- \
- _Pragma("GCC diagnostic push") \
- _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
- ___bpf_fill(___param, args); \
- _Pragma("GCC diagnostic pop") \
- \
- bpf_seq_printf(seq, ___fmt, sizeof(___fmt), \
- ___param, sizeof(___param)); \
-})
-
-/*
- * BPF_SNPRINTF wraps the bpf_snprintf helper with variadic arguments instead of
- * an array of u64.
- */
-#define BPF_SNPRINTF(out, out_size, fmt, args...) \
-({ \
- static const char ___fmt[] = fmt; \
- unsigned long long ___param[___bpf_narg(args)]; \
- \
- _Pragma("GCC diagnostic push") \
- _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
- ___bpf_fill(___param, args); \
- _Pragma("GCC diagnostic pop") \
- \
- bpf_snprintf(out, out_size, ___fmt, \
- ___param, sizeof(___param)); \
-})
-
#endif
diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_bpf_hash_map.c b/tools/testing/selftests/bpf/progs/bpf_iter_bpf_hash_map.c
index 6dfce3fd68bc..0aa3cd34cbe3 100644
--- a/tools/testing/selftests/bpf/progs/bpf_iter_bpf_hash_map.c
+++ b/tools/testing/selftests/bpf/progs/bpf_iter_bpf_hash_map.c
@@ -2,7 +2,6 @@
/* Copyright (c) 2020 Facebook */
#include "bpf_iter.h"
#include <bpf/bpf_helpers.h>
-#include <bpf/bpf_tracing.h>
char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_bpf_map.c b/tools/testing/selftests/bpf/progs/bpf_iter_bpf_map.c
index b83b5d2e17dc..6c39e86b666f 100644
--- a/tools/testing/selftests/bpf/progs/bpf_iter_bpf_map.c
+++ b/tools/testing/selftests/bpf/progs/bpf_iter_bpf_map.c
@@ -2,7 +2,6 @@
/* Copyright (c) 2020 Facebook */
#include "bpf_iter.h"
#include <bpf/bpf_helpers.h>
-#include <bpf/bpf_tracing.h>
char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_ipv6_route.c b/tools/testing/selftests/bpf/progs/bpf_iter_ipv6_route.c
index d58d9f1642b5..784a610ce039 100644
--- a/tools/testing/selftests/bpf/progs/bpf_iter_ipv6_route.c
+++ b/tools/testing/selftests/bpf/progs/bpf_iter_ipv6_route.c
@@ -3,7 +3,6 @@
#include "bpf_iter.h"
#include "bpf_tracing_net.h"
#include <bpf/bpf_helpers.h>
-#include <bpf/bpf_tracing.h>
char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_netlink.c b/tools/testing/selftests/bpf/progs/bpf_iter_netlink.c
index 95989f4c99b5..a28e51e2dcee 100644
--- a/tools/testing/selftests/bpf/progs/bpf_iter_netlink.c
+++ b/tools/testing/selftests/bpf/progs/bpf_iter_netlink.c
@@ -3,7 +3,6 @@
#include "bpf_iter.h"
#include "bpf_tracing_net.h"
#include <bpf/bpf_helpers.h>
-#include <bpf/bpf_tracing.h>
char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_task.c b/tools/testing/selftests/bpf/progs/bpf_iter_task.c
index b7f32c160f4e..c86b93f33b32 100644
--- a/tools/testing/selftests/bpf/progs/bpf_iter_task.c
+++ b/tools/testing/selftests/bpf/progs/bpf_iter_task.c
@@ -2,7 +2,6 @@
/* Copyright (c) 2020 Facebook */
#include "bpf_iter.h"
#include <bpf/bpf_helpers.h>
-#include <bpf/bpf_tracing.h>
char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_task_btf.c b/tools/testing/selftests/bpf/progs/bpf_iter_task_btf.c
index a1ddc36f13ec..bca8b889cb10 100644
--- a/tools/testing/selftests/bpf/progs/bpf_iter_task_btf.c
+++ b/tools/testing/selftests/bpf/progs/bpf_iter_task_btf.c
@@ -2,7 +2,6 @@
/* Copyright (c) 2020, Oracle and/or its affiliates. */
#include "bpf_iter.h"
#include <bpf/bpf_helpers.h>
-#include <bpf/bpf_tracing.h>
#include <bpf/bpf_core_read.h>
#include <errno.h>
diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_task_file.c b/tools/testing/selftests/bpf/progs/bpf_iter_task_file.c
index b2f7c7c5f952..6e7b400888fe 100644
--- a/tools/testing/selftests/bpf/progs/bpf_iter_task_file.c
+++ b/tools/testing/selftests/bpf/progs/bpf_iter_task_file.c
@@ -2,7 +2,6 @@
/* Copyright (c) 2020 Facebook */
#include "bpf_iter.h"
#include <bpf/bpf_helpers.h>
-#include <bpf/bpf_tracing.h>
char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_task_stack.c b/tools/testing/selftests/bpf/progs/bpf_iter_task_stack.c
index 43c36f5f7649..f2b8167b72a8 100644
--- a/tools/testing/selftests/bpf/progs/bpf_iter_task_stack.c
+++ b/tools/testing/selftests/bpf/progs/bpf_iter_task_stack.c
@@ -2,7 +2,6 @@
/* Copyright (c) 2020 Facebook */
#include "bpf_iter.h"
#include <bpf/bpf_helpers.h>
-#include <bpf/bpf_tracing.h>
char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_task_vma.c b/tools/testing/selftests/bpf/progs/bpf_iter_task_vma.c
index 11d1aa37cf11..4ea6a37d1345 100644
--- a/tools/testing/selftests/bpf/progs/bpf_iter_task_vma.c
+++ b/tools/testing/selftests/bpf/progs/bpf_iter_task_vma.c
@@ -2,7 +2,6 @@
/* Copyright (c) 2020 Facebook */
#include "bpf_iter.h"
#include <bpf/bpf_helpers.h>
-#include <bpf/bpf_tracing.h>
char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_tcp4.c b/tools/testing/selftests/bpf/progs/bpf_iter_tcp4.c
index 54380c5e1069..2e4775c35414 100644
--- a/tools/testing/selftests/bpf/progs/bpf_iter_tcp4.c
+++ b/tools/testing/selftests/bpf/progs/bpf_iter_tcp4.c
@@ -3,7 +3,6 @@
#include "bpf_iter.h"
#include "bpf_tracing_net.h"
#include <bpf/bpf_helpers.h>
-#include <bpf/bpf_tracing.h>
#include <bpf/bpf_endian.h>
char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_tcp6.c b/tools/testing/selftests/bpf/progs/bpf_iter_tcp6.c
index b4fbddfa4e10..943f7bba180e 100644
--- a/tools/testing/selftests/bpf/progs/bpf_iter_tcp6.c
+++ b/tools/testing/selftests/bpf/progs/bpf_iter_tcp6.c
@@ -3,7 +3,6 @@
#include "bpf_iter.h"
#include "bpf_tracing_net.h"
#include <bpf/bpf_helpers.h>
-#include <bpf/bpf_tracing.h>
#include <bpf/bpf_endian.h>
char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_udp4.c b/tools/testing/selftests/bpf/progs/bpf_iter_udp4.c
index f258583afbbd..cf0c485b1ed7 100644
--- a/tools/testing/selftests/bpf/progs/bpf_iter_udp4.c
+++ b/tools/testing/selftests/bpf/progs/bpf_iter_udp4.c
@@ -3,7 +3,6 @@
#include "bpf_iter.h"
#include "bpf_tracing_net.h"
#include <bpf/bpf_helpers.h>
-#include <bpf/bpf_tracing.h>
#include <bpf/bpf_endian.h>
char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_udp6.c b/tools/testing/selftests/bpf/progs/bpf_iter_udp6.c
index 65f93bb03f0f..5031e21c433f 100644
--- a/tools/testing/selftests/bpf/progs/bpf_iter_udp6.c
+++ b/tools/testing/selftests/bpf/progs/bpf_iter_udp6.c
@@ -3,7 +3,6 @@
#include "bpf_iter.h"
#include "bpf_tracing_net.h"
#include <bpf/bpf_helpers.h>
-#include <bpf/bpf_tracing.h>
#include <bpf/bpf_endian.h>
char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/test_snprintf.c b/tools/testing/selftests/bpf/progs/test_snprintf.c
index e35129bea0a0..e2ad26150f9b 100644
--- a/tools/testing/selftests/bpf/progs/test_snprintf.c
+++ b/tools/testing/selftests/bpf/progs/test_snprintf.c
@@ -3,7 +3,6 @@
#include <linux/bpf.h>
#include <bpf/bpf_helpers.h>
-#include <bpf/bpf_tracing.h>
__u32 pid = 0;
--
2.31.1.818.g46aad6cb9e-goog
On 5/25/21 10:18 PM, Florent Revest wrote:
> These macros are convenient wrappers around the bpf_seq_printf and
> bpf_snprintf helpers. They are currently provided by bpf_tracing.h which
> targets low level tracing primitives. bpf_helpers.h is a better fit.
>
> The __bpf_narg and __bpf_apply macros are needed in both files so
> provided twice and guarded by ifndefs.
>
> Reported-by: Andrii Nakryiko <[email protected]>
> Signed-off-by: Florent Revest <[email protected]>
Given v1/v2 both target bpf tree in the subject, do you really mean bpf or
rather bpf-next?
Thanks,
Daniel
On Wed, May 26, 2021 at 8:35 AM Daniel Borkmann <[email protected]> wrote:
>
> On 5/25/21 10:18 PM, Florent Revest wrote:
> > These macros are convenient wrappers around the bpf_seq_printf and
> > bpf_snprintf helpers. They are currently provided by bpf_tracing.h which
> > targets low level tracing primitives. bpf_helpers.h is a better fit.
> >
> > The __bpf_narg and __bpf_apply macros are needed in both files so
> > provided twice and guarded by ifndefs.
> >
> > Reported-by: Andrii Nakryiko <[email protected]>
> > Signed-off-by: Florent Revest <[email protected]>
>
> Given v1/v2 both target bpf tree in the subject, do you really mean bpf or
> rather bpf-next?
I don't have a preference, it's up to you :)
On one hand, I see no urgency in fixing this: BPF_SEQ_PRINTF has been
in bpf_tracing.h for a while already so it can wait for another kernel
release. Applying this to bpf-next would do.
On the other hand, BPF_SNPRINTF hasn't made it to a kernel release yet
so we still have a chance to do it right before users start including
bpf_tracing.h and we'd break them in the next release. That's why I
tagged it as bpf.
The patch applies cleanly on both trees so if you prefer landing it in
bpf-next it's fine by me.
On Wed, May 26, 2021 at 8:01 AM Florent Revest <[email protected]> wrote:
>
> On Wed, May 26, 2021 at 8:35 AM Daniel Borkmann <[email protected]> wrote:
> >
> > On 5/25/21 10:18 PM, Florent Revest wrote:
> > > These macros are convenient wrappers around the bpf_seq_printf and
> > > bpf_snprintf helpers. They are currently provided by bpf_tracing.h which
> > > targets low level tracing primitives. bpf_helpers.h is a better fit.
> > >
> > > The __bpf_narg and __bpf_apply macros are needed in both files so
> > > provided twice and guarded by ifndefs.
> > >
> > > Reported-by: Andrii Nakryiko <[email protected]>
> > > Signed-off-by: Florent Revest <[email protected]>
> >
> > Given v1/v2 both target bpf tree in the subject, do you really mean bpf or
> > rather bpf-next?
>
> I don't have a preference, it's up to you :)
>
> On one hand, I see no urgency in fixing this: BPF_SEQ_PRINTF has been
> in bpf_tracing.h for a while already so it can wait for another kernel
> release. Applying this to bpf-next would do.
> On the other hand, BPF_SNPRINTF hasn't made it to a kernel release yet
> so we still have a chance to do it right before users start including
> bpf_tracing.h and we'd break them in the next release. That's why I
> tagged it as bpf.
>
> The patch applies cleanly on both trees so if you prefer landing it in
> bpf-next it's fine by me.
I think it should go through bpf-next. It's not really a bug fix. And
we are not going to break anyone with this move. And libbpf 0.4 is
officially released without this change anyway. So, bpf-next.
On Tue, May 25, 2021 at 1:18 PM Florent Revest <[email protected]> wrote:
>
> These macros are convenient wrappers around the bpf_seq_printf and
> bpf_snprintf helpers. They are currently provided by bpf_tracing.h which
> targets low level tracing primitives. bpf_helpers.h is a better fit.
>
> The __bpf_narg and __bpf_apply macros are needed in both files so
> provided twice and guarded by ifndefs.
>
> Reported-by: Andrii Nakryiko <[email protected]>
> Signed-off-by: Florent Revest <[email protected]>
> ---
> kernel/bpf/preload/iterators/iterators.bpf.c | 1 -
> tools/lib/bpf/bpf_helpers.h | 70 +++++++++++++++++++
> tools/lib/bpf/bpf_tracing.h | 62 +++-------------
> .../bpf/progs/bpf_iter_bpf_hash_map.c | 1 -
> .../selftests/bpf/progs/bpf_iter_bpf_map.c | 1 -
> .../selftests/bpf/progs/bpf_iter_ipv6_route.c | 1 -
> .../selftests/bpf/progs/bpf_iter_netlink.c | 1 -
> .../selftests/bpf/progs/bpf_iter_task.c | 1 -
> .../selftests/bpf/progs/bpf_iter_task_btf.c | 1 -
> .../selftests/bpf/progs/bpf_iter_task_file.c | 1 -
> .../selftests/bpf/progs/bpf_iter_task_stack.c | 1 -
> .../selftests/bpf/progs/bpf_iter_task_vma.c | 1 -
> .../selftests/bpf/progs/bpf_iter_tcp4.c | 1 -
> .../selftests/bpf/progs/bpf_iter_tcp6.c | 1 -
> .../selftests/bpf/progs/bpf_iter_udp4.c | 1 -
> .../selftests/bpf/progs/bpf_iter_udp6.c | 1 -
> .../selftests/bpf/progs/test_snprintf.c | 1 -
> 17 files changed, 80 insertions(+), 67 deletions(-)
>
> diff --git a/kernel/bpf/preload/iterators/iterators.bpf.c b/kernel/bpf/preload/iterators/iterators.bpf.c
> index 52aa7b38e8b8..03af863314ea 100644
> --- a/kernel/bpf/preload/iterators/iterators.bpf.c
> +++ b/kernel/bpf/preload/iterators/iterators.bpf.c
> @@ -2,7 +2,6 @@
> /* Copyright (c) 2020 Facebook */
> #include <linux/bpf.h>
> #include <bpf/bpf_helpers.h>
> -#include <bpf/bpf_tracing.h>
> #include <bpf/bpf_core_read.h>
>
> #pragma clang attribute push (__attribute__((preserve_access_index)), apply_to = record)
> diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
> index 9720dc0b4605..68d992b30f26 100644
> --- a/tools/lib/bpf/bpf_helpers.h
> +++ b/tools/lib/bpf/bpf_helpers.h
> @@ -158,4 +158,74 @@ enum libbpf_tristate {
> #define __kconfig __attribute__((section(".kconfig")))
> #define __ksym __attribute__((section(".ksyms")))
>
> +#ifndef ___bpf_concat
> +#define ___bpf_concat(a, b) a ## b
> +#endif
> +#ifndef ___bpf_apply
> +#define ___bpf_apply(fn, n) ___bpf_concat(fn, n)
> +#endif
> +#ifndef ___bpf_nth
> +#define ___bpf_nth(_, _1, _2, _3, _4, _5, _6, _7, _8, _9, _a, _b, _c, N, ...) N
> +#endif
> +#ifndef ___bpf_narg
> +#define ___bpf_narg(...) \
> + ___bpf_nth(_, ##__VA_ARGS__, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0)
> +#endif
> +#ifndef ___bpf_empty
> +#define ___bpf_empty(...) \
> + ___bpf_nth(_, ##__VA_ARGS__, N, N, N, N, N, N, N, N, N, N, 0)
> +#endif
___bpf_empty doesn't seem to be used, let's remove it for now?
Otherwise it looks good.
> +
> +#define ___bpf_fill0(arr, p, x) do {} while (0)
> +#define ___bpf_fill1(arr, p, x) arr[p] = x
> +#define ___bpf_fill2(arr, p, x, args...) arr[p] = x; ___bpf_fill1(arr, p + 1, args)
> +#define ___bpf_fill3(arr, p, x, args...) arr[p] = x; ___bpf_fill2(arr, p + 1, args)
> +#define ___bpf_fill4(arr, p, x, args...) arr[p] = x; ___bpf_fill3(arr, p + 1, args)
> +#define ___bpf_fill5(arr, p, x, args...) arr[p] = x; ___bpf_fill4(arr, p + 1, args)
> +#define ___bpf_fill6(arr, p, x, args...) arr[p] = x; ___bpf_fill5(arr, p + 1, args)
> +#define ___bpf_fill7(arr, p, x, args...) arr[p] = x; ___bpf_fill6(arr, p + 1, args)
> +#define ___bpf_fill8(arr, p, x, args...) arr[p] = x; ___bpf_fill7(arr, p + 1, args)
> +#define ___bpf_fill9(arr, p, x, args...) arr[p] = x; ___bpf_fill8(arr, p + 1, args)
> +#define ___bpf_fill10(arr, p, x, args...) arr[p] = x; ___bpf_fill9(arr, p + 1, args)
> +#define ___bpf_fill11(arr, p, x, args...) arr[p] = x; ___bpf_fill10(arr, p + 1, args)
> +#define ___bpf_fill12(arr, p, x, args...) arr[p] = x; ___bpf_fill11(arr, p + 1, args)
> +#define ___bpf_fill(arr, args...) \
> + ___bpf_apply(___bpf_fill, ___bpf_narg(args))(arr, 0, args)
> +
[...]
On Wed, May 26, 2021 at 9:36 AM Florent Revest <[email protected]> wrote:
>
> On Wed, May 26, 2021 at 6:34 PM Andrii Nakryiko
> <[email protected]> wrote:
> >
> > On Tue, May 25, 2021 at 1:18 PM Florent Revest <[email protected]> wrote:
> > >
> > > These macros are convenient wrappers around the bpf_seq_printf and
> > > bpf_snprintf helpers. They are currently provided by bpf_tracing.h which
> > > targets low level tracing primitives. bpf_helpers.h is a better fit.
> > >
> > > The __bpf_narg and __bpf_apply macros are needed in both files so
> > > provided twice and guarded by ifndefs.
> > >
> > > Reported-by: Andrii Nakryiko <[email protected]>
> > > Signed-off-by: Florent Revest <[email protected]>
> > > ---
> > > kernel/bpf/preload/iterators/iterators.bpf.c | 1 -
> > > tools/lib/bpf/bpf_helpers.h | 70 +++++++++++++++++++
> > > tools/lib/bpf/bpf_tracing.h | 62 +++-------------
> > > .../bpf/progs/bpf_iter_bpf_hash_map.c | 1 -
> > > .../selftests/bpf/progs/bpf_iter_bpf_map.c | 1 -
> > > .../selftests/bpf/progs/bpf_iter_ipv6_route.c | 1 -
> > > .../selftests/bpf/progs/bpf_iter_netlink.c | 1 -
> > > .../selftests/bpf/progs/bpf_iter_task.c | 1 -
> > > .../selftests/bpf/progs/bpf_iter_task_btf.c | 1 -
> > > .../selftests/bpf/progs/bpf_iter_task_file.c | 1 -
> > > .../selftests/bpf/progs/bpf_iter_task_stack.c | 1 -
> > > .../selftests/bpf/progs/bpf_iter_task_vma.c | 1 -
> > > .../selftests/bpf/progs/bpf_iter_tcp4.c | 1 -
> > > .../selftests/bpf/progs/bpf_iter_tcp6.c | 1 -
> > > .../selftests/bpf/progs/bpf_iter_udp4.c | 1 -
> > > .../selftests/bpf/progs/bpf_iter_udp6.c | 1 -
> > > .../selftests/bpf/progs/test_snprintf.c | 1 -
> > > 17 files changed, 80 insertions(+), 67 deletions(-)
> > >
> > > diff --git a/kernel/bpf/preload/iterators/iterators.bpf.c b/kernel/bpf/preload/iterators/iterators.bpf.c
> > > index 52aa7b38e8b8..03af863314ea 100644
> > > --- a/kernel/bpf/preload/iterators/iterators.bpf.c
> > > +++ b/kernel/bpf/preload/iterators/iterators.bpf.c
> > > @@ -2,7 +2,6 @@
> > > /* Copyright (c) 2020 Facebook */
> > > #include <linux/bpf.h>
> > > #include <bpf/bpf_helpers.h>
> > > -#include <bpf/bpf_tracing.h>
> > > #include <bpf/bpf_core_read.h>
> > >
> > > #pragma clang attribute push (__attribute__((preserve_access_index)), apply_to = record)
> > > diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
> > > index 9720dc0b4605..68d992b30f26 100644
> > > --- a/tools/lib/bpf/bpf_helpers.h
> > > +++ b/tools/lib/bpf/bpf_helpers.h
> > > @@ -158,4 +158,74 @@ enum libbpf_tristate {
> > > #define __kconfig __attribute__((section(".kconfig")))
> > > #define __ksym __attribute__((section(".ksyms")))
> > >
> > > +#ifndef ___bpf_concat
> > > +#define ___bpf_concat(a, b) a ## b
> > > +#endif
> > > +#ifndef ___bpf_apply
> > > +#define ___bpf_apply(fn, n) ___bpf_concat(fn, n)
> > > +#endif
> > > +#ifndef ___bpf_nth
> > > +#define ___bpf_nth(_, _1, _2, _3, _4, _5, _6, _7, _8, _9, _a, _b, _c, N, ...) N
> > > +#endif
> > > +#ifndef ___bpf_narg
> > > +#define ___bpf_narg(...) \
> > > + ___bpf_nth(_, ##__VA_ARGS__, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0)
> > > +#endif
> > > +#ifndef ___bpf_empty
> > > +#define ___bpf_empty(...) \
> > > + ___bpf_nth(_, ##__VA_ARGS__, N, N, N, N, N, N, N, N, N, N, 0)
> > > +#endif
> >
> > ___bpf_empty doesn't seem to be used, let's remove it for now?
> > Otherwise it looks good.
>
> Yes it's never been used, I thought it was introduced "just in case"
> so I kept it around but then I'll remove it from both bpf_helpers.h
> and bpf_tracing.h
Oh, I didn't realize it's not used in bpf_tracing.h as well. I think
it was used at some point, but probably some subsequent macro
refactorings got rid of it.
On Wed, May 26, 2021 at 6:35 PM Andrii Nakryiko
<[email protected]> wrote:
>
> On Wed, May 26, 2021 at 8:01 AM Florent Revest <[email protected]> wrote:
> >
> > On Wed, May 26, 2021 at 8:35 AM Daniel Borkmann <[email protected]> wrote:
> > >
> > > On 5/25/21 10:18 PM, Florent Revest wrote:
> > > > These macros are convenient wrappers around the bpf_seq_printf and
> > > > bpf_snprintf helpers. They are currently provided by bpf_tracing.h which
> > > > targets low level tracing primitives. bpf_helpers.h is a better fit.
> > > >
> > > > The __bpf_narg and __bpf_apply macros are needed in both files so
> > > > provided twice and guarded by ifndefs.
> > > >
> > > > Reported-by: Andrii Nakryiko <[email protected]>
> > > > Signed-off-by: Florent Revest <[email protected]>
> > >
> > > Given v1/v2 both target bpf tree in the subject, do you really mean bpf or
> > > rather bpf-next?
> >
> > I don't have a preference, it's up to you :)
> >
> > On one hand, I see no urgency in fixing this: BPF_SEQ_PRINTF has been
> > in bpf_tracing.h for a while already so it can wait for another kernel
> > release. Applying this to bpf-next would do.
> > On the other hand, BPF_SNPRINTF hasn't made it to a kernel release yet
> > so we still have a chance to do it right before users start including
> > bpf_tracing.h and we'd break them in the next release. That's why I
> > tagged it as bpf.
> >
> > The patch applies cleanly on both trees so if you prefer landing it in
> > bpf-next it's fine by me.
>
> I think it should go through bpf-next. It's not really a bug fix. And
> we are not going to break anyone with this move. And libbpf 0.4 is
> officially released without this change anyway. So, bpf-next.
Sounds good, I'll send a v3 tagged with bpf-next ;)
On Wed, May 26, 2021 at 6:34 PM Andrii Nakryiko
<[email protected]> wrote:
>
> On Tue, May 25, 2021 at 1:18 PM Florent Revest <[email protected]> wrote:
> >
> > These macros are convenient wrappers around the bpf_seq_printf and
> > bpf_snprintf helpers. They are currently provided by bpf_tracing.h which
> > targets low level tracing primitives. bpf_helpers.h is a better fit.
> >
> > The __bpf_narg and __bpf_apply macros are needed in both files so
> > provided twice and guarded by ifndefs.
> >
> > Reported-by: Andrii Nakryiko <[email protected]>
> > Signed-off-by: Florent Revest <[email protected]>
> > ---
> > kernel/bpf/preload/iterators/iterators.bpf.c | 1 -
> > tools/lib/bpf/bpf_helpers.h | 70 +++++++++++++++++++
> > tools/lib/bpf/bpf_tracing.h | 62 +++-------------
> > .../bpf/progs/bpf_iter_bpf_hash_map.c | 1 -
> > .../selftests/bpf/progs/bpf_iter_bpf_map.c | 1 -
> > .../selftests/bpf/progs/bpf_iter_ipv6_route.c | 1 -
> > .../selftests/bpf/progs/bpf_iter_netlink.c | 1 -
> > .../selftests/bpf/progs/bpf_iter_task.c | 1 -
> > .../selftests/bpf/progs/bpf_iter_task_btf.c | 1 -
> > .../selftests/bpf/progs/bpf_iter_task_file.c | 1 -
> > .../selftests/bpf/progs/bpf_iter_task_stack.c | 1 -
> > .../selftests/bpf/progs/bpf_iter_task_vma.c | 1 -
> > .../selftests/bpf/progs/bpf_iter_tcp4.c | 1 -
> > .../selftests/bpf/progs/bpf_iter_tcp6.c | 1 -
> > .../selftests/bpf/progs/bpf_iter_udp4.c | 1 -
> > .../selftests/bpf/progs/bpf_iter_udp6.c | 1 -
> > .../selftests/bpf/progs/test_snprintf.c | 1 -
> > 17 files changed, 80 insertions(+), 67 deletions(-)
> >
> > diff --git a/kernel/bpf/preload/iterators/iterators.bpf.c b/kernel/bpf/preload/iterators/iterators.bpf.c
> > index 52aa7b38e8b8..03af863314ea 100644
> > --- a/kernel/bpf/preload/iterators/iterators.bpf.c
> > +++ b/kernel/bpf/preload/iterators/iterators.bpf.c
> > @@ -2,7 +2,6 @@
> > /* Copyright (c) 2020 Facebook */
> > #include <linux/bpf.h>
> > #include <bpf/bpf_helpers.h>
> > -#include <bpf/bpf_tracing.h>
> > #include <bpf/bpf_core_read.h>
> >
> > #pragma clang attribute push (__attribute__((preserve_access_index)), apply_to = record)
> > diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
> > index 9720dc0b4605..68d992b30f26 100644
> > --- a/tools/lib/bpf/bpf_helpers.h
> > +++ b/tools/lib/bpf/bpf_helpers.h
> > @@ -158,4 +158,74 @@ enum libbpf_tristate {
> > #define __kconfig __attribute__((section(".kconfig")))
> > #define __ksym __attribute__((section(".ksyms")))
> >
> > +#ifndef ___bpf_concat
> > +#define ___bpf_concat(a, b) a ## b
> > +#endif
> > +#ifndef ___bpf_apply
> > +#define ___bpf_apply(fn, n) ___bpf_concat(fn, n)
> > +#endif
> > +#ifndef ___bpf_nth
> > +#define ___bpf_nth(_, _1, _2, _3, _4, _5, _6, _7, _8, _9, _a, _b, _c, N, ...) N
> > +#endif
> > +#ifndef ___bpf_narg
> > +#define ___bpf_narg(...) \
> > + ___bpf_nth(_, ##__VA_ARGS__, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0)
> > +#endif
> > +#ifndef ___bpf_empty
> > +#define ___bpf_empty(...) \
> > + ___bpf_nth(_, ##__VA_ARGS__, N, N, N, N, N, N, N, N, N, N, 0)
> > +#endif
>
> ___bpf_empty doesn't seem to be used, let's remove it for now?
> Otherwise it looks good.
Yes it's never been used, I thought it was introduced "just in case"
so I kept it around but then I'll remove it from both bpf_helpers.h
and bpf_tracing.h