* [PATCH v6 bpf-next 1/9] bpf: merge printk and seq_printf VARARG max macros
2021-09-17 18:29 [PATCH v6 bpf-next 0/9] bpf: implement variadic printk helper Dave Marchevsky
@ 2021-09-17 18:29 ` Dave Marchevsky
2021-09-17 18:29 ` [PATCH v6 bpf-next 2/9] selftests/bpf: stop using bpf_program__load Dave Marchevsky
` (8 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Dave Marchevsky @ 2021-09-17 18:29 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Yonghong Song, netdev, Dave Marchevsky
MAX_SNPRINTF_VARARGS and MAX_SEQ_PRINTF_VARARGS are used by bpf helpers
bpf_snprintf and bpf_seq_printf to limit their varargs. Both call into
bpf_bprintf_prepare for print formatting logic and have convenience
macros in libbpf (BPF_SNPRINTF, BPF_SEQ_PRINTF) which use the same
helper macros to convert varargs to a byte array.
Changing shared functionality to support more varargs for either bpf
helper would affect the other as well, so let's combine the _VARARGS
macros to make this more obvious.
Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
---
include/linux/bpf.h | 2 ++
kernel/bpf/helpers.c | 4 +---
kernel/trace/bpf_trace.c | 4 +---
3 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index f4c16f19f83e..be8d57e6e78a 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -2216,6 +2216,8 @@ int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type t,
struct btf_id_set;
bool btf_id_set_contains(const struct btf_id_set *set, u32 id);
+#define MAX_BPRINTF_VARARGS 12
+
int bpf_bprintf_prepare(char *fmt, u32 fmt_size, const u64 *raw_args,
u32 **bin_buf, u32 num_args);
void bpf_bprintf_cleanup(void);
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 9aabf84afd4b..8f9f392c1322 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -979,15 +979,13 @@ int bpf_bprintf_prepare(char *fmt, u32 fmt_size, const u64 *raw_args,
return err;
}
-#define MAX_SNPRINTF_VARARGS 12
-
BPF_CALL_5(bpf_snprintf, char *, str, u32, str_size, char *, fmt,
const void *, data, u32, data_len)
{
int err, num_args;
u32 *bin_args;
- if (data_len % 8 || data_len > MAX_SNPRINTF_VARARGS * 8 ||
+ if (data_len % 8 || data_len > MAX_BPRINTF_VARARGS * 8 ||
(data_len && !data))
return -EINVAL;
num_args = data_len / 8;
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 067e88c3d2ee..4ec779fa0c1d 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -414,15 +414,13 @@ const struct bpf_func_proto *bpf_get_trace_printk_proto(void)
return &bpf_trace_printk_proto;
}
-#define MAX_SEQ_PRINTF_VARARGS 12
-
BPF_CALL_5(bpf_seq_printf, struct seq_file *, m, char *, fmt, u32, fmt_size,
const void *, data, u32, data_len)
{
int err, num_args;
u32 *bin_args;
- if (data_len & 7 || data_len > MAX_SEQ_PRINTF_VARARGS * 8 ||
+ if (data_len & 7 || data_len > MAX_BPRINTF_VARARGS * 8 ||
(data_len && !data))
return -EINVAL;
num_args = data_len / 8;
--
2.30.2
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v6 bpf-next 2/9] selftests/bpf: stop using bpf_program__load
2021-09-17 18:29 [PATCH v6 bpf-next 0/9] bpf: implement variadic printk helper Dave Marchevsky
2021-09-17 18:29 ` [PATCH v6 bpf-next 1/9] bpf: merge printk and seq_printf VARARG max macros Dave Marchevsky
@ 2021-09-17 18:29 ` Dave Marchevsky
2021-09-17 18:29 ` [PATCH v6 bpf-next 3/9] bpf: add bpf_trace_vprintk helper Dave Marchevsky
` (7 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Dave Marchevsky @ 2021-09-17 18:29 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Yonghong Song, netdev, Dave Marchevsky
bpf_program__load is not supposed to be used directly. Replace it with
bpf_object__ APIs for the reference_tracking prog_test, which is the
last offender in bpf selftests.
Some additional complexity is added for this test, namely the use of one
bpf_object to iterate through progs, while a second bpf_object is
created and opened/closed to test actual loading of progs. This is
because the test was doing bpf_program__load then __unload to test
loading of individual progs and same semantics with
bpf_object__load/__unload result in failure to load an __unload-ed obj.
Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
---
.../bpf/prog_tests/reference_tracking.c | 39 +++++++++++++++----
1 file changed, 31 insertions(+), 8 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/reference_tracking.c b/tools/testing/selftests/bpf/prog_tests/reference_tracking.c
index 4e91f4d6466c..ded2dc8ddd79 100644
--- a/tools/testing/selftests/bpf/prog_tests/reference_tracking.c
+++ b/tools/testing/selftests/bpf/prog_tests/reference_tracking.c
@@ -1,6 +1,21 @@
// SPDX-License-Identifier: GPL-2.0
#include <test_progs.h>
+static void toggle_object_autoload_progs(const struct bpf_object *obj,
+ const char *title_load)
+{
+ struct bpf_program *prog;
+
+ bpf_object__for_each_program(prog, obj) {
+ const char *title = bpf_program__section_name(prog);
+
+ if (!strcmp(title_load, title))
+ bpf_program__set_autoload(prog, true);
+ else
+ bpf_program__set_autoload(prog, false);
+ }
+}
+
void test_reference_tracking(void)
{
const char *file = "test_sk_lookup_kern.o";
@@ -9,21 +24,21 @@ void test_reference_tracking(void)
.object_name = obj_name,
.relaxed_maps = true,
);
- struct bpf_object *obj;
+ struct bpf_object *obj_iter, *obj = NULL;
struct bpf_program *prog;
__u32 duration = 0;
int err = 0;
- obj = bpf_object__open_file(file, &open_opts);
- if (!ASSERT_OK_PTR(obj, "obj_open_file"))
+ obj_iter = bpf_object__open_file(file, &open_opts);
+ if (!ASSERT_OK_PTR(obj_iter, "obj_iter_open_file"))
return;
- if (CHECK(strcmp(bpf_object__name(obj), obj_name), "obj_name",
+ if (CHECK(strcmp(bpf_object__name(obj_iter), obj_name), "obj_name",
"wrong obj name '%s', expected '%s'\n",
- bpf_object__name(obj), obj_name))
+ bpf_object__name(obj_iter), obj_name))
goto cleanup;
- bpf_object__for_each_program(prog, obj) {
+ bpf_object__for_each_program(prog, obj_iter) {
const char *title;
/* Ignore .text sections */
@@ -34,19 +49,27 @@ void test_reference_tracking(void)
if (!test__start_subtest(title))
continue;
+ obj = bpf_object__open_file(file, &open_opts);
+ if (!ASSERT_OK_PTR(obj, "obj_open_file"))
+ goto cleanup;
+
+ toggle_object_autoload_progs(obj, title);
/* Expect verifier failure if test name has 'err' */
if (strstr(title, "err_") != NULL) {
libbpf_print_fn_t old_print_fn;
old_print_fn = libbpf_set_print(NULL);
- err = !bpf_program__load(prog, "GPL", 0);
+ err = !bpf_object__load(obj);
libbpf_set_print(old_print_fn);
} else {
- err = bpf_program__load(prog, "GPL", 0);
+ err = bpf_object__load(obj);
}
CHECK(err, title, "\n");
+ bpf_object__close(obj);
+ obj = NULL;
}
cleanup:
bpf_object__close(obj);
+ bpf_object__close(obj_iter);
}
--
2.30.2
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v6 bpf-next 3/9] bpf: add bpf_trace_vprintk helper
2021-09-17 18:29 [PATCH v6 bpf-next 0/9] bpf: implement variadic printk helper Dave Marchevsky
2021-09-17 18:29 ` [PATCH v6 bpf-next 1/9] bpf: merge printk and seq_printf VARARG max macros Dave Marchevsky
2021-09-17 18:29 ` [PATCH v6 bpf-next 2/9] selftests/bpf: stop using bpf_program__load Dave Marchevsky
@ 2021-09-17 18:29 ` Dave Marchevsky
2021-09-17 18:29 ` [PATCH v6 bpf-next 4/9] libbpf: Modify bpf_printk to choose helper based on arg count Dave Marchevsky
` (6 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Dave Marchevsky @ 2021-09-17 18:29 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Yonghong Song, netdev, Dave Marchevsky
This helper is meant to be "bpf_trace_printk, but with proper vararg
support". Follow bpf_snprintf's example and take a u64 pseudo-vararg
array. Write to /sys/kernel/debug/tracing/trace_pipe using the same
mechanism as bpf_trace_printk. The functionality of this helper was
requested in the libbpf issue tracker [0].
[0] Closes: https://github.com/libbpf/libbpf/issues/315
Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
---
include/linux/bpf.h | 1 +
include/uapi/linux/bpf.h | 11 +++++++
kernel/bpf/core.c | 5 ++++
kernel/bpf/helpers.c | 2 ++
kernel/trace/bpf_trace.c | 52 +++++++++++++++++++++++++++++++++-
tools/include/uapi/linux/bpf.h | 11 +++++++
6 files changed, 81 insertions(+), 1 deletion(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index be8d57e6e78a..b6c45a6cbbba 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1088,6 +1088,7 @@ bool bpf_prog_array_compatible(struct bpf_array *array, const struct bpf_prog *f
int bpf_prog_calc_tag(struct bpf_prog *fp);
const struct bpf_func_proto *bpf_get_trace_printk_proto(void);
+const struct bpf_func_proto *bpf_get_trace_vprintk_proto(void);
typedef unsigned long (*bpf_ctx_copy_t)(void *dst, const void *src,
unsigned long off, unsigned long len);
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 3e9785f1064a..98ca79a67937 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -4898,6 +4898,16 @@ union bpf_attr {
* **-EINVAL** if *flags* is not zero.
*
* **-ENOENT** if architecture does not support branch records.
+ *
+ * long bpf_trace_vprintk(const char *fmt, u32 fmt_size, const void *data, u32 data_len)
+ * Description
+ * Behaves like **bpf_trace_printk**\ () helper, but takes an array of u64
+ * to format and can handle more format args as a result.
+ *
+ * Arguments are to be used as in **bpf_seq_printf**\ () helper.
+ * Return
+ * The number of bytes written to the buffer, or a negative error
+ * in case of failure.
*/
#define __BPF_FUNC_MAPPER(FN) \
FN(unspec), \
@@ -5077,6 +5087,7 @@ union bpf_attr {
FN(get_attach_cookie), \
FN(task_pt_regs), \
FN(get_branch_snapshot), \
+ FN(trace_vprintk), \
/* */
/* integer value in 'imm' field of BPF_CALL instruction selects which helper
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 9f4636d021b1..6fddc13fe67f 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -2357,6 +2357,11 @@ const struct bpf_func_proto * __weak bpf_get_trace_printk_proto(void)
return NULL;
}
+const struct bpf_func_proto * __weak bpf_get_trace_vprintk_proto(void)
+{
+ return NULL;
+}
+
u64 __weak
bpf_event_output(struct bpf_map *map, u64 flags, void *meta, u64 meta_size,
void *ctx, u64 ctx_size, bpf_ctx_copy_t ctx_copy)
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 8f9f392c1322..2c604ff8c7fb 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -1435,6 +1435,8 @@ bpf_base_func_proto(enum bpf_func_id func_id)
return &bpf_snprintf_proto;
case BPF_FUNC_task_pt_regs:
return &bpf_task_pt_regs_proto;
+ case BPF_FUNC_trace_vprintk:
+ return bpf_get_trace_vprintk_proto();
default:
return NULL;
}
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 4ec779fa0c1d..6b3153841a33 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -398,7 +398,7 @@ static const struct bpf_func_proto bpf_trace_printk_proto = {
.arg2_type = ARG_CONST_SIZE,
};
-const struct bpf_func_proto *bpf_get_trace_printk_proto(void)
+static void __set_printk_clr_event(void)
{
/*
* This program might be calling bpf_trace_printk,
@@ -410,10 +410,58 @@ const struct bpf_func_proto *bpf_get_trace_printk_proto(void)
*/
if (trace_set_clr_event("bpf_trace", "bpf_trace_printk", 1))
pr_warn_ratelimited("could not enable bpf_trace_printk events");
+}
+const struct bpf_func_proto *bpf_get_trace_printk_proto(void)
+{
+ __set_printk_clr_event();
return &bpf_trace_printk_proto;
}
+BPF_CALL_4(bpf_trace_vprintk, char *, fmt, u32, fmt_size, const void *, data,
+ u32, data_len)
+{
+ static char buf[BPF_TRACE_PRINTK_SIZE];
+ unsigned long flags;
+ int ret, num_args;
+ u32 *bin_args;
+
+ if (data_len & 7 || data_len > MAX_BPRINTF_VARARGS * 8 ||
+ (data_len && !data))
+ return -EINVAL;
+ num_args = data_len / 8;
+
+ ret = bpf_bprintf_prepare(fmt, fmt_size, data, &bin_args, num_args);
+ if (ret < 0)
+ return ret;
+
+ raw_spin_lock_irqsave(&trace_printk_lock, flags);
+ ret = bstr_printf(buf, sizeof(buf), fmt, bin_args);
+
+ trace_bpf_trace_printk(buf);
+ raw_spin_unlock_irqrestore(&trace_printk_lock, flags);
+
+ bpf_bprintf_cleanup();
+
+ return ret;
+}
+
+static const struct bpf_func_proto bpf_trace_vprintk_proto = {
+ .func = bpf_trace_vprintk,
+ .gpl_only = true,
+ .ret_type = RET_INTEGER,
+ .arg1_type = ARG_PTR_TO_MEM,
+ .arg2_type = ARG_CONST_SIZE,
+ .arg3_type = ARG_PTR_TO_MEM_OR_NULL,
+ .arg4_type = ARG_CONST_SIZE_OR_ZERO,
+};
+
+const struct bpf_func_proto *bpf_get_trace_vprintk_proto(void)
+{
+ __set_printk_clr_event();
+ return &bpf_trace_vprintk_proto;
+}
+
BPF_CALL_5(bpf_seq_printf, struct seq_file *, m, char *, fmt, u32, fmt_size,
const void *, data, u32, data_len)
{
@@ -1160,6 +1208,8 @@ bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
return &bpf_get_func_ip_proto_tracing;
case BPF_FUNC_get_branch_snapshot:
return &bpf_get_branch_snapshot_proto;
+ case BPF_FUNC_trace_vprintk:
+ return bpf_get_trace_vprintk_proto();
default:
return bpf_base_func_proto(func_id);
}
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 3e9785f1064a..98ca79a67937 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -4898,6 +4898,16 @@ union bpf_attr {
* **-EINVAL** if *flags* is not zero.
*
* **-ENOENT** if architecture does not support branch records.
+ *
+ * long bpf_trace_vprintk(const char *fmt, u32 fmt_size, const void *data, u32 data_len)
+ * Description
+ * Behaves like **bpf_trace_printk**\ () helper, but takes an array of u64
+ * to format and can handle more format args as a result.
+ *
+ * Arguments are to be used as in **bpf_seq_printf**\ () helper.
+ * Return
+ * The number of bytes written to the buffer, or a negative error
+ * in case of failure.
*/
#define __BPF_FUNC_MAPPER(FN) \
FN(unspec), \
@@ -5077,6 +5087,7 @@ union bpf_attr {
FN(get_attach_cookie), \
FN(task_pt_regs), \
FN(get_branch_snapshot), \
+ FN(trace_vprintk), \
/* */
/* integer value in 'imm' field of BPF_CALL instruction selects which helper
--
2.30.2
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v6 bpf-next 4/9] libbpf: Modify bpf_printk to choose helper based on arg count
2021-09-17 18:29 [PATCH v6 bpf-next 0/9] bpf: implement variadic printk helper Dave Marchevsky
` (2 preceding siblings ...)
2021-09-17 18:29 ` [PATCH v6 bpf-next 3/9] bpf: add bpf_trace_vprintk helper Dave Marchevsky
@ 2021-09-17 18:29 ` Dave Marchevsky
2021-09-17 18:29 ` [PATCH v6 bpf-next 5/9] libbpf: use static const fmt string in __bpf_printk Dave Marchevsky
` (5 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Dave Marchevsky @ 2021-09-17 18:29 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Yonghong Song, netdev, Dave Marchevsky
Instead of being a thin wrapper which calls into bpf_trace_printk,
libbpf's bpf_printk convenience macro now chooses between
bpf_trace_printk and bpf_trace_vprintk. If the arg count (excluding
format string) is >3, use bpf_trace_vprintk, otherwise use the older
helper.
The motivation behind this added complexity - instead of migrating
entirely to bpf_trace_vprintk - is to maintain good developer experience
for users compiling against new libbpf but running on older kernels.
Users who are passing <=3 args to bpf_printk will see no change in their
bytecode.
__bpf_vprintk functions similarly to BPF_SEQ_PRINTF and BPF_SNPRINTF
macros elsewhere in the file - it allows use of bpf_trace_vprintk
without manual conversion of varargs to u64 array. Previous
implementation of bpf_printk macro is moved to __bpf_printk for use by
the new implementation.
This does change behavior of bpf_printk calls with >3 args in the "new
libbpf, old kernels" scenario. Before this patch, attempting to use 4
args to bpf_printk results in a compile-time error. After this patch,
using bpf_printk with 4 args results in a trace_vprintk helper call
being emitted and a load-time failure on older kernels.
Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
---
tools/lib/bpf/bpf_helpers.h | 45 ++++++++++++++++++++++++++++++-------
1 file changed, 37 insertions(+), 8 deletions(-)
diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
index b9987c3efa3c..55a308796625 100644
--- a/tools/lib/bpf/bpf_helpers.h
+++ b/tools/lib/bpf/bpf_helpers.h
@@ -14,14 +14,6 @@
#define __type(name, val) typeof(val) *name
#define __array(name, val) typeof(val) *name[]
-/* Helper macro to print out debug messages */
-#define bpf_printk(fmt, ...) \
-({ \
- char ____fmt[] = fmt; \
- bpf_trace_printk(____fmt, sizeof(____fmt), \
- ##__VA_ARGS__); \
-})
-
/*
* Helper macro to place programs, maps, license in
* different sections in elf_bpf file. Section names
@@ -224,4 +216,41 @@ enum libbpf_tristate {
___param, sizeof(___param)); \
})
+#define __bpf_printk(fmt, ...) \
+({ \
+ char ____fmt[] = fmt; \
+ bpf_trace_printk(____fmt, sizeof(____fmt), \
+ ##__VA_ARGS__); \
+})
+
+/*
+ * __bpf_vprintk wraps the bpf_trace_vprintk helper with variadic arguments
+ * instead of an array of u64.
+ */
+#define __bpf_vprintk(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_trace_vprintk(___fmt, sizeof(___fmt), \
+ ___param, sizeof(___param)); \
+})
+
+/* Use __bpf_printk when bpf_printk call has 3 or fewer fmt args
+ * Otherwise use __bpf_vprintk
+ */
+#define ___bpf_pick_printk(...) \
+ ___bpf_nth(_, ##__VA_ARGS__, __bpf_vprintk, __bpf_vprintk, __bpf_vprintk, \
+ __bpf_vprintk, __bpf_vprintk, __bpf_vprintk, __bpf_vprintk, \
+ __bpf_vprintk, __bpf_vprintk, __bpf_printk /*3*/, __bpf_printk /*2*/,\
+ __bpf_printk /*1*/, __bpf_printk /*0*/)
+
+/* Helper macro to print out debug messages */
+#define bpf_printk(fmt, args...) ___bpf_pick_printk(args)(fmt, ##args)
+
#endif
--
2.30.2
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v6 bpf-next 5/9] libbpf: use static const fmt string in __bpf_printk
2021-09-17 18:29 [PATCH v6 bpf-next 0/9] bpf: implement variadic printk helper Dave Marchevsky
` (3 preceding siblings ...)
2021-09-17 18:29 ` [PATCH v6 bpf-next 4/9] libbpf: Modify bpf_printk to choose helper based on arg count Dave Marchevsky
@ 2021-09-17 18:29 ` Dave Marchevsky
2021-09-17 18:29 ` [PATCH v6 bpf-next 6/9] bpftool: only probe trace_vprintk feature in 'full' mode Dave Marchevsky
` (4 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Dave Marchevsky @ 2021-09-17 18:29 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Yonghong Song, netdev, Dave Marchevsky
The __bpf_printk convenience macro was using a 'char' fmt string holder
as it predates support for globals in libbpf. Move to more efficient
'static const char', but provide a fallback to the old way via
BPF_NO_GLOBAL_DATA so users on old kernels can still use the macro.
Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
---
tools/lib/bpf/bpf_helpers.h | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
index 55a308796625..963b1060d944 100644
--- a/tools/lib/bpf/bpf_helpers.h
+++ b/tools/lib/bpf/bpf_helpers.h
@@ -216,9 +216,15 @@ enum libbpf_tristate {
___param, sizeof(___param)); \
})
+#ifdef BPF_NO_GLOBAL_DATA
+#define BPF_PRINTK_FMT_MOD
+#else
+#define BPF_PRINTK_FMT_MOD static const
+#endif
+
#define __bpf_printk(fmt, ...) \
({ \
- char ____fmt[] = fmt; \
+ BPF_PRINTK_FMT_MOD char ____fmt[] = fmt; \
bpf_trace_printk(____fmt, sizeof(____fmt), \
##__VA_ARGS__); \
})
--
2.30.2
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v6 bpf-next 6/9] bpftool: only probe trace_vprintk feature in 'full' mode
2021-09-17 18:29 [PATCH v6 bpf-next 0/9] bpf: implement variadic printk helper Dave Marchevsky
` (4 preceding siblings ...)
2021-09-17 18:29 ` [PATCH v6 bpf-next 5/9] libbpf: use static const fmt string in __bpf_printk Dave Marchevsky
@ 2021-09-17 18:29 ` Dave Marchevsky
2021-09-17 18:29 ` [PATCH v6 bpf-next 7/9] selftests/bpf: Migrate prog_tests/trace_printk CHECKs to ASSERTs Dave Marchevsky
` (3 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Dave Marchevsky @ 2021-09-17 18:29 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Yonghong Song, netdev, Dave Marchevsky
Since commit 368cb0e7cdb5e ("bpftool: Make probes which emit dmesg
warnings optional"), some helpers aren't probed by bpftool unless
`full` arg is added to `bpftool feature probe`.
bpf_trace_vprintk can emit dmesg warnings when probed, so include it.
Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
---
tools/bpf/bpftool/feature.c | 1 +
tools/testing/selftests/bpf/test_bpftool.py | 22 +++++++++------------
2 files changed, 10 insertions(+), 13 deletions(-)
diff --git a/tools/bpf/bpftool/feature.c b/tools/bpf/bpftool/feature.c
index 7f36385aa9e2..ade44577688e 100644
--- a/tools/bpf/bpftool/feature.c
+++ b/tools/bpf/bpftool/feature.c
@@ -624,6 +624,7 @@ probe_helpers_for_progtype(enum bpf_prog_type prog_type, bool supported_type,
*/
switch (id) {
case BPF_FUNC_trace_printk:
+ case BPF_FUNC_trace_vprintk:
case BPF_FUNC_probe_write_user:
if (!full_mode)
continue;
diff --git a/tools/testing/selftests/bpf/test_bpftool.py b/tools/testing/selftests/bpf/test_bpftool.py
index 4fed2dc25c0a..1c2408ee1f5d 100644
--- a/tools/testing/selftests/bpf/test_bpftool.py
+++ b/tools/testing/selftests/bpf/test_bpftool.py
@@ -57,6 +57,11 @@ def default_iface(f):
return f(*args, iface, **kwargs)
return wrapper
+DMESG_EMITTING_HELPERS = [
+ "bpf_probe_write_user",
+ "bpf_trace_printk",
+ "bpf_trace_vprintk",
+ ]
class TestBpftool(unittest.TestCase):
@classmethod
@@ -67,10 +72,7 @@ class TestBpftool(unittest.TestCase):
@default_iface
def test_feature_dev_json(self, iface):
- unexpected_helpers = [
- "bpf_probe_write_user",
- "bpf_trace_printk",
- ]
+ unexpected_helpers = DMESG_EMITTING_HELPERS
expected_keys = [
"syscall_config",
"program_types",
@@ -94,10 +96,7 @@ class TestBpftool(unittest.TestCase):
bpftool_json(["feature", "probe"]),
bpftool_json(["feature"]),
]
- unexpected_helpers = [
- "bpf_probe_write_user",
- "bpf_trace_printk",
- ]
+ unexpected_helpers = DMESG_EMITTING_HELPERS
expected_keys = [
"syscall_config",
"system_config",
@@ -121,10 +120,7 @@ class TestBpftool(unittest.TestCase):
bpftool_json(["feature", "probe", "kernel", "full"]),
bpftool_json(["feature", "probe", "full"]),
]
- expected_helpers = [
- "bpf_probe_write_user",
- "bpf_trace_printk",
- ]
+ expected_helpers = DMESG_EMITTING_HELPERS
for tc in test_cases:
# Check if expected helpers are included at least once in any
@@ -157,7 +153,7 @@ class TestBpftool(unittest.TestCase):
not_full_set.add(helper)
self.assertCountEqual(full_set - not_full_set,
- {"bpf_probe_write_user", "bpf_trace_printk"})
+ set(DMESG_EMITTING_HELPERS))
self.assertCountEqual(not_full_set - full_set, set())
def test_feature_macros(self):
--
2.30.2
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v6 bpf-next 7/9] selftests/bpf: Migrate prog_tests/trace_printk CHECKs to ASSERTs
2021-09-17 18:29 [PATCH v6 bpf-next 0/9] bpf: implement variadic printk helper Dave Marchevsky
` (5 preceding siblings ...)
2021-09-17 18:29 ` [PATCH v6 bpf-next 6/9] bpftool: only probe trace_vprintk feature in 'full' mode Dave Marchevsky
@ 2021-09-17 18:29 ` Dave Marchevsky
2021-09-17 18:29 ` [PATCH v6 bpf-next 8/9] selftests/bpf: add trace_vprintk test prog Dave Marchevsky
` (2 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Dave Marchevsky @ 2021-09-17 18:29 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Yonghong Song, netdev, Dave Marchevsky
Guidance for new tests is to use ASSERT macros instead of CHECK. Since
trace_vprintk test will borrow heavily from trace_printk's, migrate its
CHECKs so it remains obvious that the two are closely related.
Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
---
.../selftests/bpf/prog_tests/trace_printk.c | 24 +++++++------------
1 file changed, 9 insertions(+), 15 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/trace_printk.c b/tools/testing/selftests/bpf/prog_tests/trace_printk.c
index d39bc00feb45..e47835f0a674 100644
--- a/tools/testing/selftests/bpf/prog_tests/trace_printk.c
+++ b/tools/testing/selftests/bpf/prog_tests/trace_printk.c
@@ -10,7 +10,7 @@
void test_trace_printk(void)
{
- int err, iter = 0, duration = 0, found = 0;
+ int err = 0, iter = 0, found = 0;
struct trace_printk__bss *bss;
struct trace_printk *skel;
char *buf = NULL;
@@ -18,25 +18,24 @@ void test_trace_printk(void)
size_t buflen;
skel = trace_printk__open();
- if (CHECK(!skel, "skel_open", "failed to open skeleton\n"))
+ if (!ASSERT_OK_PTR(skel, "trace_printk__open"))
return;
- ASSERT_EQ(skel->rodata->fmt[0], 'T', "invalid printk fmt string");
+ ASSERT_EQ(skel->rodata->fmt[0], 'T', "skel->rodata->fmt[0]");
skel->rodata->fmt[0] = 't';
err = trace_printk__load(skel);
- if (CHECK(err, "skel_load", "failed to load skeleton: %d\n", err))
+ if (!ASSERT_OK(err, "trace_printk__load"))
goto cleanup;
bss = skel->bss;
err = trace_printk__attach(skel);
- if (CHECK(err, "skel_attach", "skeleton attach failed: %d\n", err))
+ if (!ASSERT_OK(err, "trace_printk__attach"))
goto cleanup;
fp = fopen(TRACEBUF, "r");
- if (CHECK(fp == NULL, "could not open trace buffer",
- "error %d opening %s", errno, TRACEBUF))
+ if (!ASSERT_OK_PTR(fp, "fopen(TRACEBUF)"))
goto cleanup;
/* We do not want to wait forever if this test fails... */
@@ -46,14 +45,10 @@ void test_trace_printk(void)
usleep(1);
trace_printk__detach(skel);
- if (CHECK(bss->trace_printk_ran == 0,
- "bpf_trace_printk never ran",
- "ran == %d", bss->trace_printk_ran))
+ if (!ASSERT_GT(bss->trace_printk_ran, 0, "bss->trace_printk_ran"))
goto cleanup;
- if (CHECK(bss->trace_printk_ret <= 0,
- "bpf_trace_printk returned <= 0 value",
- "got %d", bss->trace_printk_ret))
+ if (!ASSERT_GT(bss->trace_printk_ret, 0, "bss->trace_printk_ret"))
goto cleanup;
/* verify our search string is in the trace buffer */
@@ -66,8 +61,7 @@ void test_trace_printk(void)
break;
}
- if (CHECK(!found, "message from bpf_trace_printk not found",
- "no instance of %s in %s", SEARCHMSG, TRACEBUF))
+ if (!ASSERT_EQ(found, bss->trace_printk_ran, "found"))
goto cleanup;
cleanup:
--
2.30.2
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v6 bpf-next 8/9] selftests/bpf: add trace_vprintk test prog
2021-09-17 18:29 [PATCH v6 bpf-next 0/9] bpf: implement variadic printk helper Dave Marchevsky
` (6 preceding siblings ...)
2021-09-17 18:29 ` [PATCH v6 bpf-next 7/9] selftests/bpf: Migrate prog_tests/trace_printk CHECKs to ASSERTs Dave Marchevsky
@ 2021-09-17 18:29 ` Dave Marchevsky
2021-09-17 18:29 ` [PATCH v6 bpf-next 9/9] bpf: Clarify data_len param in bpf_snprintf and bpf_seq_printf comments Dave Marchevsky
2021-09-17 21:06 ` [PATCH v6 bpf-next 0/9] bpf: implement variadic printk helper Alexei Starovoitov
9 siblings, 0 replies; 11+ messages in thread
From: Dave Marchevsky @ 2021-09-17 18:29 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Yonghong Song, netdev, Dave Marchevsky
This commit adds a test prog for vprintk which confirms that:
* bpf_trace_vprintk is writing to /sys/kernel/debug/tracing/trace_pipe
* __bpf_vprintk macro works as expected
* >3 args are printed
* bpf_printk w/ 0 format args compiles
* bpf_trace_vprintk call w/ a fmt specifier but NULL fmt data fails
Approach and code are borrowed from trace_printk test.
Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
---
tools/testing/selftests/bpf/Makefile | 3 +-
.../selftests/bpf/prog_tests/trace_vprintk.c | 68 +++++++++++++++++++
.../selftests/bpf/progs/trace_vprintk.c | 33 +++++++++
3 files changed, 103 insertions(+), 1 deletion(-)
create mode 100644 tools/testing/selftests/bpf/prog_tests/trace_vprintk.c
create mode 100644 tools/testing/selftests/bpf/progs/trace_vprintk.c
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 1a4d30ff3275..326ea75ce99e 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -315,7 +315,8 @@ LINKED_SKELS := test_static_linked.skel.h linked_funcs.skel.h \
linked_vars.skel.h linked_maps.skel.h
LSKELS := kfunc_call_test.c fentry_test.c fexit_test.c fexit_sleep.c \
- test_ksyms_module.c test_ringbuf.c atomics.c trace_printk.c
+ test_ksyms_module.c test_ringbuf.c atomics.c trace_printk.c \
+ trace_vprintk.c
SKEL_BLACKLIST += $$(LSKELS)
test_static_linked.skel.h-deps := test_static_linked1.o test_static_linked2.o
diff --git a/tools/testing/selftests/bpf/prog_tests/trace_vprintk.c b/tools/testing/selftests/bpf/prog_tests/trace_vprintk.c
new file mode 100644
index 000000000000..61a24e62e1a0
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/trace_vprintk.c
@@ -0,0 +1,68 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2021 Facebook */
+
+#include <test_progs.h>
+
+#include "trace_vprintk.lskel.h"
+
+#define TRACEBUF "/sys/kernel/debug/tracing/trace_pipe"
+#define SEARCHMSG "1,2,3,4,5,6,7,8,9,10"
+
+void test_trace_vprintk(void)
+{
+ int err = 0, iter = 0, found = 0;
+ struct trace_vprintk__bss *bss;
+ struct trace_vprintk *skel;
+ char *buf = NULL;
+ FILE *fp = NULL;
+ size_t buflen;
+
+ skel = trace_vprintk__open_and_load();
+ if (!ASSERT_OK_PTR(skel, "trace_vprintk__open_and_load"))
+ goto cleanup;
+
+ bss = skel->bss;
+
+ err = trace_vprintk__attach(skel);
+ if (!ASSERT_OK(err, "trace_vprintk__attach"))
+ goto cleanup;
+
+ fp = fopen(TRACEBUF, "r");
+ if (!ASSERT_OK_PTR(fp, "fopen(TRACEBUF)"))
+ goto cleanup;
+
+ /* We do not want to wait forever if this test fails... */
+ fcntl(fileno(fp), F_SETFL, O_NONBLOCK);
+
+ /* wait for tracepoint to trigger */
+ usleep(1);
+ trace_vprintk__detach(skel);
+
+ if (!ASSERT_GT(bss->trace_vprintk_ran, 0, "bss->trace_vprintk_ran"))
+ goto cleanup;
+
+ if (!ASSERT_GT(bss->trace_vprintk_ret, 0, "bss->trace_vprintk_ret"))
+ goto cleanup;
+
+ /* verify our search string is in the trace buffer */
+ while (getline(&buf, &buflen, fp) >= 0 || errno == EAGAIN) {
+ if (strstr(buf, SEARCHMSG) != NULL)
+ found++;
+ if (found == bss->trace_vprintk_ran)
+ break;
+ if (++iter > 1000)
+ break;
+ }
+
+ if (!ASSERT_EQ(found, bss->trace_vprintk_ran, "found"))
+ goto cleanup;
+
+ if (!ASSERT_LT(bss->null_data_vprintk_ret, 0, "bss->null_data_vprintk_ret"))
+ goto cleanup;
+
+cleanup:
+ trace_vprintk__destroy(skel);
+ free(buf);
+ if (fp)
+ fclose(fp);
+}
diff --git a/tools/testing/selftests/bpf/progs/trace_vprintk.c b/tools/testing/selftests/bpf/progs/trace_vprintk.c
new file mode 100644
index 000000000000..d327241ba047
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/trace_vprintk.c
@@ -0,0 +1,33 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2021 Facebook */
+
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+char _license[] SEC("license") = "GPL";
+
+int null_data_vprintk_ret = 0;
+int trace_vprintk_ret = 0;
+int trace_vprintk_ran = 0;
+
+SEC("fentry/__x64_sys_nanosleep")
+int sys_enter(void *ctx)
+{
+ static const char one[] = "1";
+ static const char three[] = "3";
+ static const char five[] = "5";
+ static const char seven[] = "7";
+ static const char nine[] = "9";
+ static const char f[] = "%pS\n";
+
+ /* runner doesn't search for \t, just ensure it compiles */
+ bpf_printk("\t");
+
+ trace_vprintk_ret = __bpf_vprintk("%s,%d,%s,%d,%s,%d,%s,%d,%s,%d %d\n",
+ one, 2, three, 4, five, 6, seven, 8, nine, 10, ++trace_vprintk_ran);
+
+ /* non-NULL fmt w/ NULL data should result in error */
+ null_data_vprintk_ret = bpf_trace_vprintk(f, sizeof(f), NULL, 0);
+ return 0;
+}
--
2.30.2
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v6 bpf-next 9/9] bpf: Clarify data_len param in bpf_snprintf and bpf_seq_printf comments
2021-09-17 18:29 [PATCH v6 bpf-next 0/9] bpf: implement variadic printk helper Dave Marchevsky
` (7 preceding siblings ...)
2021-09-17 18:29 ` [PATCH v6 bpf-next 8/9] selftests/bpf: add trace_vprintk test prog Dave Marchevsky
@ 2021-09-17 18:29 ` Dave Marchevsky
2021-09-17 21:06 ` [PATCH v6 bpf-next 0/9] bpf: implement variadic printk helper Alexei Starovoitov
9 siblings, 0 replies; 11+ messages in thread
From: Dave Marchevsky @ 2021-09-17 18:29 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Yonghong Song, netdev, Dave Marchevsky
Since the data_len in these two functions is a byte len of the preceding
u64 *data array, it must always be a multiple of 8. If this isn't the
case both helpers error out, so let's make the requirement explicit so
users don't need to infer it.
Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
---
include/uapi/linux/bpf.h | 5 +++--
tools/include/uapi/linux/bpf.h | 5 +++--
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 98ca79a67937..6fc59d61937a 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -4046,7 +4046,7 @@ union bpf_attr {
* arguments. The *data* are a **u64** array and corresponding format string
* values are stored in the array. For strings and pointers where pointees
* are accessed, only the pointer values are stored in the *data* array.
- * The *data_len* is the size of *data* in bytes.
+ * The *data_len* is the size of *data* in bytes - must be a multiple of 8.
*
* Formats **%s**, **%p{i,I}{4,6}** requires to read kernel memory.
* Reading kernel memory may fail due to either invalid address or
@@ -4751,7 +4751,8 @@ union bpf_attr {
* Each format specifier in **fmt** corresponds to one u64 element
* in the **data** array. For strings and pointers where pointees
* are accessed, only the pointer values are stored in the *data*
- * array. The *data_len* is the size of *data* in bytes.
+ * array. The *data_len* is the size of *data* in bytes - must be
+ * a multiple of 8.
*
* Formats **%s** and **%p{i,I}{4,6}** require to read kernel
* memory. Reading kernel memory may fail due to either invalid
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 98ca79a67937..6fc59d61937a 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -4046,7 +4046,7 @@ union bpf_attr {
* arguments. The *data* are a **u64** array and corresponding format string
* values are stored in the array. For strings and pointers where pointees
* are accessed, only the pointer values are stored in the *data* array.
- * The *data_len* is the size of *data* in bytes.
+ * The *data_len* is the size of *data* in bytes - must be a multiple of 8.
*
* Formats **%s**, **%p{i,I}{4,6}** requires to read kernel memory.
* Reading kernel memory may fail due to either invalid address or
@@ -4751,7 +4751,8 @@ union bpf_attr {
* Each format specifier in **fmt** corresponds to one u64 element
* in the **data** array. For strings and pointers where pointees
* are accessed, only the pointer values are stored in the *data*
- * array. The *data_len* is the size of *data* in bytes.
+ * array. The *data_len* is the size of *data* in bytes - must be
+ * a multiple of 8.
*
* Formats **%s** and **%p{i,I}{4,6}** require to read kernel
* memory. Reading kernel memory may fail due to either invalid
--
2.30.2
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v6 bpf-next 0/9] bpf: implement variadic printk helper
2021-09-17 18:29 [PATCH v6 bpf-next 0/9] bpf: implement variadic printk helper Dave Marchevsky
` (8 preceding siblings ...)
2021-09-17 18:29 ` [PATCH v6 bpf-next 9/9] bpf: Clarify data_len param in bpf_snprintf and bpf_seq_printf comments Dave Marchevsky
@ 2021-09-17 21:06 ` Alexei Starovoitov
9 siblings, 0 replies; 11+ messages in thread
From: Alexei Starovoitov @ 2021-09-17 21:06 UTC (permalink / raw)
To: Dave Marchevsky
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Yonghong Song, Network Development
On Fri, Sep 17, 2021 at 11:29 AM Dave Marchevsky <davemarchevsky@fb.com> wrote:
>
> This series introduces a new helper, bpf_trace_vprintk, which functions
> like bpf_trace_printk but supports > 3 arguments via a pseudo-vararg u64
> array. The bpf_printk libbpf convenience macro is modified to use
> bpf_trace_vprintk when > 3 varargs are passed, otherwise the previous
> behavior - using bpf_trace_printk - is retained.
>
> Helper functions and macros added during the implementation of
> bpf_seq_printf and bpf_snprintf do most of the heavy lifting for
> bpf_trace_vprintk. There's no novel format string wrangling here.
>
> Usecase here is straightforward: Giving BPF program writers a more
> powerful printk will ease development of BPF programs, particularly
> during debugging and testing, where printk tends to be used.
>
> This feature was proposed by Andrii in libbpf mirror's issue tracker
> [1].
>
> [1] https://github.com/libbpf/libbpf/issues/315
>
> v5 -> v6: Rebase to pick up newly-added helper
Applied. Thanks!
^ permalink raw reply [flat|nested] 11+ messages in thread