We have a usecase where we want to audit symbol names (if available) in
callback registration hooks. (ex: fentry/nf_register_net_hook)
A few months back, I proposed a bpf_kallsyms_lookup series but it was
decided in the reviews that a more generic helper, bpf_snprintf, would
be more useful.
This series implements the helper according to the feedback received in
https://lore.kernel.org/bpf/[email protected]/T/#u
- A new arg type guarantees the NULL-termination of string arguments and
lets us pass format strings in only one arg
- A new helper is implemented using that guarantee. Because the format
string is known at verification time, the format string validation is
done by the verifier
- To implement a series of tests for bpf_snprintf, the logic for
marshalling variadic args in a fixed-size array is reworked as per:
https://lore.kernel.org/bpf/[email protected]/T/#u
---
Changes in v3:
- Simplified temporary buffer acquisition with try_get_fmt_tmp_buf()
- Made zero-termination check more consistent
- Allowed NULL output_buffer
- Simplified the BPF_CAST_FMT_ARG macro
- Three new test cases: number padding, simple string with no arg and
string length extraction only with a NULL output buffer
- Clarified helper's description for edge cases (eg: str_size == 0)
- Lots of cosmetic changes
---
Changes in v2:
- Extracted the format validation/argument sanitization in a generic way
for all printf-like helpers.
- bpf_snprintf's str_size can now be 0
- bpf_snprintf is now exposed to all BPF program types
- We now preempt_disable when using a per-cpu temporary buffer
- Addressed a few cosmetic changes
Florent Revest (6):
bpf: Factorize bpf_trace_printk and bpf_seq_printf
bpf: Add a ARG_PTR_TO_CONST_STR argument type
bpf: Add a bpf_snprintf helper
libbpf: Initialize the bpf_seq_printf parameters array field by field
libbpf: Introduce a BPF_SNPRINTF helper macro
selftests/bpf: Add a series of tests for bpf_snprintf
include/linux/bpf.h | 7 +
include/uapi/linux/bpf.h | 28 +
kernel/bpf/helpers.c | 2 +
kernel/bpf/verifier.c | 82 +++
kernel/trace/bpf_trace.c | 579 +++++++++---------
tools/include/uapi/linux/bpf.h | 28 +
tools/lib/bpf/bpf_tracing.h | 58 +-
.../selftests/bpf/prog_tests/snprintf.c | 81 +++
.../selftests/bpf/progs/test_snprintf.c | 74 +++
9 files changed, 647 insertions(+), 292 deletions(-)
create mode 100644 tools/testing/selftests/bpf/prog_tests/snprintf.c
create mode 100644 tools/testing/selftests/bpf/progs/test_snprintf.c
--
2.31.1.295.g9ea45b61b8-goog
The implementation takes inspiration from the existing bpf_trace_printk
helper but there are a few differences:
To allow for a large number of format-specifiers, parameters are
provided in an array, like in bpf_seq_printf.
Because the output string takes two arguments and the array of
parameters also takes two arguments, the format string needs to fit in
one argument. Thankfully, ARG_PTR_TO_CONST_STR is guaranteed to point to
a zero-terminated read-only map so we don't need a format string length
arg.
Because the format-string is known at verification time, we also do
a first pass of format string validation in the verifier logic. This
makes debugging easier.
Signed-off-by: Florent Revest <[email protected]>
---
include/linux/bpf.h | 6 ++++
include/uapi/linux/bpf.h | 28 +++++++++++++++++++
kernel/bpf/helpers.c | 2 ++
kernel/bpf/verifier.c | 41 ++++++++++++++++++++++++++++
kernel/trace/bpf_trace.c | 50 ++++++++++++++++++++++++++++++++++
tools/include/uapi/linux/bpf.h | 28 +++++++++++++++++++
6 files changed, 155 insertions(+)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 7d389eeee0b3..a3650fc93068 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1953,6 +1953,7 @@ extern const struct bpf_func_proto bpf_skc_to_tcp_request_sock_proto;
extern const struct bpf_func_proto bpf_skc_to_udp6_sock_proto;
extern const struct bpf_func_proto bpf_copy_from_user_proto;
extern const struct bpf_func_proto bpf_snprintf_btf_proto;
+extern const struct bpf_func_proto bpf_snprintf_proto;
extern const struct bpf_func_proto bpf_per_cpu_ptr_proto;
extern const struct bpf_func_proto bpf_this_cpu_ptr_proto;
extern const struct bpf_func_proto bpf_ktime_get_coarse_ns_proto;
@@ -2078,4 +2079,9 @@ 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);
+enum bpf_printf_mod_type;
+int bpf_printf_prepare(char *fmt, u32 fmt_size, const u64 *raw_args,
+ u64 *final_args, enum bpf_printf_mod_type *mod,
+ u32 num_args);
+
#endif /* _LINUX_BPF_H */
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 49371eba98ba..40546d4676f1 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -4671,6 +4671,33 @@ union bpf_attr {
* Return
* The number of traversed map elements for success, **-EINVAL** for
* invalid **flags**.
+ *
+ * long bpf_snprintf(char *str, u32 str_size, const char *fmt, u64 *data, u32 data_len)
+ * Description
+ * Outputs a string into the **str** buffer of size **str_size**
+ * based on a format string stored in a read-only map pointed by
+ * **fmt**.
+ *
+ * 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.
+ *
+ * Formats **%s** and **%p{i,I}{4,6}** require to read kernel
+ * memory. Reading kernel memory may fail due to either invalid
+ * address or valid address but requiring a major memory fault. If
+ * reading kernel memory fails, the string for **%s** will be an
+ * empty string, and the ip address for **%p{i,I}{4,6}** will be 0.
+ * Not returning error to bpf program is consistent with what
+ * **bpf_trace_printk**\ () does for now.
+ *
+ * Return
+ * The strictly positive length of the formatted string, including
+ * the trailing zero character. If the return value is greater than
+ * **str_size**, **str** contains a truncated string, guaranteed to
+ * be zero-terminated except when **str_size** is 0.
+ *
+ * Or **-EBUSY** if the per-CPU memory copy buffer is busy.
*/
#define __BPF_FUNC_MAPPER(FN) \
FN(unspec), \
@@ -4838,6 +4865,7 @@ union bpf_attr {
FN(sock_from_file), \
FN(check_mtu), \
FN(for_each_map_elem), \
+ FN(snprintf), \
/* */
/* integer value in 'imm' field of BPF_CALL instruction selects which helper
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index f306611c4ddf..ec45c7526924 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -757,6 +757,8 @@ bpf_base_func_proto(enum bpf_func_id func_id)
return &bpf_probe_read_kernel_str_proto;
case BPF_FUNC_snprintf_btf:
return &bpf_snprintf_btf_proto;
+ case BPF_FUNC_snprintf:
+ return &bpf_snprintf_proto;
default:
return NULL;
}
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 5f46dd6f3383..d4020e5f91ee 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -5918,6 +5918,41 @@ static int check_reference_leak(struct bpf_verifier_env *env)
return state->acquired_refs ? -EINVAL : 0;
}
+static int check_bpf_snprintf_call(struct bpf_verifier_env *env,
+ struct bpf_reg_state *regs)
+{
+ struct bpf_reg_state *fmt_reg = ®s[BPF_REG_3];
+ struct bpf_reg_state *data_len_reg = ®s[BPF_REG_5];
+ struct bpf_map *fmt_map = fmt_reg->map_ptr;
+ int err, fmt_map_off, num_args;
+ u64 fmt_addr;
+ char *fmt;
+
+ /* data must be an array of u64 */
+ if (data_len_reg->var_off.value % 8)
+ return -EINVAL;
+ num_args = data_len_reg->var_off.value / 8;
+
+ /* fmt being ARG_PTR_TO_CONST_STR guarantees that var_off is const
+ * and map_direct_value_addr is set.
+ */
+ fmt_map_off = fmt_reg->off + fmt_reg->var_off.value;
+ err = fmt_map->ops->map_direct_value_addr(fmt_map, &fmt_addr,
+ fmt_map_off);
+ if (err)
+ return err;
+ fmt = (char *)fmt_addr + fmt_map_off;
+
+ /* We are also guaranteed that fmt+fmt_map_off is NULL terminated, we
+ * can focus on validating the format specifiers.
+ */
+ err = bpf_printf_prepare(fmt, UINT_MAX, NULL, NULL, NULL, num_args);
+ if (err < 0)
+ verbose(env, "Invalid format string\n");
+
+ return err;
+}
+
static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
int *insn_idx_p)
{
@@ -6032,6 +6067,12 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
return -EINVAL;
}
+ if (func_id == BPF_FUNC_snprintf) {
+ err = check_bpf_snprintf_call(env, regs);
+ if (err < 0)
+ return err;
+ }
+
/* reset caller saved regs */
for (i = 0; i < CALLER_SAVED_REGS; i++) {
mark_reg_not_init(env, regs, caller_saved[i]);
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 3ce9aeee6681..990ed98d2842 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1238,6 +1238,54 @@ const struct bpf_func_proto bpf_snprintf_btf_proto = {
.arg5_type = ARG_ANYTHING,
};
+#define MAX_SNPRINTF_VARARGS 12
+
+BPF_CALL_5(bpf_snprintf, char *, str, u32, str_size, char *, fmt,
+ const void *, data, u32, data_len)
+{
+ enum bpf_printf_mod_type mod[MAX_SNPRINTF_VARARGS];
+ u64 args[MAX_SNPRINTF_VARARGS];
+ int err, num_args;
+
+ if (data_len % 8 || data_len > MAX_SNPRINTF_VARARGS * 8 ||
+ (data_len && !data))
+ return -EINVAL;
+ num_args = data_len / 8;
+
+ /* ARG_PTR_TO_CONST_STR guarantees that fmt is zero-terminated so we
+ * can safely give an unbounded size.
+ */
+ err = bpf_printf_prepare(fmt, UINT_MAX, data, args, mod, num_args);
+ if (err < 0)
+ return err;
+
+ /* Maximumly we can have MAX_SNPRINTF_VARARGS parameters, just give
+ * all of them to snprintf().
+ */
+ err = snprintf(str, str_size, fmt, BPF_CAST_FMT_ARG(0, args, mod),
+ BPF_CAST_FMT_ARG(1, args, mod), BPF_CAST_FMT_ARG(2, args, mod),
+ BPF_CAST_FMT_ARG(3, args, mod), BPF_CAST_FMT_ARG(4, args, mod),
+ BPF_CAST_FMT_ARG(5, args, mod), BPF_CAST_FMT_ARG(6, args, mod),
+ BPF_CAST_FMT_ARG(7, args, mod), BPF_CAST_FMT_ARG(8, args, mod),
+ BPF_CAST_FMT_ARG(9, args, mod), BPF_CAST_FMT_ARG(10, args, mod),
+ BPF_CAST_FMT_ARG(11, args, mod));
+
+ put_fmt_tmp_buf();
+
+ return err + 1;
+}
+
+const struct bpf_func_proto bpf_snprintf_proto = {
+ .func = bpf_snprintf,
+ .gpl_only = true,
+ .ret_type = RET_INTEGER,
+ .arg1_type = ARG_PTR_TO_MEM_OR_NULL,
+ .arg2_type = ARG_CONST_SIZE_OR_ZERO,
+ .arg3_type = ARG_PTR_TO_CONST_STR,
+ .arg4_type = ARG_PTR_TO_MEM_OR_NULL,
+ .arg5_type = ARG_CONST_SIZE_OR_ZERO,
+};
+
const struct bpf_func_proto *
bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
{
@@ -1340,6 +1388,8 @@ bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
return &bpf_task_storage_delete_proto;
case BPF_FUNC_for_each_map_elem:
return &bpf_for_each_map_elem_proto;
+ case BPF_FUNC_snprintf:
+ return &bpf_snprintf_proto;
default:
return NULL;
}
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 69902603012c..ffdd2ae18c69 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -4665,6 +4665,33 @@ union bpf_attr {
* Return
* The number of traversed map elements for success, **-EINVAL** for
* invalid **flags**.
+ *
+ * long bpf_snprintf(char *str, u32 str_size, const char *fmt, u64 *data, u32 data_len)
+ * Description
+ * Outputs a string into the **str** buffer of size **str_size**
+ * based on a format string stored in a read-only map pointed by
+ * **fmt**.
+ *
+ * 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.
+ *
+ * Formats **%s** and **%p{i,I}{4,6}** require to read kernel
+ * memory. Reading kernel memory may fail due to either invalid
+ * address or valid address but requiring a major memory fault. If
+ * reading kernel memory fails, the string for **%s** will be an
+ * empty string, and the ip address for **%p{i,I}{4,6}** will be 0.
+ * Not returning error to bpf program is consistent with what
+ * **bpf_trace_printk**\ () does for now.
+ *
+ * Return
+ * The strictly positive length of the formatted string, including
+ * the trailing zero character. If the return value is greater than
+ * **str_size**, **str** contains a truncated string, guaranteed to
+ * be zero-terminated except when **str_size** is 0.
+ *
+ * Or **-EBUSY** if the per-CPU memory copy buffer is busy.
*/
#define __BPF_FUNC_MAPPER(FN) \
FN(unspec), \
@@ -4832,6 +4859,7 @@ union bpf_attr {
FN(sock_from_file), \
FN(check_mtu), \
FN(for_each_map_elem), \
+ FN(snprintf), \
/* */
/* integer value in 'imm' field of BPF_CALL instruction selects which helper
--
2.31.1.295.g9ea45b61b8-goog
Similarly to BPF_SEQ_PRINTF, this macro turns variadic arguments into an
array of u64, making it more natural to call the bpf_snprintf helper.
Signed-off-by: Florent Revest <[email protected]>
---
tools/lib/bpf/bpf_tracing.h | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
index 1c2e91ee041d..8c954ebc0c7c 100644
--- a/tools/lib/bpf/bpf_tracing.h
+++ b/tools/lib/bpf/bpf_tracing.h
@@ -447,4 +447,22 @@ static __always_inline typeof(name(0)) ____##name(struct pt_regs *ctx, ##args)
___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
--
2.31.1.295.g9ea45b61b8-goog
This exercises most of the format specifiers.
Signed-off-by: Florent Revest <[email protected]>
Acked-by: Andrii Nakryiko <[email protected]>
---
.../selftests/bpf/prog_tests/snprintf.c | 81 +++++++++++++++++++
.../selftests/bpf/progs/test_snprintf.c | 74 +++++++++++++++++
2 files changed, 155 insertions(+)
create mode 100644 tools/testing/selftests/bpf/prog_tests/snprintf.c
create mode 100644 tools/testing/selftests/bpf/progs/test_snprintf.c
diff --git a/tools/testing/selftests/bpf/prog_tests/snprintf.c b/tools/testing/selftests/bpf/prog_tests/snprintf.c
new file mode 100644
index 000000000000..3ad1ee885273
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/snprintf.c
@@ -0,0 +1,81 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2021 Google LLC. */
+
+#include <test_progs.h>
+#include "test_snprintf.skel.h"
+
+#define EXP_NUM_OUT "-8 9 96 -424242 1337 DABBAD00"
+#define EXP_NUM_RET sizeof(EXP_NUM_OUT)
+
+#define EXP_IP_OUT "127.000.000.001 0000:0000:0000:0000:0000:0000:0000:0001"
+#define EXP_IP_RET sizeof(EXP_IP_OUT)
+
+/* The third specifier, %pB, depends on compiler inlining so don't check it */
+#define EXP_SYM_OUT "schedule schedule+0x0/"
+#define MIN_SYM_RET sizeof(EXP_SYM_OUT)
+
+/* The third specifier, %p, is a hashed pointer which changes on every reboot */
+#define EXP_ADDR_OUT "0000000000000000 ffff00000add4e55 "
+#define EXP_ADDR_RET sizeof(EXP_ADDR_OUT "unknownhashedptr")
+
+#define EXP_STR_OUT "str1 longstr"
+#define EXP_STR_RET sizeof(EXP_STR_OUT)
+
+#define EXP_OVER_OUT "%over"
+#define EXP_OVER_RET 10
+
+#define EXP_PAD_OUT " 4 000"
+#define EXP_PAD_RET 900007
+
+#define EXP_NO_ARG_OUT "simple case"
+#define EXP_NO_ARG_RET 12
+
+#define EXP_NO_BUF_RET 29
+
+void test_snprintf(void)
+{
+ char exp_addr_out[] = EXP_ADDR_OUT;
+ char exp_sym_out[] = EXP_SYM_OUT;
+ struct test_snprintf *skel;
+
+ skel = test_snprintf__open_and_load();
+ if (!ASSERT_OK_PTR(skel, "skel_open"))
+ return;
+
+ if (!ASSERT_OK(test_snprintf__attach(skel), "skel_attach"))
+ goto cleanup;
+
+ /* trigger tracepoint */
+ usleep(1);
+
+ ASSERT_STREQ(skel->bss->num_out, EXP_NUM_OUT, "num_out");
+ ASSERT_EQ(skel->bss->num_ret, EXP_NUM_RET, "num_ret");
+
+ ASSERT_STREQ(skel->bss->ip_out, EXP_IP_OUT, "ip_out");
+ ASSERT_EQ(skel->bss->ip_ret, EXP_IP_RET, "ip_ret");
+
+ ASSERT_OK(memcmp(skel->bss->sym_out, exp_sym_out,
+ sizeof(exp_sym_out) - 1), "sym_out");
+ ASSERT_LT(MIN_SYM_RET, skel->bss->sym_ret, "sym_ret");
+
+ ASSERT_OK(memcmp(skel->bss->addr_out, exp_addr_out,
+ sizeof(exp_addr_out) - 1), "addr_out");
+ ASSERT_EQ(skel->bss->addr_ret, EXP_ADDR_RET, "addr_ret");
+
+ ASSERT_STREQ(skel->bss->str_out, EXP_STR_OUT, "str_out");
+ ASSERT_EQ(skel->bss->str_ret, EXP_STR_RET, "str_ret");
+
+ ASSERT_STREQ(skel->bss->over_out, EXP_OVER_OUT, "over_out");
+ ASSERT_EQ(skel->bss->over_ret, EXP_OVER_RET, "over_ret");
+
+ ASSERT_STREQ(skel->bss->pad_out, EXP_PAD_OUT, "pad_out");
+ ASSERT_EQ(skel->bss->pad_ret, EXP_PAD_RET, "pad_ret");
+
+ ASSERT_STREQ(skel->bss->noarg_out, EXP_NO_ARG_OUT, "no_arg_out");
+ ASSERT_EQ(skel->bss->noarg_ret, EXP_NO_ARG_RET, "no_arg_ret");
+
+ ASSERT_EQ(skel->bss->nobuf_ret, EXP_NO_BUF_RET, "no_buf_ret");
+
+cleanup:
+ test_snprintf__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/progs/test_snprintf.c b/tools/testing/selftests/bpf/progs/test_snprintf.c
new file mode 100644
index 000000000000..4c36f355dfca
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_snprintf.c
@@ -0,0 +1,74 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2021 Google LLC. */
+
+#include <linux/bpf.h>
+#include <bpf/bpf_endian.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+char num_out[64] = {};
+long num_ret = 0;
+
+char ip_out[64] = {};
+long ip_ret = 0;
+
+char sym_out[64] = {};
+long sym_ret = 0;
+
+char addr_out[64] = {};
+long addr_ret = 0;
+
+char str_out[64] = {};
+long str_ret = 0;
+
+char over_out[6] = {};
+long over_ret = 0;
+
+char pad_out[10] = {};
+long pad_ret = 0;
+
+char noarg_out[64] = {};
+long noarg_ret = 0;
+
+long nobuf_ret = 0;
+
+extern const void schedule __ksym;
+
+SEC("raw_tp/sys_enter")
+int handler(const void *ctx)
+{
+ /* Convenient values to pretty-print */
+ const __u8 ex_ipv4[] = {127, 0, 0, 1};
+ const __u8 ex_ipv6[] = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1};
+ const char str1[] = "str1";
+ const char longstr[] = "longstr";
+
+ /* Integer types */
+ num_ret = BPF_SNPRINTF(num_out, sizeof(num_out),
+ "%d %u %x %li %llu %lX",
+ -8, 9, 150, -424242, 1337, 0xDABBAD00);
+ /* IP addresses */
+ ip_ret = BPF_SNPRINTF(ip_out, sizeof(ip_out), "%pi4 %pI6",
+ &ex_ipv4, &ex_ipv6);
+ /* Symbol lookup formatting */
+ sym_ret = BPF_SNPRINTF(sym_out, sizeof(sym_out), "%ps %pS %pB",
+ &schedule, &schedule, &schedule);
+ /* Kernel pointers */
+ addr_ret = BPF_SNPRINTF(addr_out, sizeof(addr_out), "%pK %px %p",
+ 0, 0xFFFF00000ADD4E55, 0xFFFF00000ADD4E55);
+ /* Strings embedding */
+ str_ret = BPF_SNPRINTF(str_out, sizeof(str_out), "%s %+05s",
+ str1, longstr);
+ /* Overflow */
+ over_ret = BPF_SNPRINTF(over_out, sizeof(over_out), "%%overflow");
+ /* Padding of fixed width numbers */
+ pad_ret = BPF_SNPRINTF(pad_out, sizeof(pad_out), "%5d %0900000X", 4, 4);
+ /* No args */
+ noarg_ret = BPF_SNPRINTF(noarg_out, sizeof(noarg_out), "simple case");
+ /* No buffer */
+ nobuf_ret = BPF_SNPRINTF(NULL, 0, "only interested in length %d", 60);
+
+ return 0;
+}
+
+char _license[] SEC("license") = "GPL";
--
2.31.1.295.g9ea45b61b8-goog
Two helpers (trace_printk and seq_printf) have very similar
implementations of format string parsing and a third one is coming
(snprintf). To avoid code duplication and make the code easier to
maintain, this moves the operations associated with format string
parsing (validation and argument sanitization) into one generic
function.
The implementation of the two existing helpers already drifted quite a
bit so unifying them entailed a lot of changes:
- bpf_trace_printk always expected fmt[fmt_size] to be the terminating
NULL character, this is no longer true, the first 0 is terminating.
- bpf_trace_printk now supports %% (which produces the percentage char).
- bpf_trace_printk now skips width formating fields.
- bpf_trace_printk now supports the X modifier (capital hexadecimal).
- bpf_trace_printk now supports %pK, %px, %pB, %pi4, %pI4, %pi6 and %pI6
- argument casting on 32 bit has been simplified into one macro and
using an enum instead of obscure int increments.
- bpf_seq_printf now uses bpf_trace_copy_string instead of
strncpy_from_kernel_nofault and handles the %pks %pus specifiers.
- bpf_seq_printf now prints longs correctly on 32 bit architectures.
- both were changed to use a global per-cpu tmp buffer instead of one
stack buffer for trace_printk and 6 small buffers for seq_printf.
- to avoid per-cpu buffer usage conflict, these helpers disable
preemption while the per-cpu buffer is in use.
- both helpers now support the %ps and %pS specifiers to print symbols.
Signed-off-by: Florent Revest <[email protected]>
---
kernel/trace/bpf_trace.c | 529 ++++++++++++++++++---------------------
1 file changed, 248 insertions(+), 281 deletions(-)
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 0d23755c2747..3ce9aeee6681 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -372,7 +372,7 @@ static const struct bpf_func_proto *bpf_get_probe_write_proto(void)
return &bpf_probe_write_user_proto;
}
-static void bpf_trace_copy_string(char *buf, void *unsafe_ptr, char fmt_ptype,
+static int bpf_trace_copy_string(char *buf, void *unsafe_ptr, char fmt_ptype,
size_t bufsz)
{
void __user *user_ptr = (__force void __user *)unsafe_ptr;
@@ -382,178 +382,292 @@ static void bpf_trace_copy_string(char *buf, void *unsafe_ptr, char fmt_ptype,
switch (fmt_ptype) {
case 's':
#ifdef CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
- if ((unsigned long)unsafe_ptr < TASK_SIZE) {
- strncpy_from_user_nofault(buf, user_ptr, bufsz);
- break;
- }
+ if ((unsigned long)unsafe_ptr < TASK_SIZE)
+ return strncpy_from_user_nofault(buf, user_ptr, bufsz);
fallthrough;
#endif
case 'k':
- strncpy_from_kernel_nofault(buf, unsafe_ptr, bufsz);
- break;
+ return strncpy_from_kernel_nofault(buf, unsafe_ptr, bufsz);
case 'u':
- strncpy_from_user_nofault(buf, user_ptr, bufsz);
- break;
+ return strncpy_from_user_nofault(buf, user_ptr, bufsz);
}
+
+ return -EINVAL;
}
static DEFINE_RAW_SPINLOCK(trace_printk_lock);
-#define BPF_TRACE_PRINTK_SIZE 1024
+enum bpf_printf_mod_type {
+ BPF_PRINTF_INT,
+ BPF_PRINTF_LONG,
+ BPF_PRINTF_LONG_LONG,
+};
+
+/* Workaround for getting va_list handling working with different argument type
+ * combinations generically for 32 and 64 bit archs.
+ */
+#define BPF_CAST_FMT_ARG(arg_nb, args, mod) \
+ (mod[arg_nb] == BPF_PRINTF_LONG_LONG || \
+ (mod[arg_nb] == BPF_PRINTF_LONG && __BITS_PER_LONG == 64) \
+ ? (u64)args[arg_nb] \
+ : (u32)args[arg_nb])
-static __printf(1, 0) int bpf_do_trace_printk(const char *fmt, ...)
+/* Per-cpu temp buffers which can be used by printf-like helpers for %s or %p
+ */
+#define MAX_PRINTF_BUF_LEN 512
+
+struct bpf_printf_buf {
+ char tmp_buf[MAX_PRINTF_BUF_LEN];
+};
+static DEFINE_PER_CPU(struct bpf_printf_buf, bpf_printf_buf);
+static DEFINE_PER_CPU(int, bpf_printf_buf_used);
+
+static int try_get_fmt_tmp_buf(char **tmp_buf)
{
- static char buf[BPF_TRACE_PRINTK_SIZE];
- unsigned long flags;
- va_list ap;
- int ret;
+ struct bpf_printf_buf *bufs = this_cpu_ptr(&bpf_printf_buf);
+ int used;
- raw_spin_lock_irqsave(&trace_printk_lock, flags);
- va_start(ap, fmt);
- ret = vsnprintf(buf, sizeof(buf), fmt, ap);
- va_end(ap);
- /* vsnprintf() will not append null for zero-length strings */
- if (ret == 0)
- buf[0] = '\0';
- trace_bpf_trace_printk(buf);
- raw_spin_unlock_irqrestore(&trace_printk_lock, flags);
+ if (*tmp_buf)
+ return 0;
- return ret;
+ preempt_disable();
+ used = this_cpu_inc_return(bpf_printf_buf_used);
+ if (WARN_ON_ONCE(used > 1)) {
+ this_cpu_dec(bpf_printf_buf_used);
+ return -EBUSY;
+ }
+ *tmp_buf = bufs->tmp_buf;
+
+ return 0;
+}
+
+static void put_fmt_tmp_buf(void)
+{
+ if (this_cpu_read(bpf_printf_buf_used)) {
+ this_cpu_dec(bpf_printf_buf_used);
+ preempt_enable();
+ }
}
/*
- * Only limited trace_printk() conversion specifiers allowed:
- * %d %i %u %x %ld %li %lu %lx %lld %lli %llu %llx %p %pB %pks %pus %s
+ * bpf_parse_fmt_str - Generic pass on format strings for printf-like helpers
+ *
+ * Returns a negative value if fmt is an invalid format string or 0 otherwise.
+ *
+ * This can be used in two ways:
+ * - Format string verification only: when final_args and mod are NULL
+ * - Arguments preparation: in addition to the above verification, it writes in
+ * final_args a copy of raw_args where pointers from BPF have been sanitized
+ * into pointers safe to use by snprintf. This also writes in the mod array
+ * the size requirement of each argument, usable by BPF_CAST_FMT_ARG for ex.
+ *
+ * In argument preparation mode, if 0 is returned, safe temporary buffers are
+ * allocated and put_fmt_tmp_buf should be called to free them after use.
*/
-BPF_CALL_5(bpf_trace_printk, char *, fmt, u32, fmt_size, u64, arg1,
- u64, arg2, u64, arg3)
-{
- int i, mod[3] = {}, fmt_cnt = 0;
- char buf[64], fmt_ptype;
- void *unsafe_ptr = NULL;
- bool str_seen = false;
+int bpf_printf_prepare(char *fmt, u32 fmt_size, const u64 *raw_args,
+ u64 *final_args, enum bpf_printf_mod_type *mod,
+ u32 num_args)
+{
+ int err, i, curr_specifier = 0, copy_size;
+ char *unsafe_ptr = NULL, *tmp_buf = NULL;
+ size_t tmp_buf_len = MAX_PRINTF_BUF_LEN;
+ enum bpf_printf_mod_type current_mod;
+ u64 current_arg;
+ char fmt_ptype;
+
+ if ((final_args && !mod) || (mod && !final_args))
+ return -EINVAL;
- /*
- * bpf_check()->check_func_arg()->check_stack_boundary()
- * guarantees that fmt points to bpf program stack,
- * fmt_size bytes of it were initialized and fmt_size > 0
- */
- if (fmt[--fmt_size] != 0)
+ fmt_size = (strnchr(fmt, fmt_size, 0) - fmt);
+ if (!fmt_size)
return -EINVAL;
- /* check format string for allowed specifiers */
for (i = 0; i < fmt_size; i++) {
- if ((!isprint(fmt[i]) && !isspace(fmt[i])) || !isascii(fmt[i]))
- return -EINVAL;
+ if ((!isprint(fmt[i]) && !isspace(fmt[i])) || !isascii(fmt[i])) {
+ err = -EINVAL;
+ goto out;
+ }
if (fmt[i] != '%')
continue;
- if (fmt_cnt >= 3)
- return -EINVAL;
+ if (fmt[i + 1] == '%') {
+ i++;
+ continue;
+ }
+
+ if (curr_specifier >= num_args) {
+ err = -EINVAL;
+ goto out;
+ }
/* fmt[i] != 0 && fmt[last] == 0, so we can access fmt[i + 1] */
i++;
- if (fmt[i] == 'l') {
- mod[fmt_cnt]++;
+
+ /* skip optional "[0 +-][num]" width formatting field */
+ while (fmt[i] == '0' || fmt[i] == '+' || fmt[i] == '-' ||
+ fmt[i] == ' ')
i++;
- } else if (fmt[i] == 'p') {
- mod[fmt_cnt]++;
- if ((fmt[i + 1] == 'k' ||
- fmt[i + 1] == 'u') &&
+ if (fmt[i] >= '1' && fmt[i] <= '9') {
+ i++;
+ while (fmt[i] >= '0' && fmt[i] <= '9')
+ i++;
+ }
+
+ if (fmt[i] == 'p') {
+ current_mod = BPF_PRINTF_LONG;
+
+ if ((fmt[i + 1] == 'k' || fmt[i + 1] == 'u') &&
fmt[i + 2] == 's') {
fmt_ptype = fmt[i + 1];
i += 2;
goto fmt_str;
}
- if (fmt[i + 1] == 'B') {
- i++;
+ if (fmt[i + 1] == 0 || isspace(fmt[i + 1]) ||
+ ispunct(fmt[i + 1]) || fmt[i + 1] == 'K' ||
+ fmt[i + 1] == 'x' || fmt[i + 1] == 'B' ||
+ fmt[i + 1] == 's' || fmt[i + 1] == 'S') {
+ /* just kernel pointers */
+ if (final_args)
+ current_arg = raw_args[curr_specifier];
goto fmt_next;
}
- /* disallow any further format extensions */
- if (fmt[i + 1] != 0 &&
- !isspace(fmt[i + 1]) &&
- !ispunct(fmt[i + 1]))
- return -EINVAL;
+ /* only support "%pI4", "%pi4", "%pI6" and "%pi6". */
+ if ((fmt[i + 1] != 'i' && fmt[i + 1] != 'I') ||
+ (fmt[i + 2] != '4' && fmt[i + 2] != '6')) {
+ err = -EINVAL;
+ goto out;
+ }
+
+ i += 2;
+ if (!final_args)
+ goto fmt_next;
+
+ if (try_get_fmt_tmp_buf(&tmp_buf)) {
+ err = -EBUSY;
+ goto out;
+ }
+
+ copy_size = (fmt[i + 2] == '4') ? 4 : 16;
+ if (tmp_buf_len < copy_size) {
+ err = -ENOSPC;
+ goto out;
+ }
+
+ unsafe_ptr = (char *)(long)raw_args[curr_specifier];
+ err = copy_from_kernel_nofault(tmp_buf, unsafe_ptr,
+ copy_size);
+ if (err < 0)
+ memset(tmp_buf, 0, copy_size);
+ current_arg = (u64)(long)tmp_buf;
+ tmp_buf += copy_size;
+ tmp_buf_len -= copy_size;
goto fmt_next;
} else if (fmt[i] == 's') {
- mod[fmt_cnt]++;
+ current_mod = BPF_PRINTF_LONG;
fmt_ptype = fmt[i];
fmt_str:
- if (str_seen)
- /* allow only one '%s' per fmt string */
- return -EINVAL;
- str_seen = true;
-
if (fmt[i + 1] != 0 &&
!isspace(fmt[i + 1]) &&
- !ispunct(fmt[i + 1]))
- return -EINVAL;
+ !ispunct(fmt[i + 1])) {
+ err = -EINVAL;
+ goto out;
+ }
- switch (fmt_cnt) {
- case 0:
- unsafe_ptr = (void *)(long)arg1;
- arg1 = (long)buf;
- break;
- case 1:
- unsafe_ptr = (void *)(long)arg2;
- arg2 = (long)buf;
- break;
- case 2:
- unsafe_ptr = (void *)(long)arg3;
- arg3 = (long)buf;
- break;
+ if (!final_args)
+ goto fmt_next;
+
+ if (try_get_fmt_tmp_buf(&tmp_buf)) {
+ err = -EBUSY;
+ goto out;
+ }
+
+ if (!tmp_buf_len) {
+ err = -ENOSPC;
+ goto out;
}
- bpf_trace_copy_string(buf, unsafe_ptr, fmt_ptype,
- sizeof(buf));
+ unsafe_ptr = (char *)(long)raw_args[curr_specifier];
+ err = bpf_trace_copy_string(tmp_buf, unsafe_ptr,
+ fmt_ptype, tmp_buf_len);
+ if (err < 0) {
+ tmp_buf[0] = '\0';
+ err = 1;
+ }
+
+ current_arg = (u64)(long)tmp_buf;
+ tmp_buf += err;
+ tmp_buf_len -= err;
+
goto fmt_next;
}
+ current_mod = BPF_PRINTF_INT;
+
+ if (fmt[i] == 'l') {
+ current_mod = BPF_PRINTF_LONG;
+ i++;
+ }
if (fmt[i] == 'l') {
- mod[fmt_cnt]++;
+ current_mod = BPF_PRINTF_LONG_LONG;
i++;
}
- if (fmt[i] != 'i' && fmt[i] != 'd' &&
- fmt[i] != 'u' && fmt[i] != 'x')
- return -EINVAL;
+ if (fmt[i] != 'i' && fmt[i] != 'd' && fmt[i] != 'u' &&
+ fmt[i] != 'x' && fmt[i] != 'X') {
+ err = -EINVAL;
+ goto out;
+ }
+
+ if (final_args)
+ current_arg = raw_args[curr_specifier];
fmt_next:
- fmt_cnt++;
+ if (final_args) {
+ mod[curr_specifier] = current_mod;
+ final_args[curr_specifier] = current_arg;
+ }
+ curr_specifier++;
}
-/* Horrid workaround for getting va_list handling working with different
- * argument type combinations generically for 32 and 64 bit archs.
- */
-#define __BPF_TP_EMIT() __BPF_ARG3_TP()
-#define __BPF_TP(...) \
- bpf_do_trace_printk(fmt, ##__VA_ARGS__)
-
-#define __BPF_ARG1_TP(...) \
- ((mod[0] == 2 || (mod[0] == 1 && __BITS_PER_LONG == 64)) \
- ? __BPF_TP(arg1, ##__VA_ARGS__) \
- : ((mod[0] == 1 || (mod[0] == 0 && __BITS_PER_LONG == 32)) \
- ? __BPF_TP((long)arg1, ##__VA_ARGS__) \
- : __BPF_TP((u32)arg1, ##__VA_ARGS__)))
-
-#define __BPF_ARG2_TP(...) \
- ((mod[1] == 2 || (mod[1] == 1 && __BITS_PER_LONG == 64)) \
- ? __BPF_ARG1_TP(arg2, ##__VA_ARGS__) \
- : ((mod[1] == 1 || (mod[1] == 0 && __BITS_PER_LONG == 32)) \
- ? __BPF_ARG1_TP((long)arg2, ##__VA_ARGS__) \
- : __BPF_ARG1_TP((u32)arg2, ##__VA_ARGS__)))
-
-#define __BPF_ARG3_TP(...) \
- ((mod[2] == 2 || (mod[2] == 1 && __BITS_PER_LONG == 64)) \
- ? __BPF_ARG2_TP(arg3, ##__VA_ARGS__) \
- : ((mod[2] == 1 || (mod[2] == 0 && __BITS_PER_LONG == 32)) \
- ? __BPF_ARG2_TP((long)arg3, ##__VA_ARGS__) \
- : __BPF_ARG2_TP((u32)arg3, ##__VA_ARGS__)))
-
- return __BPF_TP_EMIT();
+ err = 0;
+out:
+ put_fmt_tmp_buf();
+ return err;
+}
+
+#define MAX_TRACE_PRINTK_VARARGS 3
+#define BPF_TRACE_PRINTK_SIZE 1024
+
+BPF_CALL_5(bpf_trace_printk, char *, fmt, u32, fmt_size, u64, arg1,
+ u64, arg2, u64, arg3)
+{
+ u64 args[MAX_TRACE_PRINTK_VARARGS] = { arg1, arg2, arg3 };
+ enum bpf_printf_mod_type mod[MAX_TRACE_PRINTK_VARARGS];
+ static char buf[BPF_TRACE_PRINTK_SIZE];
+ unsigned long flags;
+ int ret;
+
+ ret = bpf_printf_prepare(fmt, fmt_size, args, args, mod,
+ MAX_TRACE_PRINTK_VARARGS);
+ if (ret < 0)
+ return ret;
+
+ ret = snprintf(buf, sizeof(buf), fmt, BPF_CAST_FMT_ARG(0, args, mod),
+ BPF_CAST_FMT_ARG(1, args, mod), BPF_CAST_FMT_ARG(2, args, mod));
+ /* snprintf() will not append null for zero-length strings */
+ if (ret == 0)
+ buf[0] = '\0';
+
+ raw_spin_lock_irqsave(&trace_printk_lock, flags);
+ trace_bpf_trace_printk(buf);
+ raw_spin_unlock_irqrestore(&trace_printk_lock, flags);
+
+ put_fmt_tmp_buf();
+
+ return ret;
}
static const struct bpf_func_proto bpf_trace_printk_proto = {
@@ -581,184 +695,37 @@ const struct bpf_func_proto *bpf_get_trace_printk_proto(void)
}
#define MAX_SEQ_PRINTF_VARARGS 12
-#define MAX_SEQ_PRINTF_MAX_MEMCPY 6
-#define MAX_SEQ_PRINTF_STR_LEN 128
-
-struct bpf_seq_printf_buf {
- char buf[MAX_SEQ_PRINTF_MAX_MEMCPY][MAX_SEQ_PRINTF_STR_LEN];
-};
-static DEFINE_PER_CPU(struct bpf_seq_printf_buf, bpf_seq_printf_buf);
-static DEFINE_PER_CPU(int, bpf_seq_printf_buf_used);
BPF_CALL_5(bpf_seq_printf, struct seq_file *, m, char *, fmt, u32, fmt_size,
const void *, data, u32, data_len)
{
- int err = -EINVAL, fmt_cnt = 0, memcpy_cnt = 0;
- int i, buf_used, copy_size, num_args;
- u64 params[MAX_SEQ_PRINTF_VARARGS];
- struct bpf_seq_printf_buf *bufs;
- const u64 *args = data;
-
- buf_used = this_cpu_inc_return(bpf_seq_printf_buf_used);
- if (WARN_ON_ONCE(buf_used > 1)) {
- err = -EBUSY;
- goto out;
- }
-
- bufs = this_cpu_ptr(&bpf_seq_printf_buf);
-
- /*
- * bpf_check()->check_func_arg()->check_stack_boundary()
- * guarantees that fmt points to bpf program stack,
- * fmt_size bytes of it were initialized and fmt_size > 0
- */
- if (fmt[--fmt_size] != 0)
- goto out;
-
- if (data_len & 7)
- goto out;
-
- for (i = 0; i < fmt_size; i++) {
- if (fmt[i] == '%') {
- if (fmt[i + 1] == '%')
- i++;
- else if (!data || !data_len)
- goto out;
- }
- }
+ enum bpf_printf_mod_type mod[MAX_SEQ_PRINTF_VARARGS];
+ u64 args[MAX_SEQ_PRINTF_VARARGS];
+ int err, num_args;
+ if (data_len & 7 || data_len > MAX_SEQ_PRINTF_VARARGS * 8 ||
+ (data_len && !data))
+ return -EINVAL;
num_args = data_len / 8;
- /* check format string for allowed specifiers */
- for (i = 0; i < fmt_size; i++) {
- /* only printable ascii for now. */
- if ((!isprint(fmt[i]) && !isspace(fmt[i])) || !isascii(fmt[i])) {
- err = -EINVAL;
- goto out;
- }
-
- if (fmt[i] != '%')
- continue;
-
- if (fmt[i + 1] == '%') {
- i++;
- continue;
- }
-
- if (fmt_cnt >= MAX_SEQ_PRINTF_VARARGS) {
- err = -E2BIG;
- goto out;
- }
-
- if (fmt_cnt >= num_args) {
- err = -EINVAL;
- goto out;
- }
-
- /* fmt[i] != 0 && fmt[last] == 0, so we can access fmt[i + 1] */
- i++;
-
- /* skip optional "[0 +-][num]" width formating field */
- while (fmt[i] == '0' || fmt[i] == '+' || fmt[i] == '-' ||
- fmt[i] == ' ')
- i++;
- if (fmt[i] >= '1' && fmt[i] <= '9') {
- i++;
- while (fmt[i] >= '0' && fmt[i] <= '9')
- i++;
- }
-
- if (fmt[i] == 's') {
- void *unsafe_ptr;
-
- /* try our best to copy */
- if (memcpy_cnt >= MAX_SEQ_PRINTF_MAX_MEMCPY) {
- err = -E2BIG;
- goto out;
- }
-
- unsafe_ptr = (void *)(long)args[fmt_cnt];
- err = strncpy_from_kernel_nofault(bufs->buf[memcpy_cnt],
- unsafe_ptr, MAX_SEQ_PRINTF_STR_LEN);
- if (err < 0)
- bufs->buf[memcpy_cnt][0] = '\0';
- params[fmt_cnt] = (u64)(long)bufs->buf[memcpy_cnt];
-
- fmt_cnt++;
- memcpy_cnt++;
- continue;
- }
-
- if (fmt[i] == 'p') {
- if (fmt[i + 1] == 0 ||
- fmt[i + 1] == 'K' ||
- fmt[i + 1] == 'x' ||
- fmt[i + 1] == 'B') {
- /* just kernel pointers */
- params[fmt_cnt] = args[fmt_cnt];
- fmt_cnt++;
- continue;
- }
-
- /* only support "%pI4", "%pi4", "%pI6" and "%pi6". */
- if (fmt[i + 1] != 'i' && fmt[i + 1] != 'I') {
- err = -EINVAL;
- goto out;
- }
- if (fmt[i + 2] != '4' && fmt[i + 2] != '6') {
- err = -EINVAL;
- goto out;
- }
-
- if (memcpy_cnt >= MAX_SEQ_PRINTF_MAX_MEMCPY) {
- err = -E2BIG;
- goto out;
- }
-
-
- copy_size = (fmt[i + 2] == '4') ? 4 : 16;
-
- err = copy_from_kernel_nofault(bufs->buf[memcpy_cnt],
- (void *) (long) args[fmt_cnt],
- copy_size);
- if (err < 0)
- memset(bufs->buf[memcpy_cnt], 0, copy_size);
- params[fmt_cnt] = (u64)(long)bufs->buf[memcpy_cnt];
-
- i += 2;
- fmt_cnt++;
- memcpy_cnt++;
- continue;
- }
-
- if (fmt[i] == 'l') {
- i++;
- if (fmt[i] == 'l')
- i++;
- }
-
- if (fmt[i] != 'i' && fmt[i] != 'd' &&
- fmt[i] != 'u' && fmt[i] != 'x' &&
- fmt[i] != 'X') {
- err = -EINVAL;
- goto out;
- }
-
- params[fmt_cnt] = args[fmt_cnt];
- fmt_cnt++;
- }
+ err = bpf_printf_prepare(fmt, fmt_size, data, args, mod, num_args);
+ if (err < 0)
+ return err;
/* Maximumly we can have MAX_SEQ_PRINTF_VARARGS parameter, just give
* all of them to seq_printf().
*/
- seq_printf(m, fmt, params[0], params[1], params[2], params[3],
- params[4], params[5], params[6], params[7], params[8],
- params[9], params[10], params[11]);
+ seq_printf(m, fmt, BPF_CAST_FMT_ARG(0, args, mod),
+ BPF_CAST_FMT_ARG(1, args, mod), BPF_CAST_FMT_ARG(2, args, mod),
+ BPF_CAST_FMT_ARG(3, args, mod), BPF_CAST_FMT_ARG(4, args, mod),
+ BPF_CAST_FMT_ARG(5, args, mod), BPF_CAST_FMT_ARG(6, args, mod),
+ BPF_CAST_FMT_ARG(7, args, mod), BPF_CAST_FMT_ARG(8, args, mod),
+ BPF_CAST_FMT_ARG(9, args, mod), BPF_CAST_FMT_ARG(10, args, mod),
+ BPF_CAST_FMT_ARG(11, args, mod));
- err = seq_has_overflowed(m) ? -EOVERFLOW : 0;
-out:
- this_cpu_dec(bpf_seq_printf_buf_used);
- return err;
+ put_fmt_tmp_buf();
+
+ return seq_has_overflowed(m) ? -EOVERFLOW : 0;
}
BTF_ID_LIST_SINGLE(btf_seq_file_ids, struct, seq_file)
--
2.31.1.295.g9ea45b61b8-goog
When initializing the __param array with a one liner, if all args are
const, the initial array value will be placed in the rodata section but
because libbpf does not support relocation in the rodata section, any
pointer in this array will stay NULL.
Fixes: c09add2fbc5a ("tools/libbpf: Add bpf_iter support")
Signed-off-by: Florent Revest <[email protected]>
---
tools/lib/bpf/bpf_tracing.h | 40 +++++++++++++++++++++++++++----------
1 file changed, 29 insertions(+), 11 deletions(-)
diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
index f9ef37707888..1c2e91ee041d 100644
--- a/tools/lib/bpf/bpf_tracing.h
+++ b/tools/lib/bpf/bpf_tracing.h
@@ -413,20 +413,38 @@ 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...) \
- ({ \
- _Pragma("GCC diagnostic push") \
- _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
- static const char ___fmt[] = fmt; \
- unsigned long long ___param[] = { args }; \
- _Pragma("GCC diagnostic pop") \
- int ___ret = bpf_seq_printf(seq, ___fmt, sizeof(___fmt), \
- ___param, sizeof(___param)); \
- ___ret; \
- })
+#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)); \
+})
#endif
--
2.31.1.295.g9ea45b61b8-goog
Hi Florent,
I love your patch! Yet something to improve:
[auto build test ERROR on bpf-next/master]
url: https://github.com/0day-ci/linux/commits/Florent-Revest/Add-a-snprintf-eBPF-helper/20210412-233921
base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: m68k-allmodconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/168301dda58989eca274d5f5c926675540010e13
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Florent-Revest/Add-a-snprintf-eBPF-helper/20210412-233921
git checkout 168301dda58989eca274d5f5c926675540010e13
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=m68k
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
m68k-linux-ld: kernel/bpf/verifier.o: in function `check_helper_call.isra.0':
>> verifier.c:(.text+0xf79e): undefined reference to `bpf_printf_prepare'
m68k-linux-ld: kernel/bpf/helpers.o: in function `bpf_base_func_proto':
>> helpers.c:(.text+0xd82): undefined reference to `bpf_snprintf_proto'
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Hi Florent,
I love your patch! Yet something to improve:
[auto build test ERROR on bpf-next/master]
url: https://github.com/0day-ci/linux/commits/Florent-Revest/Add-a-snprintf-eBPF-helper/20210412-233921
base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: mips-decstation_r4k_defconfig (attached as .config)
compiler: mipsel-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/168301dda58989eca274d5f5c926675540010e13
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Florent-Revest/Add-a-snprintf-eBPF-helper/20210412-233921
git checkout 168301dda58989eca274d5f5c926675540010e13
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=mips
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
mipsel-linux-ld: kernel/bpf/verifier.o: in function `check_helper_call.isra.0':
verifier.c:(.text+0x13bac): undefined reference to `bpf_printf_prepare'
mipsel-linux-ld: kernel/bpf/helpers.o: in function `bpf_base_func_proto':
helpers.c:(.text+0x9c8): undefined reference to `bpf_snprintf_proto'
>> mipsel-linux-ld: helpers.c:(.text+0x9d0): undefined reference to `bpf_snprintf_proto'
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Hi Florent,
I love your patch! Perhaps something to improve:
[auto build test WARNING on bpf-next/master]
url: https://github.com/0day-ci/linux/commits/Florent-Revest/Add-a-snprintf-eBPF-helper/20210412-233921
base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: i386-randconfig-s002-20210412 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.3-280-g2cd6d34e-dirty
# https://github.com/0day-ci/linux/commit/168301dda58989eca274d5f5c926675540010e13
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Florent-Revest/Add-a-snprintf-eBPF-helper/20210412-233921
git checkout 168301dda58989eca274d5f5c926675540010e13
# save the attached .config to linux build tree
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=i386
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
sparse warnings: (new ones prefixed by >>)
kernel/bpf/verifier.c:1674:41: sparse: sparse: subtraction of functions? Share your drugs
kernel/bpf/verifier.c:12051:76: sparse: sparse: subtraction of functions? Share your drugs
kernel/bpf/verifier.c:12489:81: sparse: sparse: subtraction of functions? Share your drugs
kernel/bpf/verifier.c:12493:81: sparse: sparse: subtraction of functions? Share your drugs
kernel/bpf/verifier.c:12497:81: sparse: sparse: subtraction of functions? Share your drugs
kernel/bpf/verifier.c:12501:79: sparse: sparse: subtraction of functions? Share your drugs
kernel/bpf/verifier.c:12505:78: sparse: sparse: subtraction of functions? Share your drugs
kernel/bpf/verifier.c:12509:79: sparse: sparse: subtraction of functions? Share your drugs
kernel/bpf/verifier.c:12513:78: sparse: sparse: subtraction of functions? Share your drugs
kernel/bpf/verifier.c:12557:38: sparse: sparse: subtraction of functions? Share your drugs
>> kernel/bpf/verifier.c:5944:23: sparse: sparse: non size-preserving integer to pointer cast
vim +5944 kernel/bpf/verifier.c
5920
5921 static int check_bpf_snprintf_call(struct bpf_verifier_env *env,
5922 struct bpf_reg_state *regs)
5923 {
5924 struct bpf_reg_state *fmt_reg = ®s[BPF_REG_3];
5925 struct bpf_reg_state *data_len_reg = ®s[BPF_REG_5];
5926 struct bpf_map *fmt_map = fmt_reg->map_ptr;
5927 int err, fmt_map_off, num_args;
5928 u64 fmt_addr;
5929 char *fmt;
5930
5931 /* data must be an array of u64 */
5932 if (data_len_reg->var_off.value % 8)
5933 return -EINVAL;
5934 num_args = data_len_reg->var_off.value / 8;
5935
5936 /* fmt being ARG_PTR_TO_CONST_STR guarantees that var_off is const
5937 * and map_direct_value_addr is set.
5938 */
5939 fmt_map_off = fmt_reg->off + fmt_reg->var_off.value;
5940 err = fmt_map->ops->map_direct_value_addr(fmt_map, &fmt_addr,
5941 fmt_map_off);
5942 if (err)
5943 return err;
> 5944 fmt = (char *)fmt_addr + fmt_map_off;
5945
5946 /* We are also guaranteed that fmt+fmt_map_off is NULL terminated, we
5947 * can focus on validating the format specifiers.
5948 */
5949 err = bpf_printf_prepare(fmt, UINT_MAX, NULL, NULL, NULL, num_args);
5950 if (err < 0)
5951 verbose(env, "Invalid format string\n");
5952
5953 return err;
5954 }
5955
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
On Mon, Apr 12, 2021 at 8:38 AM Florent Revest <[email protected]> wrote:
>
> Two helpers (trace_printk and seq_printf) have very similar
> implementations of format string parsing and a third one is coming
> (snprintf). To avoid code duplication and make the code easier to
> maintain, this moves the operations associated with format string
> parsing (validation and argument sanitization) into one generic
> function.
>
> The implementation of the two existing helpers already drifted quite a
> bit so unifying them entailed a lot of changes:
>
> - bpf_trace_printk always expected fmt[fmt_size] to be the terminating
> NULL character, this is no longer true, the first 0 is terminating.
> - bpf_trace_printk now supports %% (which produces the percentage char).
> - bpf_trace_printk now skips width formating fields.
> - bpf_trace_printk now supports the X modifier (capital hexadecimal).
> - bpf_trace_printk now supports %pK, %px, %pB, %pi4, %pI4, %pi6 and %pI6
> - argument casting on 32 bit has been simplified into one macro and
> using an enum instead of obscure int increments.
>
> - bpf_seq_printf now uses bpf_trace_copy_string instead of
> strncpy_from_kernel_nofault and handles the %pks %pus specifiers.
> - bpf_seq_printf now prints longs correctly on 32 bit architectures.
>
> - both were changed to use a global per-cpu tmp buffer instead of one
> stack buffer for trace_printk and 6 small buffers for seq_printf.
> - to avoid per-cpu buffer usage conflict, these helpers disable
> preemption while the per-cpu buffer is in use.
> - both helpers now support the %ps and %pS specifiers to print symbols.
>
> Signed-off-by: Florent Revest <[email protected]>
> ---
> kernel/trace/bpf_trace.c | 529 ++++++++++++++++++---------------------
> 1 file changed, 248 insertions(+), 281 deletions(-)
>
[...]
> +/* Per-cpu temp buffers which can be used by printf-like helpers for %s or %p
> + */
> +#define MAX_PRINTF_BUF_LEN 512
> +
> +struct bpf_printf_buf {
> + char tmp_buf[MAX_PRINTF_BUF_LEN];
> +};
> +static DEFINE_PER_CPU(struct bpf_printf_buf, bpf_printf_buf);
> +static DEFINE_PER_CPU(int, bpf_printf_buf_used);
> +
> +static int try_get_fmt_tmp_buf(char **tmp_buf)
> {
> - static char buf[BPF_TRACE_PRINTK_SIZE];
> - unsigned long flags;
> - va_list ap;
> - int ret;
> + struct bpf_printf_buf *bufs = this_cpu_ptr(&bpf_printf_buf);
why doing this_cpu_ptr() if below (if *tmp_buf case), you will not use
it. just a waste of CPU, no?
> + int used;
>
> - raw_spin_lock_irqsave(&trace_printk_lock, flags);
> - va_start(ap, fmt);
> - ret = vsnprintf(buf, sizeof(buf), fmt, ap);
> - va_end(ap);
> - /* vsnprintf() will not append null for zero-length strings */
> - if (ret == 0)
> - buf[0] = '\0';
> - trace_bpf_trace_printk(buf);
> - raw_spin_unlock_irqrestore(&trace_printk_lock, flags);
> + if (*tmp_buf)
> + return 0;
>
> - return ret;
> + preempt_disable();
> + used = this_cpu_inc_return(bpf_printf_buf_used);
> + if (WARN_ON_ONCE(used > 1)) {
> + this_cpu_dec(bpf_printf_buf_used);
> + return -EBUSY;
> + }
get bufs pointer here instead?
> + *tmp_buf = bufs->tmp_buf;
> +
> + return 0;
> +}
> +
> +static void put_fmt_tmp_buf(void)
> +{
> + if (this_cpu_read(bpf_printf_buf_used)) {
> + this_cpu_dec(bpf_printf_buf_used);
> + preempt_enable();
> + }
> }
>
> /*
> - * Only limited trace_printk() conversion specifiers allowed:
> - * %d %i %u %x %ld %li %lu %lx %lld %lli %llu %llx %p %pB %pks %pus %s
> + * bpf_parse_fmt_str - Generic pass on format strings for printf-like helpers
> + *
> + * Returns a negative value if fmt is an invalid format string or 0 otherwise.
> + *
> + * This can be used in two ways:
> + * - Format string verification only: when final_args and mod are NULL
> + * - Arguments preparation: in addition to the above verification, it writes in
> + * final_args a copy of raw_args where pointers from BPF have been sanitized
> + * into pointers safe to use by snprintf. This also writes in the mod array
> + * the size requirement of each argument, usable by BPF_CAST_FMT_ARG for ex.
> + *
> + * In argument preparation mode, if 0 is returned, safe temporary buffers are
> + * allocated and put_fmt_tmp_buf should be called to free them after use.
> */
> -BPF_CALL_5(bpf_trace_printk, char *, fmt, u32, fmt_size, u64, arg1,
> - u64, arg2, u64, arg3)
> -{
> - int i, mod[3] = {}, fmt_cnt = 0;
> - char buf[64], fmt_ptype;
> - void *unsafe_ptr = NULL;
> - bool str_seen = false;
> +int bpf_printf_prepare(char *fmt, u32 fmt_size, const u64 *raw_args,
> + u64 *final_args, enum bpf_printf_mod_type *mod,
> + u32 num_args)
> +{
> + int err, i, curr_specifier = 0, copy_size;
> + char *unsafe_ptr = NULL, *tmp_buf = NULL;
> + size_t tmp_buf_len = MAX_PRINTF_BUF_LEN;
> + enum bpf_printf_mod_type current_mod;
> + u64 current_arg;
naming consistency: current_arg vs curr_specifier? maybe just cur_arg
and cur_spec?
> + char fmt_ptype;
> +
> + if ((final_args && !mod) || (mod && !final_args))
nit: same check:
if (!!final_args != !!mod)
> + return -EINVAL;
>
> - /*
> - * bpf_check()->check_func_arg()->check_stack_boundary()
> - * guarantees that fmt points to bpf program stack,
> - * fmt_size bytes of it were initialized and fmt_size > 0
> - */
> - if (fmt[--fmt_size] != 0)
> + fmt_size = (strnchr(fmt, fmt_size, 0) - fmt);
extra ()
> + if (!fmt_size)
hm... strnchr() will return NULL if the character is not found, so
fmt_size will be some non-zero value (due to - fmt), how is this
supposed to work?
some negative tests are clearly missing, it seems, if you didn't catch this
> return -EINVAL;
>
> - /* check format string for allowed specifiers */
> for (i = 0; i < fmt_size; i++) {
> - if ((!isprint(fmt[i]) && !isspace(fmt[i])) || !isascii(fmt[i]))
> - return -EINVAL;
> + if ((!isprint(fmt[i]) && !isspace(fmt[i])) || !isascii(fmt[i])) {
> + err = -EINVAL;
> + goto out;
> + }
>
> if (fmt[i] != '%')
> continue;
>
> - if (fmt_cnt >= 3)
> - return -EINVAL;
> + if (fmt[i + 1] == '%') {
> + i++;
> + continue;
> + }
> +
> + if (curr_specifier >= num_args) {
> + err = -EINVAL;
> + goto out;
> + }
>
> /* fmt[i] != 0 && fmt[last] == 0, so we can access fmt[i + 1] */
a bit outdated comment, last doesn't exist anymore. I think the
comment is trying to say that fmt[i + 1] can be read because in the
worst case it will be a final zero terminator (which we checked
above).
> i++;
> - if (fmt[i] == 'l') {
> - mod[fmt_cnt]++;
> +
[...]
> + err = 0;
> +out:
> + put_fmt_tmp_buf();
so you are putting tmp_buf unconditionally, even when there was no
error. That seems wrong? Should this be:
if (err)
put_fmt_tmp_buf()
?
> + return err;
> +}
> +
[...]
On Mon, Apr 12, 2021 at 8:38 AM Florent Revest <[email protected]> wrote:
>
> Similarly to BPF_SEQ_PRINTF, this macro turns variadic arguments into an
> array of u64, making it more natural to call the bpf_snprintf helper.
>
> Signed-off-by: Florent Revest <[email protected]>
> ---
Nice!
Acked-by: Andrii Nakryiko <[email protected]>
> tools/lib/bpf/bpf_tracing.h | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
> index 1c2e91ee041d..8c954ebc0c7c 100644
> --- a/tools/lib/bpf/bpf_tracing.h
> +++ b/tools/lib/bpf/bpf_tracing.h
> @@ -447,4 +447,22 @@ static __always_inline typeof(name(0)) ____##name(struct pt_regs *ctx, ##args)
> ___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
> --
> 2.31.1.295.g9ea45b61b8-goog
>
On Mon, Apr 12, 2021 at 8:38 AM Florent Revest <[email protected]> wrote:
>
> The implementation takes inspiration from the existing bpf_trace_printk
> helper but there are a few differences:
>
> To allow for a large number of format-specifiers, parameters are
> provided in an array, like in bpf_seq_printf.
>
> Because the output string takes two arguments and the array of
> parameters also takes two arguments, the format string needs to fit in
> one argument. Thankfully, ARG_PTR_TO_CONST_STR is guaranteed to point to
> a zero-terminated read-only map so we don't need a format string length
> arg.
>
> Because the format-string is known at verification time, we also do
> a first pass of format string validation in the verifier logic. This
> makes debugging easier.
>
> Signed-off-by: Florent Revest <[email protected]>
> ---
> include/linux/bpf.h | 6 ++++
> include/uapi/linux/bpf.h | 28 +++++++++++++++++++
> kernel/bpf/helpers.c | 2 ++
> kernel/bpf/verifier.c | 41 ++++++++++++++++++++++++++++
> kernel/trace/bpf_trace.c | 50 ++++++++++++++++++++++++++++++++++
> tools/include/uapi/linux/bpf.h | 28 +++++++++++++++++++
> 6 files changed, 155 insertions(+)
>
[...]
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 5f46dd6f3383..d4020e5f91ee 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -5918,6 +5918,41 @@ static int check_reference_leak(struct bpf_verifier_env *env)
> return state->acquired_refs ? -EINVAL : 0;
> }
>
> +static int check_bpf_snprintf_call(struct bpf_verifier_env *env,
> + struct bpf_reg_state *regs)
> +{
> + struct bpf_reg_state *fmt_reg = ®s[BPF_REG_3];
> + struct bpf_reg_state *data_len_reg = ®s[BPF_REG_5];
> + struct bpf_map *fmt_map = fmt_reg->map_ptr;
> + int err, fmt_map_off, num_args;
> + u64 fmt_addr;
> + char *fmt;
> +
> + /* data must be an array of u64 */
> + if (data_len_reg->var_off.value % 8)
> + return -EINVAL;
> + num_args = data_len_reg->var_off.value / 8;
> +
> + /* fmt being ARG_PTR_TO_CONST_STR guarantees that var_off is const
> + * and map_direct_value_addr is set.
> + */
> + fmt_map_off = fmt_reg->off + fmt_reg->var_off.value;
> + err = fmt_map->ops->map_direct_value_addr(fmt_map, &fmt_addr,
> + fmt_map_off);
> + if (err)
> + return err;
> + fmt = (char *)fmt_addr + fmt_map_off;
> +
bot complained about lack of (long) cast before fmt_addr, please address
[...]
> + /* Maximumly we can have MAX_SNPRINTF_VARARGS parameters, just give
> + * all of them to snprintf().
> + */
> + err = snprintf(str, str_size, fmt, BPF_CAST_FMT_ARG(0, args, mod),
> + BPF_CAST_FMT_ARG(1, args, mod), BPF_CAST_FMT_ARG(2, args, mod),
> + BPF_CAST_FMT_ARG(3, args, mod), BPF_CAST_FMT_ARG(4, args, mod),
> + BPF_CAST_FMT_ARG(5, args, mod), BPF_CAST_FMT_ARG(6, args, mod),
> + BPF_CAST_FMT_ARG(7, args, mod), BPF_CAST_FMT_ARG(8, args, mod),
> + BPF_CAST_FMT_ARG(9, args, mod), BPF_CAST_FMT_ARG(10, args, mod),
> + BPF_CAST_FMT_ARG(11, args, mod));
> +
> + put_fmt_tmp_buf();
reading this for at least 3rd time, this put_fmt_tmp_buf() looks a bit
out of place and kind of random. I think bpf_printf_cleanup() name
pairs with bpf_printf_prepare() better.
> +
> + return err + 1;
snprintf() already returns string length *including* terminating zero,
so this is wrong
> +}
> +
[...]
On Mon, Apr 12, 2021 at 8:38 AM Florent Revest <[email protected]> wrote:
>
> When initializing the __param array with a one liner, if all args are
> const, the initial array value will be placed in the rodata section but
> because libbpf does not support relocation in the rodata section, any
> pointer in this array will stay NULL.
>
> Fixes: c09add2fbc5a ("tools/libbpf: Add bpf_iter support")
> Signed-off-by: Florent Revest <[email protected]>
> ---
Looks good!
Acked-by: Andrii Nakryiko <[email protected]>
> tools/lib/bpf/bpf_tracing.h | 40 +++++++++++++++++++++++++++----------
> 1 file changed, 29 insertions(+), 11 deletions(-)
>
[...]
On Mon, Apr 12, 2021 at 8:38 AM Florent Revest <[email protected]> wrote:
>
> This exercises most of the format specifiers.
>
> Signed-off-by: Florent Revest <[email protected]>
> Acked-by: Andrii Nakryiko <[email protected]>
> ---
As I mentioned on another patch, we probably need negative tests even
more than positive ones.
I think an easy and nice way to do this is to have a separate BPF
skeleton where fmt string and arguments are provided through read-only
global variables, so that user-space can re-use the same BPF skeleton
to simulate multiple cases. BPF program itself would just call
bpf_snprintf() and store the returned result.
Whether we need to validate the verifier log is up to debate (though
it's not that hard to do by overriding libbpf_print_fn() callback),
I'd be ok at least knowing that some bad format strings are rejected
and don't crash the kernel.
> .../selftests/bpf/prog_tests/snprintf.c | 81 +++++++++++++++++++
> .../selftests/bpf/progs/test_snprintf.c | 74 +++++++++++++++++
> 2 files changed, 155 insertions(+)
> create mode 100644 tools/testing/selftests/bpf/prog_tests/snprintf.c
> create mode 100644 tools/testing/selftests/bpf/progs/test_snprintf.c
>
[...]
On Wed, Apr 14, 2021 at 1:21 AM Andrii Nakryiko
<[email protected]> wrote:
>
> On Mon, Apr 12, 2021 at 8:38 AM Florent Revest <[email protected]> wrote:
> >
> > This exercises most of the format specifiers.
> >
> > Signed-off-by: Florent Revest <[email protected]>
> > Acked-by: Andrii Nakryiko <[email protected]>
> > ---
>
> As I mentioned on another patch, we probably need negative tests even
> more than positive ones.
Agreed.
> I think an easy and nice way to do this is to have a separate BPF
> skeleton where fmt string and arguments are provided through read-only
> global variables, so that user-space can re-use the same BPF skeleton
> to simulate multiple cases. BPF program itself would just call
> bpf_snprintf() and store the returned result.
Ah, great idea! I was thinking of having one skeleton for each but it
would be a bit much indeed.
Because the format string needs to be in a read only map though, I
hope it can be modified from userspace before loading. I'll try it out
and see :) if it doesn't work I'll just use more skeletons
> Whether we need to validate the verifier log is up to debate (though
> it's not that hard to do by overriding libbpf_print_fn() callback),
> I'd be ok at least knowing that some bad format strings are rejected
> and don't crash the kernel.
Alright :)
>
> > .../selftests/bpf/prog_tests/snprintf.c | 81 +++++++++++++++++++
> > .../selftests/bpf/progs/test_snprintf.c | 74 +++++++++++++++++
> > 2 files changed, 155 insertions(+)
> > create mode 100644 tools/testing/selftests/bpf/prog_tests/snprintf.c
> > create mode 100644 tools/testing/selftests/bpf/progs/test_snprintf.c
> >
>
> [...]
On Wed, Apr 14, 2021 at 1:16 AM Andrii Nakryiko
<[email protected]> wrote:
> On Mon, Apr 12, 2021 at 8:38 AM Florent Revest <[email protected]> wrote:
> > +static int check_bpf_snprintf_call(struct bpf_verifier_env *env,
> > + struct bpf_reg_state *regs)
> > +{
> > + struct bpf_reg_state *fmt_reg = ®s[BPF_REG_3];
> > + struct bpf_reg_state *data_len_reg = ®s[BPF_REG_5];
> > + struct bpf_map *fmt_map = fmt_reg->map_ptr;
> > + int err, fmt_map_off, num_args;
> > + u64 fmt_addr;
> > + char *fmt;
> > +
> > + /* data must be an array of u64 */
> > + if (data_len_reg->var_off.value % 8)
> > + return -EINVAL;
> > + num_args = data_len_reg->var_off.value / 8;
> > +
> > + /* fmt being ARG_PTR_TO_CONST_STR guarantees that var_off is const
> > + * and map_direct_value_addr is set.
> > + */
> > + fmt_map_off = fmt_reg->off + fmt_reg->var_off.value;
> > + err = fmt_map->ops->map_direct_value_addr(fmt_map, &fmt_addr,
> > + fmt_map_off);
> > + if (err)
> > + return err;
> > + fmt = (char *)fmt_addr + fmt_map_off;
> > +
>
> bot complained about lack of (long) cast before fmt_addr, please address
Will do.
> > + /* Maximumly we can have MAX_SNPRINTF_VARARGS parameters, just give
> > + * all of them to snprintf().
> > + */
> > + err = snprintf(str, str_size, fmt, BPF_CAST_FMT_ARG(0, args, mod),
> > + BPF_CAST_FMT_ARG(1, args, mod), BPF_CAST_FMT_ARG(2, args, mod),
> > + BPF_CAST_FMT_ARG(3, args, mod), BPF_CAST_FMT_ARG(4, args, mod),
> > + BPF_CAST_FMT_ARG(5, args, mod), BPF_CAST_FMT_ARG(6, args, mod),
> > + BPF_CAST_FMT_ARG(7, args, mod), BPF_CAST_FMT_ARG(8, args, mod),
> > + BPF_CAST_FMT_ARG(9, args, mod), BPF_CAST_FMT_ARG(10, args, mod),
> > + BPF_CAST_FMT_ARG(11, args, mod));
> > +
> > + put_fmt_tmp_buf();
>
> reading this for at least 3rd time, this put_fmt_tmp_buf() looks a bit
> out of place and kind of random. I think bpf_printf_cleanup() name
> pairs with bpf_printf_prepare() better.
Yes, I thought it would be clever to name that function
put_fmt_tmp_buf() as a clear parallel to try_get_fmt_tmp_buf() but
because it only puts the buffer if it is used and because they get
called in two different contexts, it's after all maybe not such a
clever name... I'll revert to bpf_printf_cleanup(). Thank you for your
patience with my naming adventures! :)
> > +
> > + return err + 1;
>
> snprintf() already returns string length *including* terminating zero,
> so this is wrong
lib/vsprintf.c says:
* The return value is the number of characters which would be
* generated for the given input, excluding the trailing null,
* as per ISO C99.
Also if I look at the "no arg" test case in the selftest patch.
"simple case" is asserted to return 12 which seems correct to me
(includes the terminating zero only once). Am I missing something ?
However that makes me wonder whether it would be more appropriate to
return the value excluding the trailing null. On one hand it makes
sense to be coherent with other BPF helpers that include the trailing
zero (as discussed in patch v1), on the other hand the helper is
clearly named after the standard "snprintf" function and it's likely
that users will assume it works the same as the std snprintf.
On Wed, Apr 14, 2021 at 1:01 AM Andrii Nakryiko
<[email protected]> wrote:
> On Mon, Apr 12, 2021 at 8:38 AM Florent Revest <[email protected]> wrote:
> > +/* Per-cpu temp buffers which can be used by printf-like helpers for %s or %p
> > + */
> > +#define MAX_PRINTF_BUF_LEN 512
> > +
> > +struct bpf_printf_buf {
> > + char tmp_buf[MAX_PRINTF_BUF_LEN];
> > +};
> > +static DEFINE_PER_CPU(struct bpf_printf_buf, bpf_printf_buf);
> > +static DEFINE_PER_CPU(int, bpf_printf_buf_used);
> > +
> > +static int try_get_fmt_tmp_buf(char **tmp_buf)
> > {
> > - static char buf[BPF_TRACE_PRINTK_SIZE];
> > - unsigned long flags;
> > - va_list ap;
> > - int ret;
> > + struct bpf_printf_buf *bufs = this_cpu_ptr(&bpf_printf_buf);
>
> why doing this_cpu_ptr() if below (if *tmp_buf case), you will not use
> it. just a waste of CPU, no?
Sure I can move it past the conditions.
> > + int used;
> >
> > - raw_spin_lock_irqsave(&trace_printk_lock, flags);
> > - va_start(ap, fmt);
> > - ret = vsnprintf(buf, sizeof(buf), fmt, ap);
> > - va_end(ap);
> > - /* vsnprintf() will not append null for zero-length strings */
> > - if (ret == 0)
> > - buf[0] = '\0';
> > - trace_bpf_trace_printk(buf);
> > - raw_spin_unlock_irqrestore(&trace_printk_lock, flags);
> > + if (*tmp_buf)
> > + return 0;
> >
> > - return ret;
> > + preempt_disable();
> > + used = this_cpu_inc_return(bpf_printf_buf_used);
> > + if (WARN_ON_ONCE(used > 1)) {
> > + this_cpu_dec(bpf_printf_buf_used);
> > + return -EBUSY;
> > + }
>
> get bufs pointer here instead?
Okay :)
> > + *tmp_buf = bufs->tmp_buf;
> > +
> > + return 0;
> > +}
> > +
> > +static void put_fmt_tmp_buf(void)
> > +{
> > + if (this_cpu_read(bpf_printf_buf_used)) {
> > + this_cpu_dec(bpf_printf_buf_used);
> > + preempt_enable();
> > + }
> > }
> >
> > /*
> > - * Only limited trace_printk() conversion specifiers allowed:
> > - * %d %i %u %x %ld %li %lu %lx %lld %lli %llu %llx %p %pB %pks %pus %s
> > + * bpf_parse_fmt_str - Generic pass on format strings for printf-like helpers
> > + *
> > + * Returns a negative value if fmt is an invalid format string or 0 otherwise.
> > + *
> > + * This can be used in two ways:
> > + * - Format string verification only: when final_args and mod are NULL
> > + * - Arguments preparation: in addition to the above verification, it writes in
> > + * final_args a copy of raw_args where pointers from BPF have been sanitized
> > + * into pointers safe to use by snprintf. This also writes in the mod array
> > + * the size requirement of each argument, usable by BPF_CAST_FMT_ARG for ex.
> > + *
> > + * In argument preparation mode, if 0 is returned, safe temporary buffers are
> > + * allocated and put_fmt_tmp_buf should be called to free them after use.
> > */
> > -BPF_CALL_5(bpf_trace_printk, char *, fmt, u32, fmt_size, u64, arg1,
> > - u64, arg2, u64, arg3)
> > -{
> > - int i, mod[3] = {}, fmt_cnt = 0;
> > - char buf[64], fmt_ptype;
> > - void *unsafe_ptr = NULL;
> > - bool str_seen = false;
> > +int bpf_printf_prepare(char *fmt, u32 fmt_size, const u64 *raw_args,
> > + u64 *final_args, enum bpf_printf_mod_type *mod,
> > + u32 num_args)
> > +{
> > + int err, i, curr_specifier = 0, copy_size;
> > + char *unsafe_ptr = NULL, *tmp_buf = NULL;
> > + size_t tmp_buf_len = MAX_PRINTF_BUF_LEN;
> > + enum bpf_printf_mod_type current_mod;
> > + u64 current_arg;
>
> naming consistency: current_arg vs curr_specifier? maybe just cur_arg
> and cur_spec?
Ahah, you're right again :)
> > + char fmt_ptype;
> > +
> > + if ((final_args && !mod) || (mod && !final_args))
>
> nit: same check:
>
> if (!!final_args != !!mod)
Fancy! :)
> > + return -EINVAL;
> >
> > - /*
> > - * bpf_check()->check_func_arg()->check_stack_boundary()
> > - * guarantees that fmt points to bpf program stack,
> > - * fmt_size bytes of it were initialized and fmt_size > 0
> > - */
> > - if (fmt[--fmt_size] != 0)
> > + fmt_size = (strnchr(fmt, fmt_size, 0) - fmt);
>
> extra ()
Oops!
> > + if (!fmt_size)
>
> hm... strnchr() will return NULL if the character is not found, so
> fmt_size will be some non-zero value (due to - fmt), how is this
> supposed to work?
Ugh!
> some negative tests are clearly missing, it seems, if you didn't catch this
Agree
> > return -EINVAL;
> >
> > - /* check format string for allowed specifiers */
> > for (i = 0; i < fmt_size; i++) {
> > - if ((!isprint(fmt[i]) && !isspace(fmt[i])) || !isascii(fmt[i]))
> > - return -EINVAL;
> > + if ((!isprint(fmt[i]) && !isspace(fmt[i])) || !isascii(fmt[i])) {
> > + err = -EINVAL;
> > + goto out;
> > + }
> >
> > if (fmt[i] != '%')
> > continue;
> >
> > - if (fmt_cnt >= 3)
> > - return -EINVAL;
> > + if (fmt[i + 1] == '%') {
> > + i++;
> > + continue;
> > + }
> > +
> > + if (curr_specifier >= num_args) {
> > + err = -EINVAL;
> > + goto out;
> > + }
> >
> > /* fmt[i] != 0 && fmt[last] == 0, so we can access fmt[i + 1] */
>
> a bit outdated comment, last doesn't exist anymore. I think the
> comment is trying to say that fmt[i + 1] can be read because in the
> worst case it will be a final zero terminator (which we checked
> above).
Yes that's the idea. I will rewrite it as a sentence if "last" is confusing.
> > + err = 0;
> > +out:
> > + put_fmt_tmp_buf();
>
> so you are putting tmp_buf unconditionally, even when there was no
> error. That seems wrong? Should this be:
>
> if (err)
> put_fmt_tmp_buf()
>
> ?
Yeah the naming is unfortunate, as discussed in the other patch, I
will rename that to bpf_pintf_cleanup instead. It's not clear from the
name that it only "puts" if the buffer was already gotten.
On Mon, Apr 12, 2021 at 10:32 PM kernel test robot <[email protected]> wrote:
> m68k-linux-ld: kernel/bpf/verifier.o: in function `check_helper_call.isra.0':
> >> verifier.c:(.text+0xf79e): undefined reference to `bpf_printf_prepare'
> m68k-linux-ld: kernel/bpf/helpers.o: in function `bpf_base_func_proto':
> >> helpers.c:(.text+0xd82): undefined reference to `bpf_snprintf_proto'
I'll move the implementation of bpf_printf_prepare/bpf_printf_cleanup
and bpf_snprintf to kernel/bpf/helpers.c so that they all get built on
kernels with CONFIG_BPF_SYSCALL but not CONFIG_BPF_EVENTS.
On Wed, Apr 14, 2021 at 11:56 AM Florent Revest <[email protected]> wrote:
> On Wed, Apr 14, 2021 at 1:01 AM Andrii Nakryiko
> <[email protected]> wrote:
> > On Mon, Apr 12, 2021 at 8:38 AM Florent Revest <[email protected]> wrote:
> > > + err = 0;
> > > +out:
> > > + put_fmt_tmp_buf();
> >
> > so you are putting tmp_buf unconditionally, even when there was no
> > error. That seems wrong? Should this be:
> >
> > if (err)
> > put_fmt_tmp_buf()
> >
> > ?
>
> Yeah the naming is unfortunate, as discussed in the other patch, I
> will rename that to bpf_pintf_cleanup instead. It's not clear from the
> name that it only "puts" if the buffer was already gotten.
Ah, sorry I see what you meant! Indeed, my mistake. :|
Hi Andrii,
On Wed, Apr 14, 2021 at 9:41 AM Andrii Nakryiko
<[email protected]> wrote:
> On Mon, Apr 12, 2021 at 8:38 AM Florent Revest <[email protected]> wrote:
> > The implementation takes inspiration from the existing bpf_trace_printk
> > helper but there are a few differences:
> >
> > To allow for a large number of format-specifiers, parameters are
> > provided in an array, like in bpf_seq_printf.
> >
> > Because the output string takes two arguments and the array of
> > parameters also takes two arguments, the format string needs to fit in
> > one argument. Thankfully, ARG_PTR_TO_CONST_STR is guaranteed to point to
> > a zero-terminated read-only map so we don't need a format string length
> > arg.
> >
> > Because the format-string is known at verification time, we also do
> > a first pass of format string validation in the verifier logic. This
> > makes debugging easier.
> >
> > Signed-off-by: Florent Revest <[email protected]>
> > ---
> > include/linux/bpf.h | 6 ++++
> > include/uapi/linux/bpf.h | 28 +++++++++++++++++++
> > kernel/bpf/helpers.c | 2 ++
> > kernel/bpf/verifier.c | 41 ++++++++++++++++++++++++++++
> > kernel/trace/bpf_trace.c | 50 ++++++++++++++++++++++++++++++++++
> > tools/include/uapi/linux/bpf.h | 28 +++++++++++++++++++
> > 6 files changed, 155 insertions(+)
> >
>
> [...]
>
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 5f46dd6f3383..d4020e5f91ee 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -5918,6 +5918,41 @@ static int check_reference_leak(struct bpf_verifier_env *env)
> > return state->acquired_refs ? -EINVAL : 0;
> > }
> >
> > +static int check_bpf_snprintf_call(struct bpf_verifier_env *env,
> > + struct bpf_reg_state *regs)
> > +{
> > + struct bpf_reg_state *fmt_reg = ®s[BPF_REG_3];
> > + struct bpf_reg_state *data_len_reg = ®s[BPF_REG_5];
> > + struct bpf_map *fmt_map = fmt_reg->map_ptr;
> > + int err, fmt_map_off, num_args;
> > + u64 fmt_addr;
> > + char *fmt;
> > +
> > + /* data must be an array of u64 */
> > + if (data_len_reg->var_off.value % 8)
> > + return -EINVAL;
> > + num_args = data_len_reg->var_off.value / 8;
> > +
> > + /* fmt being ARG_PTR_TO_CONST_STR guarantees that var_off is const
> > + * and map_direct_value_addr is set.
> > + */
> > + fmt_map_off = fmt_reg->off + fmt_reg->var_off.value;
> > + err = fmt_map->ops->map_direct_value_addr(fmt_map, &fmt_addr,
> > + fmt_map_off);
> > + if (err)
> > + return err;
> > + fmt = (char *)fmt_addr + fmt_map_off;
> > +
>
> bot complained about lack of (long) cast before fmt_addr, please address
(uintptr_t), I assume?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hey Geert! :)
On Wed, Apr 14, 2021 at 8:02 PM Geert Uytterhoeven <[email protected]> wrote:
> On Wed, Apr 14, 2021 at 9:41 AM Andrii Nakryiko
> <[email protected]> wrote:
> > On Mon, Apr 12, 2021 at 8:38 AM Florent Revest <[email protected]> wrote:
> > > + fmt = (char *)fmt_addr + fmt_map_off;
> > > +
> >
> > bot complained about lack of (long) cast before fmt_addr, please address
>
> (uintptr_t), I assume?
(uintptr_t) seems more correct to me as well. However, I just had a
look at the rest of verifier.c and (long) casts are already used
pretty much everywhere whereas uintptr_t isn't used yet.
I'll send a v4 with a long cast for the sake of consistency with the
rest of the verifier.
On Wed, Apr 14, 2021 at 2:21 AM Florent Revest <[email protected]> wrote:
>
> On Wed, Apr 14, 2021 at 1:21 AM Andrii Nakryiko
> <[email protected]> wrote:
> >
> > On Mon, Apr 12, 2021 at 8:38 AM Florent Revest <[email protected]> wrote:
> > >
> > > This exercises most of the format specifiers.
> > >
> > > Signed-off-by: Florent Revest <[email protected]>
> > > Acked-by: Andrii Nakryiko <[email protected]>
> > > ---
> >
> > As I mentioned on another patch, we probably need negative tests even
> > more than positive ones.
>
> Agreed.
>
> > I think an easy and nice way to do this is to have a separate BPF
> > skeleton where fmt string and arguments are provided through read-only
> > global variables, so that user-space can re-use the same BPF skeleton
> > to simulate multiple cases. BPF program itself would just call
> > bpf_snprintf() and store the returned result.
>
> Ah, great idea! I was thinking of having one skeleton for each but it
> would be a bit much indeed.
>
> Because the format string needs to be in a read only map though, I
> hope it can be modified from userspace before loading. I'll try it out
> and see :) if it doesn't work I'll just use more skeletons
You need read-only variables (const volatile my_type). Their contents
are statically verified by BPF verifier, yet user-space can pre-setup
it at runtime.
>
> > Whether we need to validate the verifier log is up to debate (though
> > it's not that hard to do by overriding libbpf_print_fn() callback),
> > I'd be ok at least knowing that some bad format strings are rejected
> > and don't crash the kernel.
>
> Alright :)
>
> >
> > > .../selftests/bpf/prog_tests/snprintf.c | 81 +++++++++++++++++++
> > > .../selftests/bpf/progs/test_snprintf.c | 74 +++++++++++++++++
> > > 2 files changed, 155 insertions(+)
> > > create mode 100644 tools/testing/selftests/bpf/prog_tests/snprintf.c
> > > create mode 100644 tools/testing/selftests/bpf/progs/test_snprintf.c
> > >
> >
> > [...]
On Wed, Apr 14, 2021 at 2:46 AM Florent Revest <[email protected]> wrote:
>
> On Wed, Apr 14, 2021 at 1:16 AM Andrii Nakryiko
> <[email protected]> wrote:
> > On Mon, Apr 12, 2021 at 8:38 AM Florent Revest <[email protected]> wrote:
> > > +static int check_bpf_snprintf_call(struct bpf_verifier_env *env,
> > > + struct bpf_reg_state *regs)
> > > +{
> > > + struct bpf_reg_state *fmt_reg = ®s[BPF_REG_3];
> > > + struct bpf_reg_state *data_len_reg = ®s[BPF_REG_5];
> > > + struct bpf_map *fmt_map = fmt_reg->map_ptr;
> > > + int err, fmt_map_off, num_args;
> > > + u64 fmt_addr;
> > > + char *fmt;
> > > +
> > > + /* data must be an array of u64 */
> > > + if (data_len_reg->var_off.value % 8)
> > > + return -EINVAL;
> > > + num_args = data_len_reg->var_off.value / 8;
> > > +
> > > + /* fmt being ARG_PTR_TO_CONST_STR guarantees that var_off is const
> > > + * and map_direct_value_addr is set.
> > > + */
> > > + fmt_map_off = fmt_reg->off + fmt_reg->var_off.value;
> > > + err = fmt_map->ops->map_direct_value_addr(fmt_map, &fmt_addr,
> > > + fmt_map_off);
> > > + if (err)
> > > + return err;
> > > + fmt = (char *)fmt_addr + fmt_map_off;
> > > +
> >
> > bot complained about lack of (long) cast before fmt_addr, please address
>
> Will do.
>
> > > + /* Maximumly we can have MAX_SNPRINTF_VARARGS parameters, just give
> > > + * all of them to snprintf().
> > > + */
> > > + err = snprintf(str, str_size, fmt, BPF_CAST_FMT_ARG(0, args, mod),
> > > + BPF_CAST_FMT_ARG(1, args, mod), BPF_CAST_FMT_ARG(2, args, mod),
> > > + BPF_CAST_FMT_ARG(3, args, mod), BPF_CAST_FMT_ARG(4, args, mod),
> > > + BPF_CAST_FMT_ARG(5, args, mod), BPF_CAST_FMT_ARG(6, args, mod),
> > > + BPF_CAST_FMT_ARG(7, args, mod), BPF_CAST_FMT_ARG(8, args, mod),
> > > + BPF_CAST_FMT_ARG(9, args, mod), BPF_CAST_FMT_ARG(10, args, mod),
> > > + BPF_CAST_FMT_ARG(11, args, mod));
> > > +
> > > + put_fmt_tmp_buf();
> >
> > reading this for at least 3rd time, this put_fmt_tmp_buf() looks a bit
> > out of place and kind of random. I think bpf_printf_cleanup() name
> > pairs with bpf_printf_prepare() better.
>
> Yes, I thought it would be clever to name that function
> put_fmt_tmp_buf() as a clear parallel to try_get_fmt_tmp_buf() but
> because it only puts the buffer if it is used and because they get
> called in two different contexts, it's after all maybe not such a
> clever name... I'll revert to bpf_printf_cleanup(). Thank you for your
> patience with my naming adventures! :)
>
> > > +
> > > + return err + 1;
> >
> > snprintf() already returns string length *including* terminating zero,
> > so this is wrong
>
> lib/vsprintf.c says:
> * The return value is the number of characters which would be
> * generated for the given input, excluding the trailing null,
> * as per ISO C99.
>
> Also if I look at the "no arg" test case in the selftest patch.
> "simple case" is asserted to return 12 which seems correct to me
> (includes the terminating zero only once). Am I missing something ?
>
no, you are right, but that means that bpf_trace_printk is broken, it
doesn't do + 1 (which threw me off here), shall we fix that?
> However that makes me wonder whether it would be more appropriate to
> return the value excluding the trailing null. On one hand it makes
> sense to be coherent with other BPF helpers that include the trailing
> zero (as discussed in patch v1), on the other hand the helper is
> clearly named after the standard "snprintf" function and it's likely
> that users will assume it works the same as the std snprintf.
Having zero included simplifies BPF code tremendously for cases like
bpf_probe_read_str(). So no, let's stick with including zero
terminator in return size.
On Wed, Apr 14, 2021 at 11:30 AM Florent Revest <[email protected]> wrote:
>
> Hey Geert! :)
>
> On Wed, Apr 14, 2021 at 8:02 PM Geert Uytterhoeven <[email protected]> wrote:
> > On Wed, Apr 14, 2021 at 9:41 AM Andrii Nakryiko
> > <[email protected]> wrote:
> > > On Mon, Apr 12, 2021 at 8:38 AM Florent Revest <[email protected]> wrote:
> > > > + fmt = (char *)fmt_addr + fmt_map_off;
> > > > +
> > >
> > > bot complained about lack of (long) cast before fmt_addr, please address
> >
> > (uintptr_t), I assume?
>
> (uintptr_t) seems more correct to me as well. However, I just had a
> look at the rest of verifier.c and (long) casts are already used
> pretty much everywhere whereas uintptr_t isn't used yet.
> I'll send a v4 with a long cast for the sake of consistency with the
> rest of the verifier.
right, I don't care about long or uintptr_t, both are guaranteed to
work, I just remember seeing a lot of code with (long) cast. I have no
preference.
Hi Andrii,
On Thu, Apr 15, 2021 at 12:58 AM Andrii Nakryiko
<[email protected]> wrote:
> On Wed, Apr 14, 2021 at 11:30 AM Florent Revest <[email protected]> wrote:
> > On Wed, Apr 14, 2021 at 8:02 PM Geert Uytterhoeven <[email protected]> wrote:
> > > On Wed, Apr 14, 2021 at 9:41 AM Andrii Nakryiko
> > > <[email protected]> wrote:
> > > > On Mon, Apr 12, 2021 at 8:38 AM Florent Revest <[email protected]> wrote:
> > > > > + fmt = (char *)fmt_addr + fmt_map_off;
> > > > > +
> > > >
> > > > bot complained about lack of (long) cast before fmt_addr, please address
> > >
> > > (uintptr_t), I assume?
> >
> > (uintptr_t) seems more correct to me as well. However, I just had a
> > look at the rest of verifier.c and (long) casts are already used
> > pretty much everywhere whereas uintptr_t isn't used yet.
> > I'll send a v4 with a long cast for the sake of consistency with the
> > rest of the verifier.
>
> right, I don't care about long or uintptr_t, both are guaranteed to
> work, I just remember seeing a lot of code with (long) cast. I have no
> preference.
AFAIR, uintptr_t was introduced only in C99. Early Linux code predates that,
hence uses long, and this behavior was of course copied to new code.
Please use uintptr_t in new code.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Thu, Apr 15, 2021 at 12:57 AM Andrii Nakryiko
<[email protected]> wrote:
>
> On Wed, Apr 14, 2021 at 2:46 AM Florent Revest <[email protected]> wrote:
> >
> > On Wed, Apr 14, 2021 at 1:16 AM Andrii Nakryiko
> > <[email protected]> wrote:
> > > On Mon, Apr 12, 2021 at 8:38 AM Florent Revest <[email protected]> wrote:
> > > > +
> > > > + return err + 1;
> > >
> > > snprintf() already returns string length *including* terminating zero,
> > > so this is wrong
> >
> > lib/vsprintf.c says:
> > * The return value is the number of characters which would be
> > * generated for the given input, excluding the trailing null,
> > * as per ISO C99.
> >
> > Also if I look at the "no arg" test case in the selftest patch.
> > "simple case" is asserted to return 12 which seems correct to me
> > (includes the terminating zero only once). Am I missing something ?
> >
>
> no, you are right, but that means that bpf_trace_printk is broken, it
> doesn't do + 1 (which threw me off here), shall we fix that?
Answered in the 1/6 thread
> > However that makes me wonder whether it would be more appropriate to
> > return the value excluding the trailing null. On one hand it makes
> > sense to be coherent with other BPF helpers that include the trailing
> > zero (as discussed in patch v1), on the other hand the helper is
> > clearly named after the standard "snprintf" function and it's likely
> > that users will assume it works the same as the std snprintf.
>
>
> Having zero included simplifies BPF code tremendously for cases like
> bpf_probe_read_str(). So no, let's stick with including zero
> terminator in return size.
Cool :)
On Thu, Apr 15, 2021 at 12:16 AM Andrii Nakryiko
<[email protected]> wrote:
>
> On Wed, Apr 14, 2021 at 2:21 AM Florent Revest <[email protected]> wrote:
> >
> > On Wed, Apr 14, 2021 at 1:21 AM Andrii Nakryiko
> > <[email protected]> wrote:
> > >
> > > On Mon, Apr 12, 2021 at 8:38 AM Florent Revest <[email protected]> wrote:
> > > >
> > > > This exercises most of the format specifiers.
> > > >
> > > > Signed-off-by: Florent Revest <[email protected]>
> > > > Acked-by: Andrii Nakryiko <[email protected]>
> > > > ---
> > >
> > > As I mentioned on another patch, we probably need negative tests even
> > > more than positive ones.
> >
> > Agreed.
> >
> > > I think an easy and nice way to do this is to have a separate BPF
> > > skeleton where fmt string and arguments are provided through read-only
> > > global variables, so that user-space can re-use the same BPF skeleton
> > > to simulate multiple cases. BPF program itself would just call
> > > bpf_snprintf() and store the returned result.
> >
> > Ah, great idea! I was thinking of having one skeleton for each but it
> > would be a bit much indeed.
> >
> > Because the format string needs to be in a read only map though, I
> > hope it can be modified from userspace before loading. I'll try it out
> > and see :) if it doesn't work I'll just use more skeletons
>
> You need read-only variables (const volatile my_type). Their contents
> are statically verified by BPF verifier, yet user-space can pre-setup
> it at runtime.
Thanks :) v4 has negative fmt tests