[PATCH bpf-next 0/5] Add a snprintf eBPF helper

[PATCH bpf-next 0/5] Add a snprintf eBPF helper

[Florent Revest]

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/20201126165748.1748417-1-revest@google.com/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/20210310015455.1095207-1-revest@chromium.org/T/#u

Florent Revest (5):
  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                           |   5 +
 include/uapi/linux/bpf.h                      |  28 +++
 kernel/bpf/verifier.c                         | 178 ++++++++++++++++++
 kernel/trace/bpf_trace.c                      | 110 +++++++++++
 tools/include/uapi/linux/bpf.h                |  28 +++
 tools/lib/bpf/bpf_tracing.h                   |  45 ++++-
 .../selftests/bpf/prog_tests/snprintf.c       |  71 +++++++
 .../selftests/bpf/progs/test_snprintf.c       |  71 +++++++
 8 files changed, 535 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/snprintf.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_snprintf.c

-- 
2.30.1.766.gb4fecdf3b7-goog

[PATCH bpf-next 1/5] bpf: Add a ARG_PTR_TO_CONST_STR argument type

[Florent Revest]

This type provides the guarantee that an argument is going to be a const
pointer to somewhere in a read-only map value. It also checks that this
pointer is followed by a NULL character before the end of the map value.

Signed-off-by: Florent Revest <revest@chromium.org>
---
 include/linux/bpf.h   |  1 +
 kernel/bpf/verifier.c | 41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index a25730eaa148..7b5319d75b3e 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -308,6 +308,7 @@ enum bpf_arg_type {
 	ARG_PTR_TO_PERCPU_BTF_ID,	/* pointer to in-kernel percpu type */
 	ARG_PTR_TO_FUNC,	/* pointer to a bpf program function */
 	ARG_PTR_TO_STACK_OR_NULL,	/* pointer to stack or NULL */
+	ARG_PTR_TO_CONST_STR,	/* pointer to a null terminated read-only string */
 	__BPF_ARG_TYPE_MAX,
 };
 
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index f9096b049cd6..c99b2b67dc8d 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -4601,6 +4601,7 @@ static const struct bpf_reg_types spin_lock_types = { .types = { PTR_TO_MAP_VALU
 static const struct bpf_reg_types percpu_btf_ptr_types = { .types = { PTR_TO_PERCPU_BTF_ID } };
 static const struct bpf_reg_types func_ptr_types = { .types = { PTR_TO_FUNC } };
 static const struct bpf_reg_types stack_ptr_types = { .types = { PTR_TO_STACK } };
+static const struct bpf_reg_types const_str_ptr_types = { .types = { PTR_TO_MAP_VALUE } };
 
 static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = {
 	[ARG_PTR_TO_MAP_KEY]		= &map_key_value_types,
@@ -4631,6 +4632,7 @@ static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = {
 	[ARG_PTR_TO_PERCPU_BTF_ID]	= &percpu_btf_ptr_types,
 	[ARG_PTR_TO_FUNC]		= &func_ptr_types,
 	[ARG_PTR_TO_STACK_OR_NULL]	= &stack_ptr_types,
+	[ARG_PTR_TO_CONST_STR]		= &const_str_ptr_types,
 };
 
 static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
@@ -4881,6 +4883,45 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
 		if (err)
 			return err;
 		err = check_ptr_alignment(env, reg, 0, size, true);
+	} else if (arg_type == ARG_PTR_TO_CONST_STR) {
+		struct bpf_map *map = reg->map_ptr;
+		int map_off, i;
+		u64 map_addr;
+		char *map_ptr;
+
+		if (!map || !bpf_map_is_rdonly(map)) {
+			verbose(env, "R%d does not point to a readonly map'\n", regno);
+			return -EACCES;
+		}
+
+		if (!tnum_is_const(reg->var_off)) {
+			verbose(env, "R%d is not a constant address'\n", regno);
+			return -EACCES;
+		}
+
+		if (!map->ops->map_direct_value_addr) {
+			verbose(env, "no direct value access support for this map type\n");
+			return -EACCES;
+		}
+
+		err = check_helper_mem_access(env, regno,
+					      map->value_size - reg->off,
+					      false, meta);
+		if (err)
+			return err;
+
+		map_off = reg->off + reg->var_off.value;
+		err = map->ops->map_direct_value_addr(map, &map_addr, map_off);
+		if (err)
+			return err;
+
+		map_ptr = (char *)(map_addr);
+		for (i = map_off; map_ptr[i] != '\0'; i++) {
+			if (i == map->value_size - 1) {
+				verbose(env, "map does not contain a NULL-terminated string\n");
+				return -EACCES;
+			}
+		}
 	}
 
 	return err;
-- 
2.30.1.766.gb4fecdf3b7-goog

[PATCH bpf-next 2/5] bpf: Add a bpf_snprintf helper

[Florent Revest]

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. But because ARG_PTR_TO_CONST_STR guarantees to point to a
NULL-terminated read-only map, we don't need a format string length arg.

Because the format-string is known at verification time, we also move
most of the format string validation, currently done in formatting
helper calls, into the verifier logic. This makes debugging easier and
also slightly improves the runtime performance.

Signed-off-by: Florent Revest <revest@chromium.org>
---
 include/linux/bpf.h            |   4 +
 include/uapi/linux/bpf.h       |  28 +++++++
 kernel/bpf/verifier.c          | 137 +++++++++++++++++++++++++++++++++
 kernel/trace/bpf_trace.c       | 110 ++++++++++++++++++++++++++
 tools/include/uapi/linux/bpf.h |  28 +++++++
 5 files changed, 307 insertions(+)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 7b5319d75b3e..d78175c9a887 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1902,6 +1902,10 @@ extern const struct bpf_func_proto bpf_task_storage_get_proto;
 extern const struct bpf_func_proto bpf_task_storage_delete_proto;
 extern const struct bpf_func_proto bpf_for_each_map_elem_proto;
 
+#define MAX_SNPRINTF_VARARGS		12
+#define MAX_SNPRINTF_MEMCPY		6
+#define MAX_SNPRINTF_STR_LEN		128
+
 const struct bpf_func_proto *bpf_tracing_func_proto(
 	enum bpf_func_id func_id, const struct bpf_prog *prog);
 
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 2d3036e292a9..3cbdc8ae00e7 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -4660,6 +4660,33 @@ union bpf_attr {
  *	Return
  *		The number of traversed map elements for success, **-EINVAL** for
  *		invalid **flags**.
+ *
+ * long bpf_snprintf(char *out, u32 out_size, const char *fmt, u64 *data, u32 data_len)
+ *	Description
+ *		Outputs a string into the **out** buffer of size **out_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 printed string, including
+ *		the trailing NUL character. If the return value is greater than
+ *		**out_size**, **out** contains a truncated string, without a
+ *		trailing NULL character.
+ *
+ *		Or **-EBUSY** if the per-CPU memory copy buffer is busy.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -4827,6 +4854,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/verifier.c b/kernel/bpf/verifier.c
index c99b2b67dc8d..3ab549df817b 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -5732,6 +5732,137 @@ static int check_reference_leak(struct bpf_verifier_env *env)
 	return state->acquired_refs ? -EINVAL : 0;
 }
 
+int check_bpf_snprintf_call(struct bpf_verifier_env *env,
+			    struct bpf_reg_state *regs)
+{
+	struct bpf_reg_state *fmt_reg = &regs[BPF_REG_3];
+	struct bpf_reg_state *data_len_reg = &regs[BPF_REG_5];
+	struct bpf_map *fmt_map = fmt_reg->map_ptr;
+	int err, fmt_map_off, i, fmt_cnt = 0, memcpy_cnt = 0, num_args;
+	u64 fmt_addr;
+	char *fmt;
+
+	/* data must be an array of u64 so data_len must be a multiple of 8 */
+	if (data_len_reg->var_off.value & 7)
+		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;
+
+	/* We are also guaranteed that fmt+fmt_map_off is NULL terminated, we
+	 * can focus on validating the format specifiers.
+	 */
+	for (i = fmt_map_off; fmt[i] != '\0'; i++) {
+		if ((!isprint(fmt[i]) && !isspace(fmt[i])) ||
+		    !isascii(fmt[i])) {
+			verbose(env, "only printable ascii for now\n");
+			return -EINVAL;
+		}
+
+		if (fmt[i] != '%')
+			continue;
+
+		if (fmt[i + 1] == '%') {
+			i++;
+			continue;
+		}
+
+		if (fmt_cnt >= MAX_SNPRINTF_VARARGS) {
+			verbose(env, "too many format specifiers\n");
+			return -E2BIG;
+		}
+
+		if (fmt_cnt >= num_args) {
+			verbose(env, "not enough parameters to print\n");
+			return -EINVAL;
+		}
+
+		/* 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') {
+			if (memcpy_cnt >= MAX_SNPRINTF_MEMCPY) {
+				verbose(env, "too many buffer copies\n");
+				return -E2BIG;
+			}
+
+			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' ||
+			    fmt[i + 1] == 's' || fmt[i + 1] == 'S') {
+				fmt_cnt++;
+				continue;
+			}
+
+			/* only support "%pI4", "%pi4", "%pI6" and "%pi6". */
+			if (fmt[i + 1] != 'i' && fmt[i + 1] != 'I') {
+				verbose(env, "invalid specifier %%p%c\n",
+					fmt[i+1]);
+				return -EINVAL;
+			}
+			if (fmt[i + 2] != '4' && fmt[i + 2] != '6') {
+				verbose(env, "invalid specifier %%p%c%c\n",
+					fmt[i+1], fmt[i+2]);
+				return -EINVAL;
+			}
+
+			if (memcpy_cnt >= MAX_SNPRINTF_MEMCPY) {
+				verbose(env, "too many buffer copies\n");
+				return -E2BIG;
+			}
+
+			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') {
+			verbose(env, "invalid format specifier %%%c\n", fmt[i]);
+			return -EINVAL;
+		}
+
+		fmt_cnt++;
+	}
+
+	if (fmt_cnt != num_args) {
+		verbose(env, "too many parameters to print\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
 			     int *insn_idx_p)
 {
@@ -5846,6 +5977,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 0d23755c2747..7b80759c10a9 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1271,6 +1271,114 @@ const struct bpf_func_proto bpf_snprintf_btf_proto = {
 	.arg5_type	= ARG_ANYTHING,
 };
 
+struct bpf_snprintf_buf {
+	char buf[MAX_SNPRINTF_MEMCPY][MAX_SNPRINTF_STR_LEN];
+};
+static DEFINE_PER_CPU(struct bpf_snprintf_buf, bpf_snprintf_buf);
+static DEFINE_PER_CPU(int, bpf_snprintf_buf_used);
+
+BPF_CALL_5(bpf_snprintf, char *, out, u32, out_size, char *, fmt, u64 *, args,
+	   u32, args_len)
+{
+	int err, i, buf_used, copy_size, fmt_cnt = 0, memcpy_cnt = 0;
+	u64 params[MAX_SNPRINTF_VARARGS];
+	struct bpf_snprintf_buf *bufs;
+
+	buf_used = this_cpu_inc_return(bpf_snprintf_buf_used);
+	if (WARN_ON_ONCE(buf_used > 1)) {
+		err = -EBUSY;
+		goto out;
+	}
+
+	bufs = this_cpu_ptr(&bpf_snprintf_buf);
+
+	/*
+	 * The verifier has already done most of the heavy-work for us in
+	 * check_bpf_snprintf_call. We know that fmt is well formatted and that
+	 * args_len is valid. The only task left is to convert some of the
+	 * arguments. For the %s and %pi* specifiers, we need to read buffers
+	 * from a kernel address during the helper call.
+	 */
+	for (i = 0; fmt[i] != '\0'; i++) {
+		if (fmt[i] != '%')
+			continue;
+
+		if (fmt[i + 1] == '%') {
+			i++;
+			continue;
+		}
+
+		/* 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 = (void *)(long)args[fmt_cnt];
+
+			err = strncpy_from_kernel_nofault(bufs->buf[memcpy_cnt],
+							  unsafe_ptr,
+							  MAX_SNPRINTF_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' && (fmt[i + 1] == 'i' || fmt[i + 1] == 'I')) {
+			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;
+		}
+
+		params[fmt_cnt] = args[fmt_cnt];
+		fmt_cnt++;
+	}
+
+	/* Maximumly we can have MAX_SNPRINTF_VARARGS parameters, just give
+	 * all of them to snprintf().
+	 */
+	err = snprintf(out, out_size, 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]) + 1;
+
+out:
+	this_cpu_dec(bpf_snprintf_buf_used);
+	return err;
+}
+
+static const struct bpf_func_proto bpf_snprintf_proto = {
+	.func		= bpf_snprintf,
+	.gpl_only	= true,
+	.ret_type	= RET_INTEGER,
+	.arg1_type	= ARG_PTR_TO_MEM,
+	.arg2_type	= ARG_CONST_SIZE,
+	.arg3_type	= ARG_PTR_TO_CONST_STR,
+	.arg4_type	= ARG_PTR_TO_MEM,
+	.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)
 {
@@ -1373,6 +1481,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 2d3036e292a9..3cbdc8ae00e7 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -4660,6 +4660,33 @@ union bpf_attr {
  *	Return
  *		The number of traversed map elements for success, **-EINVAL** for
  *		invalid **flags**.
+ *
+ * long bpf_snprintf(char *out, u32 out_size, const char *fmt, u64 *data, u32 data_len)
+ *	Description
+ *		Outputs a string into the **out** buffer of size **out_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 printed string, including
+ *		the trailing NUL character. If the return value is greater than
+ *		**out_size**, **out** contains a truncated string, without a
+ *		trailing NULL character.
+ *
+ *		Or **-EBUSY** if the per-CPU memory copy buffer is busy.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -4827,6 +4854,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.30.1.766.gb4fecdf3b7-goog

[PATCH bpf-next 3/5] libbpf: Initialize the bpf_seq_printf parameters array field by field

[Florent Revest]

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.

This is a workaround, ideally the rodata relocation should be supported
by libbpf but this would require a disproportionate amount of work given
the actual usecases. (it is very unlikely that one uses a const array of
relocated addresses)

Signed-off-by: Florent Revest <revest@chromium.org>
---
 tools/lib/bpf/bpf_tracing.h | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
index f9ef37707888..f6a2deb3cd5b 100644
--- a/tools/lib/bpf/bpf_tracing.h
+++ b/tools/lib/bpf/bpf_tracing.h
@@ -413,6 +413,34 @@ typeof(name(0)) name(struct pt_regs *ctx)				    \
 }									    \
 static __always_inline typeof(name(0)) ____##name(struct pt_regs *ctx, ##args)
 
+#define ___bpf_build_param0(narg, x)
+#define ___bpf_build_param1(narg, x) ___param[narg - 1] = x
+#define ___bpf_build_param2(narg, x, args...) ___param[narg - 2] = x; \
+					      ___bpf_build_param1(narg, args)
+#define ___bpf_build_param3(narg, x, args...) ___param[narg - 3] = x; \
+					      ___bpf_build_param2(narg, args)
+#define ___bpf_build_param4(narg, x, args...) ___param[narg - 4] = x; \
+					      ___bpf_build_param3(narg, args)
+#define ___bpf_build_param5(narg, x, args...) ___param[narg - 5] = x; \
+					      ___bpf_build_param4(narg, args)
+#define ___bpf_build_param6(narg, x, args...) ___param[narg - 6] = x; \
+					      ___bpf_build_param5(narg, args)
+#define ___bpf_build_param7(narg, x, args...) ___param[narg - 7] = x; \
+					      ___bpf_build_param6(narg, args)
+#define ___bpf_build_param8(narg, x, args...) ___param[narg - 8] = x; \
+					      ___bpf_build_param7(narg, args)
+#define ___bpf_build_param9(narg, x, args...) ___param[narg - 9] = x; \
+					      ___bpf_build_param8(narg, args)
+#define ___bpf_build_param10(narg, x, args...) ___param[narg - 10] = x; \
+					       ___bpf_build_param9(narg, args)
+#define ___bpf_build_param11(narg, x, args...) ___param[narg - 11] = x; \
+					       ___bpf_build_param10(narg, args)
+#define ___bpf_build_param12(narg, x, args...) ___param[narg - 12] = x; \
+					       ___bpf_build_param11(narg, args)
+#define ___bpf_build_param(args...) \
+	unsigned long long ___param[___bpf_narg(args)];			\
+	___bpf_apply(___bpf_build_param, ___bpf_narg(args))(___bpf_narg(args), args)
+
 /*
  * BPF_SEQ_PRINTF to wrap bpf_seq_printf to-be-printed values
  * in a structure.
@@ -422,7 +450,7 @@ static __always_inline typeof(name(0)) ____##name(struct pt_regs *ctx, ##args)
 		_Pragma("GCC diagnostic push")				    \
 		_Pragma("GCC diagnostic ignored \"-Wint-conversion\"")	    \
 		static const char ___fmt[] = fmt;			    \
-		unsigned long long ___param[] = { args };		    \
+		___bpf_build_param(args);				    \
 		_Pragma("GCC diagnostic pop")				    \
 		int ___ret = bpf_seq_printf(seq, ___fmt, sizeof(___fmt),    \
 					    ___param, sizeof(___param));    \
-- 
2.30.1.766.gb4fecdf3b7-goog

[PATCH bpf-next 4/5] libbpf: Introduce a BPF_SNPRINTF helper macro

[Florent Revest]

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 <revest@chromium.org>
---
 tools/lib/bpf/bpf_tracing.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
index f6a2deb3cd5b..89e82da9b8a0 100644
--- a/tools/lib/bpf/bpf_tracing.h
+++ b/tools/lib/bpf/bpf_tracing.h
@@ -457,4 +457,19 @@ static __always_inline typeof(name(0)) ____##name(struct pt_regs *ctx, ##args)
 		___ret;							    \
 	})
 
+/*
+ * BPF_SNPRINTF wraps the bpf_snprintf helper with variadic arguments instead of
+ * an array of u64.
+ */
+#define BPF_SNPRINTF(out, out_size, fmt, args...)			    \
+	({								    \
+		_Pragma("GCC diagnostic push")				    \
+		_Pragma("GCC diagnostic ignored \"-Wint-conversion\"")	    \
+		___bpf_build_param(args);				    \
+		_Pragma("GCC diagnostic pop")				    \
+		int ___ret = bpf_snprintf(out, out_size, fmt,		    \
+					    ___param, sizeof(___param));    \
+		___ret;							    \
+	})
+
 #endif
-- 
2.30.1.766.gb4fecdf3b7-goog

[PATCH bpf-next 5/5] selftests/bpf: Add a series of tests for bpf_snprintf

[Florent Revest]

This exercices most of the format specifiers when things go well.

Signed-off-by: Florent Revest <revest@chromium.org>
---
 .../selftests/bpf/prog_tests/snprintf.c       | 71 +++++++++++++++++++
 .../selftests/bpf/progs/test_snprintf.c       | 71 +++++++++++++++++++
 2 files changed, 142 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..23af1dbd1eeb
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/snprintf.c
@@ -0,0 +1,71 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2021 Google LLC. */
+
+#include <test_progs.h>
+#include "test_snprintf.skel.h"
+
+static int duration;
+
+#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 {'%', 'o', 'v', 'e', 'r'}
+#define EXP_OVER_RET 10
+
+void test_snprintf(void)
+{
+	char exp_addr_out[] = EXP_ADDR_OUT;
+	char exp_over_out[] = EXP_OVER_OUT;
+	char exp_sym_out[]  = EXP_SYM_OUT;
+	struct test_snprintf *skel;
+	int err;
+
+	skel = test_snprintf__open_and_load();
+	if (CHECK(!skel, "skel_open", "failed to open and load skeleton\n"))
+		return;
+
+	err = test_snprintf__attach(skel);
+	if (CHECK(err, "skel_attach", "skeleton attach failed: %d\n", err))
+		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_OK(memcmp(skel->bss->over_out, exp_over_out,
+			 sizeof(exp_over_out)), "over_out");
+	ASSERT_EQ(skel->bss->over_ret, EXP_OVER_RET, "over_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..6c8aa4988e69
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_snprintf.c
@@ -0,0 +1,71 @@
+// 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>
+
+#define OUT_LEN 64
+
+/* Integer types */
+static const char num_fmt[] = "%d %u %x %li %llu %lX";
+#define NUMBERS -8, 9, 150, -424242, 1337, 0xDABBAD00
+
+char num_out[OUT_LEN] = {};
+long num_ret = 0;
+
+/* IP addresses */
+static const char ip_fmt[] = "%pi4 %pI6";
+static const __u8 dummy_ipv4[] = {127, 0, 0, 1}; /* 127.0.0.1 */
+static const __u32 dummy_ipv6[] = {0, 0, 0, bpf_htonl(1)}; /* ::1/128 */
+#define IPS &dummy_ipv4, &dummy_ipv6
+
+char ip_out[OUT_LEN] = {};
+long ip_ret = 0;
+
+/* Symbol lookup formatting */
+static const char sym_fmt[] = "%ps %pS %pB";
+extern const void schedule __ksym;
+#define SYMBOLS &schedule, &schedule, &schedule
+
+char sym_out[OUT_LEN] = {};
+long sym_ret = 0;
+
+/* Kernel pointers */
+static const char addr_fmt[] = "%pK %px %p";
+#define ADDRESSES 0, 0xFFFF00000ADD4E55, 0xFFFF00000ADD4E55
+
+char addr_out[OUT_LEN] = {};
+long addr_ret = 0;
+
+/* Strings embedding */
+static const char str_fmt[] = "%s %+05s";
+static const char str1[] = "str1";
+static const char longstr[] = "longstr";
+#define STRINGS str1, longstr
+
+char str_out[OUT_LEN] = {};
+long str_ret = 0;
+
+/* Overflow */
+static const char over_fmt[] = "%%overflow";
+
+#define OVER_OUT_LEN 6
+char over_out[OVER_OUT_LEN] = {};
+long over_ret = 0;
+
+SEC("raw_tp/sys_enter")
+int handler(const void *ctx)
+{
+	num_ret  = BPF_SNPRINTF(num_out,  OUT_LEN, num_fmt,  NUMBERS);
+	ip_ret   = BPF_SNPRINTF(ip_out,   OUT_LEN, ip_fmt,   IPS);
+	sym_ret  = BPF_SNPRINTF(sym_out,  OUT_LEN, sym_fmt,  SYMBOLS);
+	addr_ret = BPF_SNPRINTF(addr_out, OUT_LEN, addr_fmt, ADDRESSES);
+	str_ret  = BPF_SNPRINTF(str_out,  OUT_LEN, str_fmt,  STRINGS);
+	over_ret = BPF_SNPRINTF(over_out, OVER_OUT_LEN, over_fmt);
+
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";
-- 
2.30.1.766.gb4fecdf3b7-goog

Re: [PATCH bpf-next 1/5] bpf: Add a ARG_PTR_TO_CONST_STR argument type

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 15541 bytes --]

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/bpf-Add-a-ARG_PTR_TO_CONST_STR-argument-type/20210311-070306
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: openrisc-randconfig-r023-20210308 (attached as .config)
compiler: or1k-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/cbb95ec99fafe0955aeada270c9be3d1477c3866
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Florent-Revest/bpf-Add-a-ARG_PTR_TO_CONST_STR-argument-type/20210311-070306
        git checkout cbb95ec99fafe0955aeada270c9be3d1477c3866
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=openrisc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   kernel/bpf/verifier.c: In function 'check_func_arg':
>> kernel/bpf/verifier.c:4918:13: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
    4918 |   map_ptr = (char *)(map_addr);
         |             ^
   In file included from include/linux/bpf_verifier.h:9,
                    from kernel/bpf/verifier.c:12:
   kernel/bpf/verifier.c: In function 'jit_subprogs':
   include/linux/filter.h:363:4: warning: cast between incompatible function types from 'unsigned int (*)(const void *, const struct bpf_insn *)' to 'u64 (*)(u64,  u64,  u64,  u64,  u64)' {aka 'long long unsigned int (*)(long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int)'} [-Wcast-function-type]
     363 |   ((u64 (*)(u64, u64, u64, u64, u64))(x))
         |    ^
   kernel/bpf/verifier.c:11728:16: note: in expansion of macro 'BPF_CAST_CALL'
   11728 |    insn->imm = BPF_CAST_CALL(func[subprog]->bpf_func) -
         |                ^~~~~~~~~~~~~
   kernel/bpf/verifier.c: In function 'do_misc_fixups':
   include/linux/filter.h:363:4: warning: cast between incompatible function types from 'void * (* const)(struct bpf_map *, void *)' to 'u64 (*)(u64,  u64,  u64,  u64,  u64)' {aka 'long long unsigned int (*)(long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int)'} [-Wcast-function-type]
     363 |   ((u64 (*)(u64, u64, u64, u64, u64))(x))
         |    ^
   kernel/bpf/verifier.c:12136:17: note: in expansion of macro 'BPF_CAST_CALL'
   12136 |     insn->imm = BPF_CAST_CALL(ops->map_lookup_elem) -
         |                 ^~~~~~~~~~~~~
   include/linux/filter.h:363:4: warning: cast between incompatible function types from 'int (* const)(struct bpf_map *, void *, void *, u64)' {aka 'int (* const)(struct bpf_map *, void *, void *, long long unsigned int)'} to 'u64 (*)(u64,  u64,  u64,  u64,  u64)' {aka 'long long unsigned int (*)(long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int)'} [-Wcast-function-type]
     363 |   ((u64 (*)(u64, u64, u64, u64, u64))(x))
         |    ^
   kernel/bpf/verifier.c:12140:17: note: in expansion of macro 'BPF_CAST_CALL'
   12140 |     insn->imm = BPF_CAST_CALL(ops->map_update_elem) -
         |                 ^~~~~~~~~~~~~
   include/linux/filter.h:363:4: warning: cast between incompatible function types from 'int (* const)(struct bpf_map *, void *)' to 'u64 (*)(u64,  u64,  u64,  u64,  u64)' {aka 'long long unsigned int (*)(long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int)'} [-Wcast-function-type]
     363 |   ((u64 (*)(u64, u64, u64, u64, u64))(x))
         |    ^
   kernel/bpf/verifier.c:12144:17: note: in expansion of macro 'BPF_CAST_CALL'
   12144 |     insn->imm = BPF_CAST_CALL(ops->map_delete_elem) -
         |                 ^~~~~~~~~~~~~
   include/linux/filter.h:363:4: warning: cast between incompatible function types from 'int (* const)(struct bpf_map *, void *, u64)' {aka 'int (* const)(struct bpf_map *, void *, long long unsigned int)'} to 'u64 (*)(u64,  u64,  u64,  u64,  u64)' {aka 'long long unsigned int (*)(long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int)'} [-Wcast-function-type]
     363 |   ((u64 (*)(u64, u64, u64, u64, u64))(x))
         |    ^
   kernel/bpf/verifier.c:12148:17: note: in expansion of macro 'BPF_CAST_CALL'
   12148 |     insn->imm = BPF_CAST_CALL(ops->map_push_elem) -
         |                 ^~~~~~~~~~~~~
   include/linux/filter.h:363:4: warning: cast between incompatible function types from 'int (* const)(struct bpf_map *, void *)' to 'u64 (*)(u64,  u64,  u64,  u64,  u64)' {aka 'long long unsigned int (*)(long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int)'} [-Wcast-function-type]
     363 |   ((u64 (*)(u64, u64, u64, u64, u64))(x))
         |    ^
   kernel/bpf/verifier.c:12152:17: note: in expansion of macro 'BPF_CAST_CALL'
   12152 |     insn->imm = BPF_CAST_CALL(ops->map_pop_elem) -
         |                 ^~~~~~~~~~~~~
   include/linux/filter.h:363:4: warning: cast between incompatible function types from 'int (* const)(struct bpf_map *, void *)' to 'u64 (*)(u64,  u64,  u64,  u64,  u64)' {aka 'long long unsigned int (*)(long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int)'} [-Wcast-function-type]
     363 |   ((u64 (*)(u64, u64, u64, u64, u64))(x))
         |    ^
   kernel/bpf/verifier.c:12156:17: note: in expansion of macro 'BPF_CAST_CALL'
   12156 |     insn->imm = BPF_CAST_CALL(ops->map_peek_elem) -
         |                 ^~~~~~~~~~~~~
   include/linux/filter.h:363:4: warning: cast between incompatible function types from 'int (* const)(struct bpf_map *, u32,  u64)' {aka 'int (* const)(struct bpf_map *, unsigned int,  long long unsigned int)'} to 'u64 (*)(u64,  u64,  u64,  u64,  u64)' {aka 'long long unsigned int (*)(long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int)'} [-Wcast-function-type]
     363 |   ((u64 (*)(u64, u64, u64, u64, u64))(x))
         |    ^
   kernel/bpf/verifier.c:12160:17: note: in expansion of macro 'BPF_CAST_CALL'
   12160 |     insn->imm = BPF_CAST_CALL(ops->map_redirect) -
         |                 ^~~~~~~~~~~~~


vim +4918 kernel/bpf/verifier.c

  4695	
  4696	static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
  4697				  struct bpf_call_arg_meta *meta,
  4698				  const struct bpf_func_proto *fn)
  4699	{
  4700		u32 regno = BPF_REG_1 + arg;
  4701		struct bpf_reg_state *regs = cur_regs(env), *reg = &regs[regno];
  4702		enum bpf_arg_type arg_type = fn->arg_type[arg];
  4703		enum bpf_reg_type type = reg->type;
  4704		int err = 0;
  4705	
  4706		if (arg_type == ARG_DONTCARE)
  4707			return 0;
  4708	
  4709		err = check_reg_arg(env, regno, SRC_OP);
  4710		if (err)
  4711			return err;
  4712	
  4713		if (arg_type == ARG_ANYTHING) {
  4714			if (is_pointer_value(env, regno)) {
  4715				verbose(env, "R%d leaks addr into helper function\n",
  4716					regno);
  4717				return -EACCES;
  4718			}
  4719			return 0;
  4720		}
  4721	
  4722		if (type_is_pkt_pointer(type) &&
  4723		    !may_access_direct_pkt_data(env, meta, BPF_READ)) {
  4724			verbose(env, "helper access to the packet is not allowed\n");
  4725			return -EACCES;
  4726		}
  4727	
  4728		if (arg_type == ARG_PTR_TO_MAP_VALUE ||
  4729		    arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE ||
  4730		    arg_type == ARG_PTR_TO_MAP_VALUE_OR_NULL) {
  4731			err = resolve_map_arg_type(env, meta, &arg_type);
  4732			if (err)
  4733				return err;
  4734		}
  4735	
  4736		if (register_is_null(reg) && arg_type_may_be_null(arg_type))
  4737			/* A NULL register has a SCALAR_VALUE type, so skip
  4738			 * type checking.
  4739			 */
  4740			goto skip_type_check;
  4741	
  4742		err = check_reg_type(env, regno, arg_type, fn->arg_btf_id[arg]);
  4743		if (err)
  4744			return err;
  4745	
  4746		if (type == PTR_TO_CTX) {
  4747			err = check_ctx_reg(env, reg, regno);
  4748			if (err < 0)
  4749				return err;
  4750		}
  4751	
  4752	skip_type_check:
  4753		if (reg->ref_obj_id) {
  4754			if (meta->ref_obj_id) {
  4755				verbose(env, "verifier internal error: more than one arg with ref_obj_id R%d %u %u\n",
  4756					regno, reg->ref_obj_id,
  4757					meta->ref_obj_id);
  4758				return -EFAULT;
  4759			}
  4760			meta->ref_obj_id = reg->ref_obj_id;
  4761		}
  4762	
  4763		if (arg_type == ARG_CONST_MAP_PTR) {
  4764			/* bpf_map_xxx(map_ptr) call: remember that map_ptr */
  4765			meta->map_ptr = reg->map_ptr;
  4766		} else if (arg_type == ARG_PTR_TO_MAP_KEY) {
  4767			/* bpf_map_xxx(..., map_ptr, ..., key) call:
  4768			 * check that [key, key + map->key_size) are within
  4769			 * stack limits and initialized
  4770			 */
  4771			if (!meta->map_ptr) {
  4772				/* in function declaration map_ptr must come before
  4773				 * map_key, so that it's verified and known before
  4774				 * we have to check map_key here. Otherwise it means
  4775				 * that kernel subsystem misconfigured verifier
  4776				 */
  4777				verbose(env, "invalid map_ptr to access map->key\n");
  4778				return -EACCES;
  4779			}
  4780			err = check_helper_mem_access(env, regno,
  4781						      meta->map_ptr->key_size, false,
  4782						      NULL);
  4783		} else if (arg_type == ARG_PTR_TO_MAP_VALUE ||
  4784			   (arg_type == ARG_PTR_TO_MAP_VALUE_OR_NULL &&
  4785			    !register_is_null(reg)) ||
  4786			   arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE) {
  4787			/* bpf_map_xxx(..., map_ptr, ..., value) call:
  4788			 * check [value, value + map->value_size) validity
  4789			 */
  4790			if (!meta->map_ptr) {
  4791				/* kernel subsystem misconfigured verifier */
  4792				verbose(env, "invalid map_ptr to access map->value\n");
  4793				return -EACCES;
  4794			}
  4795			meta->raw_mode = (arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE);
  4796			err = check_helper_mem_access(env, regno,
  4797						      meta->map_ptr->value_size, false,
  4798						      meta);
  4799		} else if (arg_type == ARG_PTR_TO_PERCPU_BTF_ID) {
  4800			if (!reg->btf_id) {
  4801				verbose(env, "Helper has invalid btf_id in R%d\n", regno);
  4802				return -EACCES;
  4803			}
  4804			meta->ret_btf = reg->btf;
  4805			meta->ret_btf_id = reg->btf_id;
  4806		} else if (arg_type == ARG_PTR_TO_SPIN_LOCK) {
  4807			if (meta->func_id == BPF_FUNC_spin_lock) {
  4808				if (process_spin_lock(env, regno, true))
  4809					return -EACCES;
  4810			} else if (meta->func_id == BPF_FUNC_spin_unlock) {
  4811				if (process_spin_lock(env, regno, false))
  4812					return -EACCES;
  4813			} else {
  4814				verbose(env, "verifier internal error\n");
  4815				return -EFAULT;
  4816			}
  4817		} else if (arg_type == ARG_PTR_TO_FUNC) {
  4818			meta->subprogno = reg->subprogno;
  4819		} else if (arg_type_is_mem_ptr(arg_type)) {
  4820			/* The access to this pointer is only checked when we hit the
  4821			 * next is_mem_size argument below.
  4822			 */
  4823			meta->raw_mode = (arg_type == ARG_PTR_TO_UNINIT_MEM);
  4824		} else if (arg_type_is_mem_size(arg_type)) {
  4825			bool zero_size_allowed = (arg_type == ARG_CONST_SIZE_OR_ZERO);
  4826	
  4827			/* This is used to refine r0 return value bounds for helpers
  4828			 * that enforce this value as an upper bound on return values.
  4829			 * See do_refine_retval_range() for helpers that can refine
  4830			 * the return value. C type of helper is u32 so we pull register
  4831			 * bound from umax_value however, if negative verifier errors
  4832			 * out. Only upper bounds can be learned because retval is an
  4833			 * int type and negative retvals are allowed.
  4834			 */
  4835			meta->msize_max_value = reg->umax_value;
  4836	
  4837			/* The register is SCALAR_VALUE; the access check
  4838			 * happens using its boundaries.
  4839			 */
  4840			if (!tnum_is_const(reg->var_off))
  4841				/* For unprivileged variable accesses, disable raw
  4842				 * mode so that the program is required to
  4843				 * initialize all the memory that the helper could
  4844				 * just partially fill up.
  4845				 */
  4846				meta = NULL;
  4847	
  4848			if (reg->smin_value < 0) {
  4849				verbose(env, "R%d min value is negative, either use unsigned or 'var &= const'\n",
  4850					regno);
  4851				return -EACCES;
  4852			}
  4853	
  4854			if (reg->umin_value == 0) {
  4855				err = check_helper_mem_access(env, regno - 1, 0,
  4856							      zero_size_allowed,
  4857							      meta);
  4858				if (err)
  4859					return err;
  4860			}
  4861	
  4862			if (reg->umax_value >= BPF_MAX_VAR_SIZ) {
  4863				verbose(env, "R%d unbounded memory access, use 'var &= const' or 'if (var < const)'\n",
  4864					regno);
  4865				return -EACCES;
  4866			}
  4867			err = check_helper_mem_access(env, regno - 1,
  4868						      reg->umax_value,
  4869						      zero_size_allowed, meta);
  4870			if (!err)
  4871				err = mark_chain_precision(env, regno);
  4872		} else if (arg_type_is_alloc_size(arg_type)) {
  4873			if (!tnum_is_const(reg->var_off)) {
  4874				verbose(env, "R%d is not a known constant'\n",
  4875					regno);
  4876				return -EACCES;
  4877			}
  4878			meta->mem_size = reg->var_off.value;
  4879		} else if (arg_type_is_int_ptr(arg_type)) {
  4880			int size = int_ptr_type_to_size(arg_type);
  4881	
  4882			err = check_helper_mem_access(env, regno, size, false, meta);
  4883			if (err)
  4884				return err;
  4885			err = check_ptr_alignment(env, reg, 0, size, true);
  4886		} else if (arg_type == ARG_PTR_TO_CONST_STR) {
  4887			struct bpf_map *map = reg->map_ptr;
  4888			int map_off, i;
  4889			u64 map_addr;
  4890			char *map_ptr;
  4891	
  4892			if (!map || !bpf_map_is_rdonly(map)) {
  4893				verbose(env, "R%d does not point to a readonly map'\n", regno);
  4894				return -EACCES;
  4895			}
  4896	
  4897			if (!tnum_is_const(reg->var_off)) {
  4898				verbose(env, "R%d is not a constant address'\n", regno);
  4899				return -EACCES;
  4900			}
  4901	
  4902			if (!map->ops->map_direct_value_addr) {
  4903				verbose(env, "no direct value access support for this map type\n");
  4904				return -EACCES;
  4905			}
  4906	
  4907			err = check_helper_mem_access(env, regno,
  4908						      map->value_size - reg->off,
  4909						      false, meta);
  4910			if (err)
  4911				return err;
  4912	
  4913			map_off = reg->off + reg->var_off.value;
  4914			err = map->ops->map_direct_value_addr(map, &map_addr, map_off);
  4915			if (err)
  4916				return err;
  4917	
> 4918			map_ptr = (char *)(map_addr);
  4919			for (i = map_off; map_ptr[i] != '\0'; i++) {
  4920				if (i == map->value_size - 1) {
  4921					verbose(env, "map does not contain a NULL-terminated string\n");
  4922					return -EACCES;
  4923				}
  4924			}
  4925		}
  4926	
  4927		return err;
  4928	}
  4929	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29308 bytes --]

Re: [PATCH bpf-next 2/5] bpf: Add a bpf_snprintf helper

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 11462 bytes --]

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/bpf-Add-a-ARG_PTR_TO_CONST_STR-argument-type/20210311-070306
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: openrisc-randconfig-r023-20210308 (attached as .config)
compiler: or1k-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/c06d419a45370b897b7383625f0435873a7458fa
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Florent-Revest/bpf-Add-a-ARG_PTR_TO_CONST_STR-argument-type/20210311-070306
        git checkout c06d419a45370b897b7383625f0435873a7458fa
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=openrisc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   kernel/bpf/verifier.c: In function 'check_func_arg':
   kernel/bpf/verifier.c:4918:13: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
    4918 |   map_ptr = (char *)(map_addr);
         |             ^
   kernel/bpf/verifier.c: At top level:
>> kernel/bpf/verifier.c:5735:5: warning: no previous prototype for 'check_bpf_snprintf_call' [-Wmissing-prototypes]
    5735 | int check_bpf_snprintf_call(struct bpf_verifier_env *env,
         |     ^~~~~~~~~~~~~~~~~~~~~~~
   kernel/bpf/verifier.c: In function 'check_bpf_snprintf_call':
   kernel/bpf/verifier.c:5758:8: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
    5758 |  fmt = (char *)fmt_addr;
         |        ^
   In file included from include/linux/bpf_verifier.h:9,
                    from kernel/bpf/verifier.c:12:
   kernel/bpf/verifier.c: In function 'jit_subprogs':
   include/linux/filter.h:363:4: warning: cast between incompatible function types from 'unsigned int (*)(const void *, const struct bpf_insn *)' to 'u64 (*)(u64,  u64,  u64,  u64,  u64)' {aka 'long long unsigned int (*)(long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int)'} [-Wcast-function-type]
     363 |   ((u64 (*)(u64, u64, u64, u64, u64))(x))
         |    ^
   kernel/bpf/verifier.c:11865:16: note: in expansion of macro 'BPF_CAST_CALL'
   11865 |    insn->imm = BPF_CAST_CALL(func[subprog]->bpf_func) -
         |                ^~~~~~~~~~~~~
   kernel/bpf/verifier.c: In function 'do_misc_fixups':
   include/linux/filter.h:363:4: warning: cast between incompatible function types from 'void * (* const)(struct bpf_map *, void *)' to 'u64 (*)(u64,  u64,  u64,  u64,  u64)' {aka 'long long unsigned int (*)(long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int)'} [-Wcast-function-type]
     363 |   ((u64 (*)(u64, u64, u64, u64, u64))(x))
         |    ^
   kernel/bpf/verifier.c:12273:17: note: in expansion of macro 'BPF_CAST_CALL'
   12273 |     insn->imm = BPF_CAST_CALL(ops->map_lookup_elem) -
         |                 ^~~~~~~~~~~~~
   include/linux/filter.h:363:4: warning: cast between incompatible function types from 'int (* const)(struct bpf_map *, void *, void *, u64)' {aka 'int (* const)(struct bpf_map *, void *, void *, long long unsigned int)'} to 'u64 (*)(u64,  u64,  u64,  u64,  u64)' {aka 'long long unsigned int (*)(long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int)'} [-Wcast-function-type]
     363 |   ((u64 (*)(u64, u64, u64, u64, u64))(x))
         |    ^
   kernel/bpf/verifier.c:12277:17: note: in expansion of macro 'BPF_CAST_CALL'
   12277 |     insn->imm = BPF_CAST_CALL(ops->map_update_elem) -
         |                 ^~~~~~~~~~~~~
   include/linux/filter.h:363:4: warning: cast between incompatible function types from 'int (* const)(struct bpf_map *, void *)' to 'u64 (*)(u64,  u64,  u64,  u64,  u64)' {aka 'long long unsigned int (*)(long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int)'} [-Wcast-function-type]
     363 |   ((u64 (*)(u64, u64, u64, u64, u64))(x))
         |    ^
   kernel/bpf/verifier.c:12281:17: note: in expansion of macro 'BPF_CAST_CALL'
   12281 |     insn->imm = BPF_CAST_CALL(ops->map_delete_elem) -
         |                 ^~~~~~~~~~~~~
   include/linux/filter.h:363:4: warning: cast between incompatible function types from 'int (* const)(struct bpf_map *, void *, u64)' {aka 'int (* const)(struct bpf_map *, void *, long long unsigned int)'} to 'u64 (*)(u64,  u64,  u64,  u64,  u64)' {aka 'long long unsigned int (*)(long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int)'} [-Wcast-function-type]
     363 |   ((u64 (*)(u64, u64, u64, u64, u64))(x))
         |    ^
   kernel/bpf/verifier.c:12285:17: note: in expansion of macro 'BPF_CAST_CALL'
   12285 |     insn->imm = BPF_CAST_CALL(ops->map_push_elem) -
         |                 ^~~~~~~~~~~~~
   include/linux/filter.h:363:4: warning: cast between incompatible function types from 'int (* const)(struct bpf_map *, void *)' to 'u64 (*)(u64,  u64,  u64,  u64,  u64)' {aka 'long long unsigned int (*)(long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int)'} [-Wcast-function-type]
     363 |   ((u64 (*)(u64, u64, u64, u64, u64))(x))
         |    ^
   kernel/bpf/verifier.c:12289:17: note: in expansion of macro 'BPF_CAST_CALL'
   12289 |     insn->imm = BPF_CAST_CALL(ops->map_pop_elem) -
         |                 ^~~~~~~~~~~~~
   include/linux/filter.h:363:4: warning: cast between incompatible function types from 'int (* const)(struct bpf_map *, void *)' to 'u64 (*)(u64,  u64,  u64,  u64,  u64)' {aka 'long long unsigned int (*)(long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int)'} [-Wcast-function-type]
     363 |   ((u64 (*)(u64, u64, u64, u64, u64))(x))
         |    ^
   kernel/bpf/verifier.c:12293:17: note: in expansion of macro 'BPF_CAST_CALL'
   12293 |     insn->imm = BPF_CAST_CALL(ops->map_peek_elem) -
         |                 ^~~~~~~~~~~~~
   include/linux/filter.h:363:4: warning: cast between incompatible function types from 'int (* const)(struct bpf_map *, u32,  u64)' {aka 'int (* const)(struct bpf_map *, unsigned int,  long long unsigned int)'} to 'u64 (*)(u64,  u64,  u64,  u64,  u64)' {aka 'long long unsigned int (*)(long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int,  long long unsigned int)'} [-Wcast-function-type]
     363 |   ((u64 (*)(u64, u64, u64, u64, u64))(x))
         |    ^
   kernel/bpf/verifier.c:12297:17: note: in expansion of macro 'BPF_CAST_CALL'
   12297 |     insn->imm = BPF_CAST_CALL(ops->map_redirect) -
         |                 ^~~~~~~~~~~~~


vim +/check_bpf_snprintf_call +5735 kernel/bpf/verifier.c

  5734	
> 5735	int check_bpf_snprintf_call(struct bpf_verifier_env *env,
  5736				    struct bpf_reg_state *regs)
  5737	{
  5738		struct bpf_reg_state *fmt_reg = &regs[BPF_REG_3];
  5739		struct bpf_reg_state *data_len_reg = &regs[BPF_REG_5];
  5740		struct bpf_map *fmt_map = fmt_reg->map_ptr;
  5741		int err, fmt_map_off, i, fmt_cnt = 0, memcpy_cnt = 0, num_args;
  5742		u64 fmt_addr;
  5743		char *fmt;
  5744	
  5745		/* data must be an array of u64 so data_len must be a multiple of 8 */
  5746		if (data_len_reg->var_off.value & 7)
  5747			return -EINVAL;
  5748		num_args = data_len_reg->var_off.value / 8;
  5749	
  5750		/* fmt being ARG_PTR_TO_CONST_STR guarantees that var_off is const
  5751		 * and map_direct_value_addr is set.
  5752		 */
  5753		fmt_map_off = fmt_reg->off + fmt_reg->var_off.value;
  5754		err = fmt_map->ops->map_direct_value_addr(fmt_map, &fmt_addr,
  5755							  fmt_map_off);
  5756		if (err)
  5757			return err;
  5758		fmt = (char *)fmt_addr;
  5759	
  5760		/* We are also guaranteed that fmt+fmt_map_off is NULL terminated, we
  5761		 * can focus on validating the format specifiers.
  5762		 */
  5763		for (i = fmt_map_off; fmt[i] != '\0'; i++) {
  5764			if ((!isprint(fmt[i]) && !isspace(fmt[i])) ||
  5765			    !isascii(fmt[i])) {
  5766				verbose(env, "only printable ascii for now\n");
  5767				return -EINVAL;
  5768			}
  5769	
  5770			if (fmt[i] != '%')
  5771				continue;
  5772	
  5773			if (fmt[i + 1] == '%') {
  5774				i++;
  5775				continue;
  5776			}
  5777	
  5778			if (fmt_cnt >= MAX_SNPRINTF_VARARGS) {
  5779				verbose(env, "too many format specifiers\n");
  5780				return -E2BIG;
  5781			}
  5782	
  5783			if (fmt_cnt >= num_args) {
  5784				verbose(env, "not enough parameters to print\n");
  5785				return -EINVAL;
  5786			}
  5787	
  5788			/* fmt[i] != 0 && fmt[last] == 0, so we can access fmt[i + 1] */
  5789			i++;
  5790	
  5791			/* skip optional "[0 +-][num]" width formating field */
  5792			while (fmt[i] == '0' || fmt[i] == '+'  || fmt[i] == '-' ||
  5793			       fmt[i] == ' ')
  5794				i++;
  5795			if (fmt[i] >= '1' && fmt[i] <= '9') {
  5796				i++;
  5797				while (fmt[i] >= '0' && fmt[i] <= '9')
  5798					i++;
  5799			}
  5800	
  5801			if (fmt[i] == 's') {
  5802				if (memcpy_cnt >= MAX_SNPRINTF_MEMCPY) {
  5803					verbose(env, "too many buffer copies\n");
  5804					return -E2BIG;
  5805				}
  5806	
  5807				fmt_cnt++;
  5808				memcpy_cnt++;
  5809				continue;
  5810			}
  5811	
  5812			if (fmt[i] == 'p') {
  5813				if (fmt[i + 1] == 0 ||  fmt[i + 1] == 'K' ||
  5814				    fmt[i + 1] == 'x' || fmt[i + 1] == 'B' ||
  5815				    fmt[i + 1] == 's' || fmt[i + 1] == 'S') {
  5816					fmt_cnt++;
  5817					continue;
  5818				}
  5819	
  5820				/* only support "%pI4", "%pi4", "%pI6" and "%pi6". */
  5821				if (fmt[i + 1] != 'i' && fmt[i + 1] != 'I') {
  5822					verbose(env, "invalid specifier %%p%c\n",
  5823						fmt[i+1]);
  5824					return -EINVAL;
  5825				}
  5826				if (fmt[i + 2] != '4' && fmt[i + 2] != '6') {
  5827					verbose(env, "invalid specifier %%p%c%c\n",
  5828						fmt[i+1], fmt[i+2]);
  5829					return -EINVAL;
  5830				}
  5831	
  5832				if (memcpy_cnt >= MAX_SNPRINTF_MEMCPY) {
  5833					verbose(env, "too many buffer copies\n");
  5834					return -E2BIG;
  5835				}
  5836	
  5837				i += 2;
  5838				fmt_cnt++;
  5839				memcpy_cnt++;
  5840				continue;
  5841			}
  5842	
  5843			if (fmt[i] == 'l') {
  5844				i++;
  5845				if (fmt[i] == 'l')
  5846					i++;
  5847			}
  5848	
  5849			if (fmt[i] != 'i' && fmt[i] != 'd' && fmt[i] != 'u' &&
  5850			    fmt[i] != 'x' && fmt[i] != 'X') {
  5851				verbose(env, "invalid format specifier %%%c\n", fmt[i]);
  5852				return -EINVAL;
  5853			}
  5854	
  5855			fmt_cnt++;
  5856		}
  5857	
  5858		if (fmt_cnt != num_args) {
  5859			verbose(env, "too many parameters to print\n");
  5860			return -EINVAL;
  5861		}
  5862	
  5863		return 0;
  5864	}
  5865	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29308 bytes --]

Re: [PATCH bpf-next 1/5] bpf: Add a ARG_PTR_TO_CONST_STR argument type

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 10816 bytes --]

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/bpf-Add-a-ARG_PTR_TO_CONST_STR-argument-type/20210311-070306
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: i386-randconfig-s001-20210308 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.3-262-g5e674421-dirty
        # https://github.com/0day-ci/linux/commit/cbb95ec99fafe0955aeada270c9be3d1477c3866
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Florent-Revest/bpf-Add-a-ARG_PTR_TO_CONST_STR-argument-type/20210311-070306
        git checkout cbb95ec99fafe0955aeada270c9be3d1477c3866
        # 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 <lkp@intel.com>


"sparse warnings: (new ones prefixed by >>)"
   kernel/bpf/verifier.c:11728:76: sparse: sparse: subtraction of functions? Share your drugs
   kernel/bpf/verifier.c:12136:81: sparse: sparse: subtraction of functions? Share your drugs
   kernel/bpf/verifier.c:12140:81: sparse: sparse: subtraction of functions? Share your drugs
   kernel/bpf/verifier.c:12144:81: sparse: sparse: subtraction of functions? Share your drugs
   kernel/bpf/verifier.c:12148:79: sparse: sparse: subtraction of functions? Share your drugs
   kernel/bpf/verifier.c:12152:78: sparse: sparse: subtraction of functions? Share your drugs
   kernel/bpf/verifier.c:12156:79: sparse: sparse: subtraction of functions? Share your drugs
   kernel/bpf/verifier.c:12160:78: sparse: sparse: subtraction of functions? Share your drugs
   kernel/bpf/verifier.c:12204:38: sparse: sparse: subtraction of functions? Share your drugs
>> kernel/bpf/verifier.c:4918:36: sparse: sparse: non size-preserving integer to pointer cast

vim +4918 kernel/bpf/verifier.c

  4695	
  4696	static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
  4697				  struct bpf_call_arg_meta *meta,
  4698				  const struct bpf_func_proto *fn)
  4699	{
  4700		u32 regno = BPF_REG_1 + arg;
  4701		struct bpf_reg_state *regs = cur_regs(env), *reg = &regs[regno];
  4702		enum bpf_arg_type arg_type = fn->arg_type[arg];
  4703		enum bpf_reg_type type = reg->type;
  4704		int err = 0;
  4705	
  4706		if (arg_type == ARG_DONTCARE)
  4707			return 0;
  4708	
  4709		err = check_reg_arg(env, regno, SRC_OP);
  4710		if (err)
  4711			return err;
  4712	
  4713		if (arg_type == ARG_ANYTHING) {
  4714			if (is_pointer_value(env, regno)) {
  4715				verbose(env, "R%d leaks addr into helper function\n",
  4716					regno);
  4717				return -EACCES;
  4718			}
  4719			return 0;
  4720		}
  4721	
  4722		if (type_is_pkt_pointer(type) &&
  4723		    !may_access_direct_pkt_data(env, meta, BPF_READ)) {
  4724			verbose(env, "helper access to the packet is not allowed\n");
  4725			return -EACCES;
  4726		}
  4727	
  4728		if (arg_type == ARG_PTR_TO_MAP_VALUE ||
  4729		    arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE ||
  4730		    arg_type == ARG_PTR_TO_MAP_VALUE_OR_NULL) {
  4731			err = resolve_map_arg_type(env, meta, &arg_type);
  4732			if (err)
  4733				return err;
  4734		}
  4735	
  4736		if (register_is_null(reg) && arg_type_may_be_null(arg_type))
  4737			/* A NULL register has a SCALAR_VALUE type, so skip
  4738			 * type checking.
  4739			 */
  4740			goto skip_type_check;
  4741	
  4742		err = check_reg_type(env, regno, arg_type, fn->arg_btf_id[arg]);
  4743		if (err)
  4744			return err;
  4745	
  4746		if (type == PTR_TO_CTX) {
  4747			err = check_ctx_reg(env, reg, regno);
  4748			if (err < 0)
  4749				return err;
  4750		}
  4751	
  4752	skip_type_check:
  4753		if (reg->ref_obj_id) {
  4754			if (meta->ref_obj_id) {
  4755				verbose(env, "verifier internal error: more than one arg with ref_obj_id R%d %u %u\n",
  4756					regno, reg->ref_obj_id,
  4757					meta->ref_obj_id);
  4758				return -EFAULT;
  4759			}
  4760			meta->ref_obj_id = reg->ref_obj_id;
  4761		}
  4762	
  4763		if (arg_type == ARG_CONST_MAP_PTR) {
  4764			/* bpf_map_xxx(map_ptr) call: remember that map_ptr */
  4765			meta->map_ptr = reg->map_ptr;
  4766		} else if (arg_type == ARG_PTR_TO_MAP_KEY) {
  4767			/* bpf_map_xxx(..., map_ptr, ..., key) call:
  4768			 * check that [key, key + map->key_size) are within
  4769			 * stack limits and initialized
  4770			 */
  4771			if (!meta->map_ptr) {
  4772				/* in function declaration map_ptr must come before
  4773				 * map_key, so that it's verified and known before
  4774				 * we have to check map_key here. Otherwise it means
  4775				 * that kernel subsystem misconfigured verifier
  4776				 */
  4777				verbose(env, "invalid map_ptr to access map->key\n");
  4778				return -EACCES;
  4779			}
  4780			err = check_helper_mem_access(env, regno,
  4781						      meta->map_ptr->key_size, false,
  4782						      NULL);
  4783		} else if (arg_type == ARG_PTR_TO_MAP_VALUE ||
  4784			   (arg_type == ARG_PTR_TO_MAP_VALUE_OR_NULL &&
  4785			    !register_is_null(reg)) ||
  4786			   arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE) {
  4787			/* bpf_map_xxx(..., map_ptr, ..., value) call:
  4788			 * check [value, value + map->value_size) validity
  4789			 */
  4790			if (!meta->map_ptr) {
  4791				/* kernel subsystem misconfigured verifier */
  4792				verbose(env, "invalid map_ptr to access map->value\n");
  4793				return -EACCES;
  4794			}
  4795			meta->raw_mode = (arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE);
  4796			err = check_helper_mem_access(env, regno,
  4797						      meta->map_ptr->value_size, false,
  4798						      meta);
  4799		} else if (arg_type == ARG_PTR_TO_PERCPU_BTF_ID) {
  4800			if (!reg->btf_id) {
  4801				verbose(env, "Helper has invalid btf_id in R%d\n", regno);
  4802				return -EACCES;
  4803			}
  4804			meta->ret_btf = reg->btf;
  4805			meta->ret_btf_id = reg->btf_id;
  4806		} else if (arg_type == ARG_PTR_TO_SPIN_LOCK) {
  4807			if (meta->func_id == BPF_FUNC_spin_lock) {
  4808				if (process_spin_lock(env, regno, true))
  4809					return -EACCES;
  4810			} else if (meta->func_id == BPF_FUNC_spin_unlock) {
  4811				if (process_spin_lock(env, regno, false))
  4812					return -EACCES;
  4813			} else {
  4814				verbose(env, "verifier internal error\n");
  4815				return -EFAULT;
  4816			}
  4817		} else if (arg_type == ARG_PTR_TO_FUNC) {
  4818			meta->subprogno = reg->subprogno;
  4819		} else if (arg_type_is_mem_ptr(arg_type)) {
  4820			/* The access to this pointer is only checked when we hit the
  4821			 * next is_mem_size argument below.
  4822			 */
  4823			meta->raw_mode = (arg_type == ARG_PTR_TO_UNINIT_MEM);
  4824		} else if (arg_type_is_mem_size(arg_type)) {
  4825			bool zero_size_allowed = (arg_type == ARG_CONST_SIZE_OR_ZERO);
  4826	
  4827			/* This is used to refine r0 return value bounds for helpers
  4828			 * that enforce this value as an upper bound on return values.
  4829			 * See do_refine_retval_range() for helpers that can refine
  4830			 * the return value. C type of helper is u32 so we pull register
  4831			 * bound from umax_value however, if negative verifier errors
  4832			 * out. Only upper bounds can be learned because retval is an
  4833			 * int type and negative retvals are allowed.
  4834			 */
  4835			meta->msize_max_value = reg->umax_value;
  4836	
  4837			/* The register is SCALAR_VALUE; the access check
  4838			 * happens using its boundaries.
  4839			 */
  4840			if (!tnum_is_const(reg->var_off))
  4841				/* For unprivileged variable accesses, disable raw
  4842				 * mode so that the program is required to
  4843				 * initialize all the memory that the helper could
  4844				 * just partially fill up.
  4845				 */
  4846				meta = NULL;
  4847	
  4848			if (reg->smin_value < 0) {
  4849				verbose(env, "R%d min value is negative, either use unsigned or 'var &= const'\n",
  4850					regno);
  4851				return -EACCES;
  4852			}
  4853	
  4854			if (reg->umin_value == 0) {
  4855				err = check_helper_mem_access(env, regno - 1, 0,
  4856							      zero_size_allowed,
  4857							      meta);
  4858				if (err)
  4859					return err;
  4860			}
  4861	
  4862			if (reg->umax_value >= BPF_MAX_VAR_SIZ) {
  4863				verbose(env, "R%d unbounded memory access, use 'var &= const' or 'if (var < const)'\n",
  4864					regno);
  4865				return -EACCES;
  4866			}
  4867			err = check_helper_mem_access(env, regno - 1,
  4868						      reg->umax_value,
  4869						      zero_size_allowed, meta);
  4870			if (!err)
  4871				err = mark_chain_precision(env, regno);
  4872		} else if (arg_type_is_alloc_size(arg_type)) {
  4873			if (!tnum_is_const(reg->var_off)) {
  4874				verbose(env, "R%d is not a known constant'\n",
  4875					regno);
  4876				return -EACCES;
  4877			}
  4878			meta->mem_size = reg->var_off.value;
  4879		} else if (arg_type_is_int_ptr(arg_type)) {
  4880			int size = int_ptr_type_to_size(arg_type);
  4881	
  4882			err = check_helper_mem_access(env, regno, size, false, meta);
  4883			if (err)
  4884				return err;
  4885			err = check_ptr_alignment(env, reg, 0, size, true);
  4886		} else if (arg_type == ARG_PTR_TO_CONST_STR) {
  4887			struct bpf_map *map = reg->map_ptr;
  4888			int map_off, i;
  4889			u64 map_addr;
  4890			char *map_ptr;
  4891	
  4892			if (!map || !bpf_map_is_rdonly(map)) {
  4893				verbose(env, "R%d does not point to a readonly map'\n", regno);
  4894				return -EACCES;
  4895			}
  4896	
  4897			if (!tnum_is_const(reg->var_off)) {
  4898				verbose(env, "R%d is not a constant address'\n", regno);
  4899				return -EACCES;
  4900			}
  4901	
  4902			if (!map->ops->map_direct_value_addr) {
  4903				verbose(env, "no direct value access support for this map type\n");
  4904				return -EACCES;
  4905			}
  4906	
  4907			err = check_helper_mem_access(env, regno,
  4908						      map->value_size - reg->off,
  4909						      false, meta);
  4910			if (err)
  4911				return err;
  4912	
  4913			map_off = reg->off + reg->var_off.value;
  4914			err = map->ops->map_direct_value_addr(map, &map_addr, map_off);
  4915			if (err)
  4916				return err;
  4917	
> 4918			map_ptr = (char *)(map_addr);
  4919			for (i = map_off; map_ptr[i] != '\0'; i++) {
  4920				if (i == map->value_size - 1) {
  4921					verbose(env, "map does not contain a NULL-terminated string\n");
  4922					return -EACCES;
  4923				}
  4924			}
  4925		}
  4926	
  4927		return err;
  4928	}
  4929	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 40991 bytes --]

Re: [PATCH bpf-next 2/5] bpf: Add a bpf_snprintf helper

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2277 bytes --]

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/bpf-Add-a-ARG_PTR_TO_CONST_STR-argument-type/20210311-070306
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: x86_64-randconfig-s022-20210310 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.3-262-g5e674421-dirty
        # https://github.com/0day-ci/linux/commit/c06d419a45370b897b7383625f0435873a7458fa
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Florent-Revest/bpf-Add-a-ARG_PTR_TO_CONST_STR-argument-type/20210311-070306
        git checkout c06d419a45370b897b7383625f0435873a7458fa
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


"sparse warnings: (new ones prefixed by >>)"
>> kernel/bpf/verifier.c:5735:5: sparse: sparse: symbol 'check_bpf_snprintf_call' was not declared. Should it be static?
   kernel/bpf/verifier.c:11865:76: sparse: sparse: subtraction of functions? Share your drugs
   kernel/bpf/verifier.c:12273:81: sparse: sparse: subtraction of functions? Share your drugs
   kernel/bpf/verifier.c:12277:81: sparse: sparse: subtraction of functions? Share your drugs
   kernel/bpf/verifier.c:12281:81: sparse: sparse: subtraction of functions? Share your drugs
   kernel/bpf/verifier.c:12285:79: sparse: sparse: subtraction of functions? Share your drugs
   kernel/bpf/verifier.c:12289:78: sparse: sparse: subtraction of functions? Share your drugs
   kernel/bpf/verifier.c:12293:79: sparse: sparse: subtraction of functions? Share your drugs
   kernel/bpf/verifier.c:12297:78: sparse: sparse: subtraction of functions? Share your drugs
   kernel/bpf/verifier.c:12341:38: sparse: sparse: subtraction of functions? Share your drugs

Please review and possibly fold the followup patch.

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 34370 bytes --]

[RFC PATCH] bpf: check_bpf_snprintf_call() can be static

[kernel test robot]

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: kernel test robot <lkp@intel.com>
---
 verifier.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 3ab549df817b6..06c868989852d 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -5732,8 +5732,8 @@ static int check_reference_leak(struct bpf_verifier_env *env)
 	return state->acquired_refs ? -EINVAL : 0;
 }
 
-int check_bpf_snprintf_call(struct bpf_verifier_env *env,
-			    struct bpf_reg_state *regs)
+static int check_bpf_snprintf_call(struct bpf_verifier_env *env,
+				   struct bpf_reg_state *regs)
 {
 	struct bpf_reg_state *fmt_reg = &regs[BPF_REG_3];
 	struct bpf_reg_state *data_len_reg = &regs[BPF_REG_5];

Re: [PATCH bpf-next 1/5] bpf: Add a ARG_PTR_TO_CONST_STR argument type

[Andrii Nakryiko]

On Wed, Mar 10, 2021 at 2:02 PM Florent Revest <revest@chromium.org> wrote:
>
> This type provides the guarantee that an argument is going to be a const
> pointer to somewhere in a read-only map value. It also checks that this
> pointer is followed by a NULL character before the end of the map value.
>
> Signed-off-by: Florent Revest <revest@chromium.org>
> ---
>  include/linux/bpf.h   |  1 +
>  kernel/bpf/verifier.c | 41 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 42 insertions(+)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index a25730eaa148..7b5319d75b3e 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -308,6 +308,7 @@ enum bpf_arg_type {
>         ARG_PTR_TO_PERCPU_BTF_ID,       /* pointer to in-kernel percpu type */
>         ARG_PTR_TO_FUNC,        /* pointer to a bpf program function */
>         ARG_PTR_TO_STACK_OR_NULL,       /* pointer to stack or NULL */
> +       ARG_PTR_TO_CONST_STR,   /* pointer to a null terminated read-only string */
>         __BPF_ARG_TYPE_MAX,
>  };
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index f9096b049cd6..c99b2b67dc8d 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -4601,6 +4601,7 @@ static const struct bpf_reg_types spin_lock_types = { .types = { PTR_TO_MAP_VALU
>  static const struct bpf_reg_types percpu_btf_ptr_types = { .types = { PTR_TO_PERCPU_BTF_ID } };
>  static const struct bpf_reg_types func_ptr_types = { .types = { PTR_TO_FUNC } };
>  static const struct bpf_reg_types stack_ptr_types = { .types = { PTR_TO_STACK } };
> +static const struct bpf_reg_types const_str_ptr_types = { .types = { PTR_TO_MAP_VALUE } };
>
>  static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = {
>         [ARG_PTR_TO_MAP_KEY]            = &map_key_value_types,
> @@ -4631,6 +4632,7 @@ static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = {
>         [ARG_PTR_TO_PERCPU_BTF_ID]      = &percpu_btf_ptr_types,
>         [ARG_PTR_TO_FUNC]               = &func_ptr_types,
>         [ARG_PTR_TO_STACK_OR_NULL]      = &stack_ptr_types,
> +       [ARG_PTR_TO_CONST_STR]          = &const_str_ptr_types,
>  };
>
>  static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
> @@ -4881,6 +4883,45 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
>                 if (err)
>                         return err;
>                 err = check_ptr_alignment(env, reg, 0, size, true);
> +       } else if (arg_type == ARG_PTR_TO_CONST_STR) {
> +               struct bpf_map *map = reg->map_ptr;
> +               int map_off, i;
> +               u64 map_addr;
> +               char *map_ptr;
> +
> +               if (!map || !bpf_map_is_rdonly(map)) {
> +                       verbose(env, "R%d does not point to a readonly map'\n", regno);
> +                       return -EACCES;
> +               }
> +
> +               if (!tnum_is_const(reg->var_off)) {
> +                       verbose(env, "R%d is not a constant address'\n", regno);
> +                       return -EACCES;
> +               }
> +
> +               if (!map->ops->map_direct_value_addr) {
> +                       verbose(env, "no direct value access support for this map type\n");
> +                       return -EACCES;
> +               }
> +
> +               err = check_helper_mem_access(env, regno,
> +                                             map->value_size - reg->off,
> +                                             false, meta);

you expect reg to be PTR_TO_MAP_VALUE, so probably better to directly
use check_map_access(). And double-check that register is of expected
type. just the presence of ref->map_ptr might not be sufficient?

> +               if (err)
> +                       return err;
> +
> +               map_off = reg->off + reg->var_off.value;
> +               err = map->ops->map_direct_value_addr(map, &map_addr, map_off);
> +               if (err)
> +                       return err;
> +
> +               map_ptr = (char *)(map_addr);

map_ptr is a very confusing name. str_ptr or value ptr?

> +               for (i = map_off; map_ptr[i] != '\0'; i++) {
> +                       if (i == map->value_size - 1) {

use strnchr()?

> +                               verbose(env, "map does not contain a NULL-terminated string\n");

map in the user-visible message is quite confusing, given that users
will probably use this through static variables, so maybe just "string
is not zero-terminated?" And it's not really a NULL, it's zero
character.

> +                               return -EACCES;
> +                       }
> +               }
>         }
>
>         return err;
> --
> 2.30.1.766.gb4fecdf3b7-goog
>

Re: [PATCH bpf-next 2/5] bpf: Add a bpf_snprintf helper

[Andrii Nakryiko]

On Wed, Mar 10, 2021 at 2:02 PM Florent Revest <revest@chromium.org> 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. But because ARG_PTR_TO_CONST_STR guarantees to point to a
> NULL-terminated read-only map, we don't need a format string length arg.
>
> Because the format-string is known at verification time, we also move
> most of the format string validation, currently done in formatting
> helper calls, into the verifier logic. This makes debugging easier and
> also slightly improves the runtime performance.
>
> Signed-off-by: Florent Revest <revest@chromium.org>
> ---
>  include/linux/bpf.h            |   4 +
>  include/uapi/linux/bpf.h       |  28 +++++++
>  kernel/bpf/verifier.c          | 137 +++++++++++++++++++++++++++++++++
>  kernel/trace/bpf_trace.c       | 110 ++++++++++++++++++++++++++
>  tools/include/uapi/linux/bpf.h |  28 +++++++
>  5 files changed, 307 insertions(+)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 7b5319d75b3e..d78175c9a887 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1902,6 +1902,10 @@ extern const struct bpf_func_proto bpf_task_storage_get_proto;
>  extern const struct bpf_func_proto bpf_task_storage_delete_proto;
>  extern const struct bpf_func_proto bpf_for_each_map_elem_proto;
>
> +#define MAX_SNPRINTF_VARARGS           12
> +#define MAX_SNPRINTF_MEMCPY            6
> +#define MAX_SNPRINTF_STR_LEN           128
> +
>  const struct bpf_func_proto *bpf_tracing_func_proto(
>         enum bpf_func_id func_id, const struct bpf_prog *prog);
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 2d3036e292a9..3cbdc8ae00e7 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -4660,6 +4660,33 @@ union bpf_attr {
>   *     Return
>   *             The number of traversed map elements for success, **-EINVAL** for
>   *             invalid **flags**.
> + *
> + * long bpf_snprintf(char *out, u32 out_size, const char *fmt, u64 *data, u32 data_len)

bpf_snprintf_btf calls out and out_size str and str_size, let's be consistent?

> + *     Description
> + *             Outputs a string into the **out** buffer of size **out_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 printed string, including
> + *             the trailing NUL character. If the return value is greater than
> + *             **out_size**, **out** contains a truncated string, without a
> + *             trailing NULL character.

this deviates from the behavior in other BPF helpers dealing with
strings. and it's extremely inconvenient for users to get
non-zero-terminated string. I think we should always zero-terminate.

> + *
> + *             Or **-EBUSY** if the per-CPU memory copy buffer is busy.
>   */
>  #define __BPF_FUNC_MAPPER(FN)          \
>         FN(unspec),                     \
> @@ -4827,6 +4854,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/verifier.c b/kernel/bpf/verifier.c
> index c99b2b67dc8d..3ab549df817b 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -5732,6 +5732,137 @@ static int check_reference_leak(struct bpf_verifier_env *env)
>         return state->acquired_refs ? -EINVAL : 0;
>  }
>
> +int check_bpf_snprintf_call(struct bpf_verifier_env *env,
> +                           struct bpf_reg_state *regs)
> +{

can we please extra the printf format string parsing/checking logic
and re-use them across all functions? We now have at least 4 variants
of it, it's not great to say the least. I hope it's possible to
generalize it in such a way that the same function will parse the
string, and will record each expected argument and it's type, with
whatever extra flags we need to. That should make the printing part
simpler as well, as it will just follow "directions" from the parsing
part? Devil is in the details, of course :) But it's worthwhile to try
at least.

> +       struct bpf_reg_state *fmt_reg = &regs[BPF_REG_3];
> +       struct bpf_reg_state *data_len_reg = &regs[BPF_REG_5];
> +       struct bpf_map *fmt_map = fmt_reg->map_ptr;
> +       int err, fmt_map_off, i, fmt_cnt = 0, memcpy_cnt = 0, num_args;
> +       u64 fmt_addr;
> +       char *fmt;
> +
> +       /* data must be an array of u64 so data_len must be a multiple of 8 */
> +       if (data_len_reg->var_off.value & 7)
> +               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;
> +

[...] not fun to read this part over and over :)

> +       }
> +
> +       return 0;
> +}
> +
>  static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>                              int *insn_idx_p)
>  {
> @@ -5846,6 +5977,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 0d23755c2747..7b80759c10a9 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -1271,6 +1271,114 @@ const struct bpf_func_proto bpf_snprintf_btf_proto = {
>         .arg5_type      = ARG_ANYTHING,
>  };
>
> +struct bpf_snprintf_buf {
> +       char buf[MAX_SNPRINTF_MEMCPY][MAX_SNPRINTF_STR_LEN];
> +};
> +static DEFINE_PER_CPU(struct bpf_snprintf_buf, bpf_snprintf_buf);
> +static DEFINE_PER_CPU(int, bpf_snprintf_buf_used);
> +
> +BPF_CALL_5(bpf_snprintf, char *, out, u32, out_size, char *, fmt, u64 *, args,
> +          u32, args_len)
> +{
> +       int err, i, buf_used, copy_size, fmt_cnt = 0, memcpy_cnt = 0;
> +       u64 params[MAX_SNPRINTF_VARARGS];
> +       struct bpf_snprintf_buf *bufs;
> +
> +       buf_used = this_cpu_inc_return(bpf_snprintf_buf_used);
> +       if (WARN_ON_ONCE(buf_used > 1)) {
> +               err = -EBUSY;
> +               goto out;
> +       }
> +
> +       bufs = this_cpu_ptr(&bpf_snprintf_buf);
> +
> +       /*
> +        * The verifier has already done most of the heavy-work for us in
> +        * check_bpf_snprintf_call. We know that fmt is well formatted and that
> +        * args_len is valid. The only task left is to convert some of the
> +        * arguments. For the %s and %pi* specifiers, we need to read buffers
> +        * from a kernel address during the helper call.
> +        */
> +       for (i = 0; fmt[i] != '\0'; i++) {

same function should hopefully be reused here

> +       }
> +
> +       /* Maximumly we can have MAX_SNPRINTF_VARARGS parameters, just give
> +        * all of them to snprintf().
> +        */
> +       err = snprintf(out, out_size, 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]) + 1;
> +
> +out:
> +       this_cpu_dec(bpf_snprintf_buf_used);
> +       return err;
> +}
> +
> +static const struct bpf_func_proto bpf_snprintf_proto = {
> +       .func           = bpf_snprintf,
> +       .gpl_only       = true,
> +       .ret_type       = RET_INTEGER,
> +       .arg1_type      = ARG_PTR_TO_MEM,
> +       .arg2_type      = ARG_CONST_SIZE,

can we mark is CONST_SIZE_OR_ZERO and just do nothing on zero at
runtime? I still have scars from having to deal (prove, actually) with
ARG_CONST_SIZE (> 0) limitations in perf_event_output. No need to make
anyone's life harder, if it's easy to just do something sensible on
zero (i.e., do nothing, but emit desired amount of bytes).

> +       .arg3_type      = ARG_PTR_TO_CONST_STR,
> +       .arg4_type      = ARG_PTR_TO_MEM,
> +       .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)
>  {
> @@ -1373,6 +1481,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;

why just tracing? can't all BPF programs use this functionality?

>         default:
>                 return NULL;
>         }
> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index 2d3036e292a9..3cbdc8ae00e7 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -4660,6 +4660,33 @@ union bpf_attr {
>   *     Return
>   *             The number of traversed map elements for success, **-EINVAL** for
>   *             invalid **flags**.
> + *
> + * long bpf_snprintf(char *out, u32 out_size, const char *fmt, u64 *data, u32 data_len)
> + *     Description
> + *             Outputs a string into the **out** buffer of size **out_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 printed string, including
> + *             the trailing NUL character. If the return value is greater than
> + *             **out_size**, **out** contains a truncated string, without a
> + *             trailing NULL character.
> + *
> + *             Or **-EBUSY** if the per-CPU memory copy buffer is busy.
>   */
>  #define __BPF_FUNC_MAPPER(FN)          \
>         FN(unspec),                     \
> @@ -4827,6 +4854,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.30.1.766.gb4fecdf3b7-goog
>

Re: [PATCH bpf-next 3/5] libbpf: Initialize the bpf_seq_printf parameters array field by field

[Andrii Nakryiko]

On Wed, Mar 10, 2021 at 2:02 PM Florent Revest <revest@chromium.org> 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.
>
> This is a workaround, ideally the rodata relocation should be supported
> by libbpf but this would require a disproportionate amount of work given
> the actual usecases. (it is very unlikely that one uses a const array of
> relocated addresses)
>
> Signed-off-by: Florent Revest <revest@chromium.org>
> ---
>  tools/lib/bpf/bpf_tracing.h | 30 +++++++++++++++++++++++++++++-
>  1 file changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
> index f9ef37707888..f6a2deb3cd5b 100644
> --- a/tools/lib/bpf/bpf_tracing.h
> +++ b/tools/lib/bpf/bpf_tracing.h
> @@ -413,6 +413,34 @@ typeof(name(0)) name(struct pt_regs *ctx)                              \
>  }                                                                          \
>  static __always_inline typeof(name(0)) ____##name(struct pt_regs *ctx, ##args)
>
> +#define ___bpf_build_param0(narg, x)
> +#define ___bpf_build_param1(narg, x) ___param[narg - 1] = x
> +#define ___bpf_build_param2(narg, x, args...) ___param[narg - 2] = x; \
> +                                             ___bpf_build_param1(narg, args)
> +#define ___bpf_build_param3(narg, x, args...) ___param[narg - 3] = x; \
> +                                             ___bpf_build_param2(narg, args)
> +#define ___bpf_build_param4(narg, x, args...) ___param[narg - 4] = x; \
> +                                             ___bpf_build_param3(narg, args)
> +#define ___bpf_build_param5(narg, x, args...) ___param[narg - 5] = x; \
> +                                             ___bpf_build_param4(narg, args)
> +#define ___bpf_build_param6(narg, x, args...) ___param[narg - 6] = x; \
> +                                             ___bpf_build_param5(narg, args)
> +#define ___bpf_build_param7(narg, x, args...) ___param[narg - 7] = x; \
> +                                             ___bpf_build_param6(narg, args)
> +#define ___bpf_build_param8(narg, x, args...) ___param[narg - 8] = x; \
> +                                             ___bpf_build_param7(narg, args)
> +#define ___bpf_build_param9(narg, x, args...) ___param[narg - 9] = x; \
> +                                             ___bpf_build_param8(narg, args)
> +#define ___bpf_build_param10(narg, x, args...) ___param[narg - 10] = x; \
> +                                              ___bpf_build_param9(narg, args)
> +#define ___bpf_build_param11(narg, x, args...) ___param[narg - 11] = x; \
> +                                              ___bpf_build_param10(narg, args)
> +#define ___bpf_build_param12(narg, x, args...) ___param[narg - 12] = x; \
> +                                              ___bpf_build_param11(narg, args)

took me some time to get why the [narg - 12] :) it makes sense, but
then I started wondering why not

#define ___bpf_build_param12(narg, x, args...)
___bpf_build_param11(narg, args); ___param[11] = x

? seems more straightforward, no?

also please keep all of them on single line. And to make lines
shorter, let's call it ___bpf_fillX? I also don't like hard-coded
___param, which is both inflexible and is obscure at the point of use
of this macro. So let's pass it as the first argument?

> +#define ___bpf_build_param(args...) \
> +       unsigned long long ___param[___bpf_narg(args)];                 \
> +       ___bpf_apply(___bpf_build_param, ___bpf_narg(args))(___bpf_narg(args), args)
> +

And here I'd pass array as a parameter and let caller define it, so
macro is literally just filling the array elements, not defining the
array itself and what's the type of elements

>  /*
>   * BPF_SEQ_PRINTF to wrap bpf_seq_printf to-be-printed values
>   * in a structure.
> @@ -422,7 +450,7 @@ static __always_inline typeof(name(0)) ____##name(struct pt_regs *ctx, ##args)
>                 _Pragma("GCC diagnostic push")                              \
>                 _Pragma("GCC diagnostic ignored \"-Wint-conversion\"")      \
>                 static const char ___fmt[] = fmt;                           \
> -               unsigned long long ___param[] = { args };                   \
> +               ___bpf_build_param(args);                                   \
>                 _Pragma("GCC diagnostic pop")                               \
>                 int ___ret = bpf_seq_printf(seq, ___fmt, sizeof(___fmt),    \
>                                             ___param, sizeof(___param));    \

here you are violating separation of variables and code,
___bpf_build_param is defining a variable, then has code statements,
then you are declaring ___ret after the code. So please split ___ret
definition,

> --
> 2.30.1.766.gb4fecdf3b7-goog
>

Re: [PATCH bpf-next 4/5] libbpf: Introduce a BPF_SNPRINTF helper macro

[Andrii Nakryiko]

On Wed, Mar 10, 2021 at 2:02 PM Florent Revest <revest@chromium.org> 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 <revest@chromium.org>
> ---
>  tools/lib/bpf/bpf_tracing.h | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
> index f6a2deb3cd5b..89e82da9b8a0 100644
> --- a/tools/lib/bpf/bpf_tracing.h
> +++ b/tools/lib/bpf/bpf_tracing.h
> @@ -457,4 +457,19 @@ static __always_inline typeof(name(0)) ____##name(struct pt_regs *ctx, ##args)
>                 ___ret;                                                     \
>         })
>
> +/*
> + * BPF_SNPRINTF wraps the bpf_snprintf helper with variadic arguments instead of
> + * an array of u64.
> + */
> +#define BPF_SNPRINTF(out, out_size, fmt, args...)                          \
> +       ({                                                                  \
> +               _Pragma("GCC diagnostic push")                              \
> +               _Pragma("GCC diagnostic ignored \"-Wint-conversion\"")      \
> +               ___bpf_build_param(args);                                   \
> +               _Pragma("GCC diagnostic pop")                               \
> +               int ___ret = bpf_snprintf(out, out_size, fmt,               \
> +                                           ___param, sizeof(___param));    \

same problem, mixing variable declarations and code

> +               ___ret;                                                     \
> +       })
> +
>  #endif
> --
> 2.30.1.766.gb4fecdf3b7-goog
>

Re: [PATCH bpf-next 3/5] libbpf: Initialize the bpf_seq_printf parameters array field by field

[Andrii Nakryiko]

On Mon, Mar 15, 2021 at 9:36 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Wed, Mar 10, 2021 at 2:02 PM Florent Revest <revest@chromium.org> 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.
> >
> > This is a workaround, ideally the rodata relocation should be supported
> > by libbpf but this would require a disproportionate amount of work given
> > the actual usecases. (it is very unlikely that one uses a const array of
> > relocated addresses)

Can you please drop this paragraph? This is not a workaround, it's a
completely working code that should continue working. And this is not
something that libbpf doesn't support, there is no kernel interface to
make it work at all.

Please add Fixes: tag as well.

> >
> > Signed-off-by: Florent Revest <revest@chromium.org>
> > ---
> >  tools/lib/bpf/bpf_tracing.h | 30 +++++++++++++++++++++++++++++-
> >  1 file changed, 29 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
> > index f9ef37707888..f6a2deb3cd5b 100644
> > --- a/tools/lib/bpf/bpf_tracing.h
> > +++ b/tools/lib/bpf/bpf_tracing.h
> > @@ -413,6 +413,34 @@ typeof(name(0)) name(struct pt_regs *ctx)                              \
> >  }                                                                          \
> >  static __always_inline typeof(name(0)) ____##name(struct pt_regs *ctx, ##args)
> >
> > +#define ___bpf_build_param0(narg, x)
> > +#define ___bpf_build_param1(narg, x) ___param[narg - 1] = x
> > +#define ___bpf_build_param2(narg, x, args...) ___param[narg - 2] = x; \
> > +                                             ___bpf_build_param1(narg, args)
> > +#define ___bpf_build_param3(narg, x, args...) ___param[narg - 3] = x; \
> > +                                             ___bpf_build_param2(narg, args)
> > +#define ___bpf_build_param4(narg, x, args...) ___param[narg - 4] = x; \
> > +                                             ___bpf_build_param3(narg, args)
> > +#define ___bpf_build_param5(narg, x, args...) ___param[narg - 5] = x; \
> > +                                             ___bpf_build_param4(narg, args)
> > +#define ___bpf_build_param6(narg, x, args...) ___param[narg - 6] = x; \
> > +                                             ___bpf_build_param5(narg, args)
> > +#define ___bpf_build_param7(narg, x, args...) ___param[narg - 7] = x; \
> > +                                             ___bpf_build_param6(narg, args)
> > +#define ___bpf_build_param8(narg, x, args...) ___param[narg - 8] = x; \
> > +                                             ___bpf_build_param7(narg, args)
> > +#define ___bpf_build_param9(narg, x, args...) ___param[narg - 9] = x; \
> > +                                             ___bpf_build_param8(narg, args)
> > +#define ___bpf_build_param10(narg, x, args...) ___param[narg - 10] = x; \
> > +                                              ___bpf_build_param9(narg, args)
> > +#define ___bpf_build_param11(narg, x, args...) ___param[narg - 11] = x; \
> > +                                              ___bpf_build_param10(narg, args)
> > +#define ___bpf_build_param12(narg, x, args...) ___param[narg - 12] = x; \
> > +                                              ___bpf_build_param11(narg, args)
>
> took me some time to get why the [narg - 12] :) it makes sense, but
> then I started wondering why not
>
> #define ___bpf_build_param12(narg, x, args...)
> ___bpf_build_param11(narg, args); ___param[11] = x
>
> ? seems more straightforward, no?
>
> also please keep all of them on single line. And to make lines
> shorter, let's call it ___bpf_fillX? I also don't like hard-coded
> ___param, which is both inflexible and is obscure at the point of use
> of this macro. So let's pass it as the first argument?
>
> > +#define ___bpf_build_param(args...) \
> > +       unsigned long long ___param[___bpf_narg(args)];                 \
> > +       ___bpf_apply(___bpf_build_param, ___bpf_narg(args))(___bpf_narg(args), args)
> > +
>
> And here I'd pass array as a parameter and let caller define it, so
> macro is literally just filling the array elements, not defining the
> array itself and what's the type of elements
>
> >  /*
> >   * BPF_SEQ_PRINTF to wrap bpf_seq_printf to-be-printed values
> >   * in a structure.
> > @@ -422,7 +450,7 @@ static __always_inline typeof(name(0)) ____##name(struct pt_regs *ctx, ##args)
> >                 _Pragma("GCC diagnostic push")                              \
> >                 _Pragma("GCC diagnostic ignored \"-Wint-conversion\"")      \
> >                 static const char ___fmt[] = fmt;                           \
> > -               unsigned long long ___param[] = { args };                   \
> > +               ___bpf_build_param(args);                                   \
> >                 _Pragma("GCC diagnostic pop")                               \
> >                 int ___ret = bpf_seq_printf(seq, ___fmt, sizeof(___fmt),    \
> >                                             ___param, sizeof(___param));    \
>
> here you are violating separation of variables and code,
> ___bpf_build_param is defining a variable, then has code statements,
> then you are declaring ___ret after the code. So please split ___ret
> definition,
>
> > --
> > 2.30.1.766.gb4fecdf3b7-goog
> >

Re: [PATCH bpf-next 5/5] selftests/bpf: Add a series of tests for bpf_snprintf

[Andrii Nakryiko]

On Wed, Mar 10, 2021 at 2:02 PM Florent Revest <revest@chromium.org> wrote:
>
> This exercices most of the format specifiers when things go well.

typo: exercises

>
> Signed-off-by: Florent Revest <revest@chromium.org>
> ---
>  .../selftests/bpf/prog_tests/snprintf.c       | 71 +++++++++++++++++++
>  .../selftests/bpf/progs/test_snprintf.c       | 71 +++++++++++++++++++
>  2 files changed, 142 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..23af1dbd1eeb
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/snprintf.c
> @@ -0,0 +1,71 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2021 Google LLC. */
> +
> +#include <test_progs.h>
> +#include "test_snprintf.skel.h"
> +
> +static int duration;

if you drop CHECK() below, you won't need duration here at all

> +
> +#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 {'%', 'o', 'v', 'e', 'r'}
> +#define EXP_OVER_RET 10
> +
> +void test_snprintf(void)
> +{
> +       char exp_addr_out[] = EXP_ADDR_OUT;
> +       char exp_over_out[] = EXP_OVER_OUT;
> +       char exp_sym_out[]  = EXP_SYM_OUT;
> +       struct test_snprintf *skel;
> +       int err;
> +
> +       skel = test_snprintf__open_and_load();
> +       if (CHECK(!skel, "skel_open", "failed to open and load skeleton\n"))

ASSERT_OK_PTR
> +               return;
> +
> +       err = test_snprintf__attach(skel);
> +       if (CHECK(err, "skel_attach", "skeleton attach failed: %d\n", err))

ASSERT_OK
> +               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_OK(memcmp(skel->bss->over_out, exp_over_out,
> +                        sizeof(exp_over_out)), "over_out");
> +       ASSERT_EQ(skel->bss->over_ret, EXP_OVER_RET, "over_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..6c8aa4988e69
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_snprintf.c
> @@ -0,0 +1,71 @@
> +// 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>
> +
> +#define OUT_LEN 64
> +
> +/* Integer types */
> +static const char num_fmt[] = "%d %u %x %li %llu %lX";
> +#define NUMBERS -8, 9, 150, -424242, 1337, 0xDABBAD00

here I actually don't get the point of #define, can you please just
inline them at the invocation place? I think that will be nicer and
simpler (and will match common usage pattern)

> +
> +char num_out[OUT_LEN] = {};
> +long num_ret = 0;
> +
> +/* IP addresses */
> +static const char ip_fmt[] = "%pi4 %pI6";
> +static const __u8 dummy_ipv4[] = {127, 0, 0, 1}; /* 127.0.0.1 */
> +static const __u32 dummy_ipv6[] = {0, 0, 0, bpf_htonl(1)}; /* ::1/128 */
> +#define IPS &dummy_ipv4, &dummy_ipv6
> +
> +char ip_out[OUT_LEN] = {};
> +long ip_ret = 0;
> +
> +/* Symbol lookup formatting */
> +static const char sym_fmt[] = "%ps %pS %pB";
> +extern const void schedule __ksym;
> +#define SYMBOLS &schedule, &schedule, &schedule
> +
> +char sym_out[OUT_LEN] = {};
> +long sym_ret = 0;
> +
> +/* Kernel pointers */
> +static const char addr_fmt[] = "%pK %px %p";
> +#define ADDRESSES 0, 0xFFFF00000ADD4E55, 0xFFFF00000ADD4E55
> +
> +char addr_out[OUT_LEN] = {};
> +long addr_ret = 0;
> +
> +/* Strings embedding */
> +static const char str_fmt[] = "%s %+05s";
> +static const char str1[] = "str1";
> +static const char longstr[] = "longstr";
> +#define STRINGS str1, longstr
> +
> +char str_out[OUT_LEN] = {};
> +long str_ret = 0;
> +
> +/* Overflow */
> +static const char over_fmt[] = "%%overflow";
> +
> +#define OVER_OUT_LEN 6
> +char over_out[OVER_OUT_LEN] = {};
> +long over_ret = 0;
> +

same for all the above #defines, tests will be easier to follow if you
just use value in BPF_SNPRINTF below

> +SEC("raw_tp/sys_enter")
> +int handler(const void *ctx)
> +{
> +       num_ret  = BPF_SNPRINTF(num_out,  OUT_LEN, num_fmt,  NUMBERS);
> +       ip_ret   = BPF_SNPRINTF(ip_out,   OUT_LEN, ip_fmt,   IPS);
> +       sym_ret  = BPF_SNPRINTF(sym_out,  OUT_LEN, sym_fmt,  SYMBOLS);
> +       addr_ret = BPF_SNPRINTF(addr_out, OUT_LEN, addr_fmt, ADDRESSES);
> +       str_ret  = BPF_SNPRINTF(str_out,  OUT_LEN, str_fmt,  STRINGS);
> +       over_ret = BPF_SNPRINTF(over_out, OVER_OUT_LEN, over_fmt);

in practice you'd do BPF_SNPRINTF(num_out, sizeof(num_out), ...). So
use that in test code as well please.

> +
> +       return 0;
> +}
> +
> +char _license[] SEC("license") = "GPL";
> --
> 2.30.1.766.gb4fecdf3b7-goog
>

Re: [PATCH bpf-next 2/5] bpf: Add a bpf_snprintf helper

[Florent Revest]

On Tue, Mar 16, 2021 at 2:25 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Wed, Mar 10, 2021 at 2:02 PM Florent Revest <revest@chromium.org> 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. But because ARG_PTR_TO_CONST_STR guarantees to point to a
> > NULL-terminated read-only map, we don't need a format string length arg.
> >
> > Because the format-string is known at verification time, we also move
> > most of the format string validation, currently done in formatting
> > helper calls, into the verifier logic. This makes debugging easier and
> > also slightly improves the runtime performance.
> >
> > Signed-off-by: Florent Revest <revest@chromium.org>
> > ---
> >  include/linux/bpf.h            |   4 +
> >  include/uapi/linux/bpf.h       |  28 +++++++
> >  kernel/bpf/verifier.c          | 137 +++++++++++++++++++++++++++++++++
> >  kernel/trace/bpf_trace.c       | 110 ++++++++++++++++++++++++++
> >  tools/include/uapi/linux/bpf.h |  28 +++++++
> >  5 files changed, 307 insertions(+)
> >
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index 7b5319d75b3e..d78175c9a887 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -1902,6 +1902,10 @@ extern const struct bpf_func_proto bpf_task_storage_get_proto;
> >  extern const struct bpf_func_proto bpf_task_storage_delete_proto;
> >  extern const struct bpf_func_proto bpf_for_each_map_elem_proto;
> >
> > +#define MAX_SNPRINTF_VARARGS           12
> > +#define MAX_SNPRINTF_MEMCPY            6
> > +#define MAX_SNPRINTF_STR_LEN           128
> > +
> >  const struct bpf_func_proto *bpf_tracing_func_proto(
> >         enum bpf_func_id func_id, const struct bpf_prog *prog);
> >
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 2d3036e292a9..3cbdc8ae00e7 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -4660,6 +4660,33 @@ union bpf_attr {
> >   *     Return
> >   *             The number of traversed map elements for success, **-EINVAL** for
> >   *             invalid **flags**.
> > + *
> > + * long bpf_snprintf(char *out, u32 out_size, const char *fmt, u64 *data, u32 data_len)
>
> bpf_snprintf_btf calls out and out_size str and str_size, let's be consistent?
>
> > + *     Description
> > + *             Outputs a string into the **out** buffer of size **out_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 printed string, including
> > + *             the trailing NUL character. If the return value is greater than
> > + *             **out_size**, **out** contains a truncated string, without a
> > + *             trailing NULL character.
>
> this deviates from the behavior in other BPF helpers dealing with
> strings. and it's extremely inconvenient for users to get
> non-zero-terminated string. I think we should always zero-terminate.
>
> > + *
> > + *             Or **-EBUSY** if the per-CPU memory copy buffer is busy.
> >   */
> >  #define __BPF_FUNC_MAPPER(FN)          \
> >         FN(unspec),                     \
> > @@ -4827,6 +4854,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/verifier.c b/kernel/bpf/verifier.c
> > index c99b2b67dc8d..3ab549df817b 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -5732,6 +5732,137 @@ static int check_reference_leak(struct bpf_verifier_env *env)
> >         return state->acquired_refs ? -EINVAL : 0;
> >  }
> >
> > +int check_bpf_snprintf_call(struct bpf_verifier_env *env,
> > +                           struct bpf_reg_state *regs)
> > +{
>
> can we please extra the printf format string parsing/checking logic
> and re-use them across all functions? We now have at least 4 variants
> of it, it's not great to say the least. I hope it's possible to
> generalize it in such a way that the same function will parse the
> string, and will record each expected argument and it's type, with
> whatever extra flags we need to. That should make the printing part
> simpler as well, as it will just follow "directions" from the parsing
> part? Devil is in the details, of course :) But it's worthwhile to try
> at least.

Eheh this is gonna be fun, I'll try it out and see if I can come up
with something ~decent. :)

Thanks for the thorough review! I agree with all your points and will
address them in v2.

> > +       struct bpf_reg_state *fmt_reg = &regs[BPF_REG_3];
> > +       struct bpf_reg_state *data_len_reg = &regs[BPF_REG_5];
> > +       struct bpf_map *fmt_map = fmt_reg->map_ptr;
> > +       int err, fmt_map_off, i, fmt_cnt = 0, memcpy_cnt = 0, num_args;
> > +       u64 fmt_addr;
> > +       char *fmt;
> > +
> > +       /* data must be an array of u64 so data_len must be a multiple of 8 */
> > +       if (data_len_reg->var_off.value & 7)
> > +               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;
> > +
>
> [...] not fun to read this part over and over :)
>
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> >  static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
> >                              int *insn_idx_p)
> >  {
> > @@ -5846,6 +5977,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 0d23755c2747..7b80759c10a9 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -1271,6 +1271,114 @@ const struct bpf_func_proto bpf_snprintf_btf_proto = {
> >         .arg5_type      = ARG_ANYTHING,
> >  };
> >
> > +struct bpf_snprintf_buf {
> > +       char buf[MAX_SNPRINTF_MEMCPY][MAX_SNPRINTF_STR_LEN];
> > +};
> > +static DEFINE_PER_CPU(struct bpf_snprintf_buf, bpf_snprintf_buf);
> > +static DEFINE_PER_CPU(int, bpf_snprintf_buf_used);
> > +
> > +BPF_CALL_5(bpf_snprintf, char *, out, u32, out_size, char *, fmt, u64 *, args,
> > +          u32, args_len)
> > +{
> > +       int err, i, buf_used, copy_size, fmt_cnt = 0, memcpy_cnt = 0;
> > +       u64 params[MAX_SNPRINTF_VARARGS];
> > +       struct bpf_snprintf_buf *bufs;
> > +
> > +       buf_used = this_cpu_inc_return(bpf_snprintf_buf_used);
> > +       if (WARN_ON_ONCE(buf_used > 1)) {
> > +               err = -EBUSY;
> > +               goto out;
> > +       }
> > +
> > +       bufs = this_cpu_ptr(&bpf_snprintf_buf);
> > +
> > +       /*
> > +        * The verifier has already done most of the heavy-work for us in
> > +        * check_bpf_snprintf_call. We know that fmt is well formatted and that
> > +        * args_len is valid. The only task left is to convert some of the
> > +        * arguments. For the %s and %pi* specifiers, we need to read buffers
> > +        * from a kernel address during the helper call.
> > +        */
> > +       for (i = 0; fmt[i] != '\0'; i++) {
>
> same function should hopefully be reused here
>
> > +       }
> > +
> > +       /* Maximumly we can have MAX_SNPRINTF_VARARGS parameters, just give
> > +        * all of them to snprintf().
> > +        */
> > +       err = snprintf(out, out_size, 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]) + 1;
> > +
> > +out:
> > +       this_cpu_dec(bpf_snprintf_buf_used);
> > +       return err;
> > +}
> > +
> > +static const struct bpf_func_proto bpf_snprintf_proto = {
> > +       .func           = bpf_snprintf,
> > +       .gpl_only       = true,
> > +       .ret_type       = RET_INTEGER,
> > +       .arg1_type      = ARG_PTR_TO_MEM,
> > +       .arg2_type      = ARG_CONST_SIZE,
>
> can we mark is CONST_SIZE_OR_ZERO and just do nothing on zero at
> runtime? I still have scars from having to deal (prove, actually) with
> ARG_CONST_SIZE (> 0) limitations in perf_event_output. No need to make
> anyone's life harder, if it's easy to just do something sensible on
> zero (i.e., do nothing, but emit desired amount of bytes).
>
> > +       .arg3_type      = ARG_PTR_TO_CONST_STR,
> > +       .arg4_type      = ARG_PTR_TO_MEM,
> > +       .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)
> >  {
> > @@ -1373,6 +1481,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;
>
> why just tracing? can't all BPF programs use this functionality?
>
> >         default:
> >                 return NULL;
> >         }
> > diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> > index 2d3036e292a9..3cbdc8ae00e7 100644
> > --- a/tools/include/uapi/linux/bpf.h
> > +++ b/tools/include/uapi/linux/bpf.h
> > @@ -4660,6 +4660,33 @@ union bpf_attr {
> >   *     Return
> >   *             The number of traversed map elements for success, **-EINVAL** for
> >   *             invalid **flags**.
> > + *
> > + * long bpf_snprintf(char *out, u32 out_size, const char *fmt, u64 *data, u32 data_len)
> > + *     Description
> > + *             Outputs a string into the **out** buffer of size **out_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 printed string, including
> > + *             the trailing NUL character. If the return value is greater than
> > + *             **out_size**, **out** contains a truncated string, without a
> > + *             trailing NULL character.
> > + *
> > + *             Or **-EBUSY** if the per-CPU memory copy buffer is busy.
> >   */
> >  #define __BPF_FUNC_MAPPER(FN)          \
> >         FN(unspec),                     \
> > @@ -4827,6 +4854,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.30.1.766.gb4fecdf3b7-goog
> >

Re: [PATCH bpf-next 3/5] libbpf: Initialize the bpf_seq_printf parameters array field by field

[Florent Revest]

On Tue, Mar 16, 2021 at 5:36 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
> On Wed, Mar 10, 2021 at 2:02 PM Florent Revest <revest@chromium.org> wrote:
> > +#define ___bpf_build_param0(narg, x)
> > +#define ___bpf_build_param1(narg, x) ___param[narg - 1] = x
> > +#define ___bpf_build_param2(narg, x, args...) ___param[narg - 2] = x; \
> > +                                             ___bpf_build_param1(narg, args)
> > +#define ___bpf_build_param3(narg, x, args...) ___param[narg - 3] = x; \
> > +                                             ___bpf_build_param2(narg, args)
> > +#define ___bpf_build_param4(narg, x, args...) ___param[narg - 4] = x; \
> > +                                             ___bpf_build_param3(narg, args)
> > +#define ___bpf_build_param5(narg, x, args...) ___param[narg - 5] = x; \
> > +                                             ___bpf_build_param4(narg, args)
> > +#define ___bpf_build_param6(narg, x, args...) ___param[narg - 6] = x; \
> > +                                             ___bpf_build_param5(narg, args)
> > +#define ___bpf_build_param7(narg, x, args...) ___param[narg - 7] = x; \
> > +                                             ___bpf_build_param6(narg, args)
> > +#define ___bpf_build_param8(narg, x, args...) ___param[narg - 8] = x; \
> > +                                             ___bpf_build_param7(narg, args)
> > +#define ___bpf_build_param9(narg, x, args...) ___param[narg - 9] = x; \
> > +                                             ___bpf_build_param8(narg, args)
> > +#define ___bpf_build_param10(narg, x, args...) ___param[narg - 10] = x; \
> > +                                              ___bpf_build_param9(narg, args)
> > +#define ___bpf_build_param11(narg, x, args...) ___param[narg - 11] = x; \
> > +                                              ___bpf_build_param10(narg, args)
> > +#define ___bpf_build_param12(narg, x, args...) ___param[narg - 12] = x; \
> > +                                              ___bpf_build_param11(narg, args)
>
> took me some time to get why the [narg - 12] :) it makes sense, but
> then I started wondering why not
>
> #define ___bpf_build_param12(narg, x, args...)
> ___bpf_build_param11(narg, args); ___param[11] = x
>
> ? seems more straightforward, no?

Unless I'm misunderstanding something, I don't think this would work.
The awkward "narg - 12" comes from the fact that these variadic macros
work by taking the first argument out of the variadic arguments (x
followed by args) and calling another macro with what's left (args).

So if you do __bpf_build_param(arg1, arg2) you will have
__bpf_build_param2() called with arg1 and __bpf_build_param1() called
with arg2. And if you do __bpf_build_param(arg1, arg2, arg3) you will
have __bpf_build_param3() called with arg1, __bpf_build_param2()
called with arg2, and __bpf_build_param1() called with arg3.
Basically, things are inverted, the position at which you need to
insert in ___param evolves in the opposite direction of the X after
___bpf_build_param which is the number of arguments left.

No matter in which order __bpf_build_paramX calls
__bpf_build_param(X-1) (before or after setting ___param[n]) you will
be unable to know just from the macro name at which cell in __param
you need to write the argument. (except for __bpf_build_param12 which
is an exception, because the max number of arg is 12, if this macro
gets called, then we know that narg=12 and we will always write at
__param[0])

That being said, I share your concern that this code is hard to read.
So instead of giving narg to each macro, I tried to give a pos
argument which indicates in which cell the macro should write. pos is
basically a counter that goes from 0 to narg as macros go from narg to
0.

#define ___bpf_fill0(array, pos, x)
#define ___bpf_fill1(array, pos, x) array[pos] = x
#define ___bpf_fill2(array, pos, x, args...) array[pos] = x;
___bpf_fill1(array, pos + 1, args)
#define ___bpf_fill3(array, pos, x, args...) array[pos] = x;
___bpf_fill2(array, pos + 1, args)
#define ___bpf_fill4(array, pos, x, args...) array[pos] = x;
___bpf_fill3(array, pos + 1, args)
#define ___bpf_fill5(array, pos, x, args...) array[pos] = x;
___bpf_fill4(array, pos + 1, args)
#define ___bpf_fill6(array, pos, x, args...) array[pos] = x;
___bpf_fill5(array, pos + 1, args)
#define ___bpf_fill7(array, pos, x, args...) array[pos] = x;
___bpf_fill6(array, pos + 1, args)
#define ___bpf_fill8(array, pos, x, args...) array[pos] = x;
___bpf_fill7(array, pos + 1, args)
#define ___bpf_fill9(array, pos, x, args...) array[pos] = x;
___bpf_fill8(array, pos + 1, args)
#define ___bpf_fill10(array, pos, x, args...) array[pos] = x;
___bpf_fill9(array, pos + 1, args)
#define ___bpf_fill11(array, pos, x, args...) array[pos] = x;
___bpf_fill10(array, pos + 1, args)
#define ___bpf_fill12(array, pos, x, args...) array[pos] = x;
___bpf_fill11(array, pos + 1, args)
#define ___bpf_fill(array, args...) \
___bpf_apply(___bpf_fill, ___bpf_narg(args))(array, 0, args)

I hope this makes things a bit clearer ? (I often joke that BPF is
written in preprocessor... :p)

Re: [PATCH bpf-next 3/5] libbpf: Initialize the bpf_seq_printf parameters array field by field

[Andrii Nakryiko]

On Tue, Mar 16, 2021 at 3:43 PM Florent Revest <revest@chromium.org> wrote:
>
> On Tue, Mar 16, 2021 at 5:36 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> > On Wed, Mar 10, 2021 at 2:02 PM Florent Revest <revest@chromium.org> wrote:
> > > +#define ___bpf_build_param0(narg, x)
> > > +#define ___bpf_build_param1(narg, x) ___param[narg - 1] = x
> > > +#define ___bpf_build_param2(narg, x, args...) ___param[narg - 2] = x; \
> > > +                                             ___bpf_build_param1(narg, args)
> > > +#define ___bpf_build_param3(narg, x, args...) ___param[narg - 3] = x; \
> > > +                                             ___bpf_build_param2(narg, args)
> > > +#define ___bpf_build_param4(narg, x, args...) ___param[narg - 4] = x; \
> > > +                                             ___bpf_build_param3(narg, args)
> > > +#define ___bpf_build_param5(narg, x, args...) ___param[narg - 5] = x; \
> > > +                                             ___bpf_build_param4(narg, args)
> > > +#define ___bpf_build_param6(narg, x, args...) ___param[narg - 6] = x; \
> > > +                                             ___bpf_build_param5(narg, args)
> > > +#define ___bpf_build_param7(narg, x, args...) ___param[narg - 7] = x; \
> > > +                                             ___bpf_build_param6(narg, args)
> > > +#define ___bpf_build_param8(narg, x, args...) ___param[narg - 8] = x; \
> > > +                                             ___bpf_build_param7(narg, args)
> > > +#define ___bpf_build_param9(narg, x, args...) ___param[narg - 9] = x; \
> > > +                                             ___bpf_build_param8(narg, args)
> > > +#define ___bpf_build_param10(narg, x, args...) ___param[narg - 10] = x; \
> > > +                                              ___bpf_build_param9(narg, args)
> > > +#define ___bpf_build_param11(narg, x, args...) ___param[narg - 11] = x; \
> > > +                                              ___bpf_build_param10(narg, args)
> > > +#define ___bpf_build_param12(narg, x, args...) ___param[narg - 12] = x; \
> > > +                                              ___bpf_build_param11(narg, args)
> >
> > took me some time to get why the [narg - 12] :) it makes sense, but
> > then I started wondering why not
> >
> > #define ___bpf_build_param12(narg, x, args...)
> > ___bpf_build_param11(narg, args); ___param[11] = x
> >
> > ? seems more straightforward, no?
>
> Unless I'm misunderstanding something, I don't think this would work.
> The awkward "narg - 12" comes from the fact that these variadic macros
> work by taking the first argument out of the variadic arguments (x
> followed by args) and calling another macro with what's left (args).

You are right, of course, silly me.

>
> So if you do __bpf_build_param(arg1, arg2) you will have
> __bpf_build_param2() called with arg1 and __bpf_build_param1() called
> with arg2. And if you do __bpf_build_param(arg1, arg2, arg3) you will
> have __bpf_build_param3() called with arg1, __bpf_build_param2()
> called with arg2, and __bpf_build_param1() called with arg3.
> Basically, things are inverted, the position at which you need to
> insert in ___param evolves in the opposite direction of the X after
> ___bpf_build_param which is the number of arguments left.
>
> No matter in which order __bpf_build_paramX calls
> __bpf_build_param(X-1) (before or after setting ___param[n]) you will
> be unable to know just from the macro name at which cell in __param
> you need to write the argument. (except for __bpf_build_param12 which
> is an exception, because the max number of arg is 12, if this macro
> gets called, then we know that narg=12 and we will always write at
> __param[0])
>
> That being said, I share your concern that this code is hard to read.
> So instead of giving narg to each macro, I tried to give a pos
> argument which indicates in which cell the macro should write. pos is
> basically a counter that goes from 0 to narg as macros go from narg to
> 0.
>
> #define ___bpf_fill0(array, pos, x)
> #define ___bpf_fill1(array, pos, x) array[pos] = x
> #define ___bpf_fill2(array, pos, x, args...) array[pos] = x;
> ___bpf_fill1(array, pos + 1, args)
> #define ___bpf_fill3(array, pos, x, args...) array[pos] = x;
> ___bpf_fill2(array, pos + 1, args)
> #define ___bpf_fill4(array, pos, x, args...) array[pos] = x;
> ___bpf_fill3(array, pos + 1, args)
> #define ___bpf_fill5(array, pos, x, args...) array[pos] = x;
> ___bpf_fill4(array, pos + 1, args)
> #define ___bpf_fill6(array, pos, x, args...) array[pos] = x;
> ___bpf_fill5(array, pos + 1, args)
> #define ___bpf_fill7(array, pos, x, args...) array[pos] = x;
> ___bpf_fill6(array, pos + 1, args)
> #define ___bpf_fill8(array, pos, x, args...) array[pos] = x;
> ___bpf_fill7(array, pos + 1, args)
> #define ___bpf_fill9(array, pos, x, args...) array[pos] = x;
> ___bpf_fill8(array, pos + 1, args)
> #define ___bpf_fill10(array, pos, x, args...) array[pos] = x;
> ___bpf_fill9(array, pos + 1, args)
> #define ___bpf_fill11(array, pos, x, args...) array[pos] = x;
> ___bpf_fill10(array, pos + 1, args)
> #define ___bpf_fill12(array, pos, x, args...) array[pos] = x;
> ___bpf_fill11(array, pos + 1, args)
> #define ___bpf_fill(array, args...) \
> ___bpf_apply(___bpf_fill, ___bpf_narg(args))(array, 0, args)

Yeah, this is still more straightforward, I think. Please use shorter
names to keep it a bit more succinct: arr and p seems clear enough.

>
> I hope this makes things a bit clearer ? (I often joke that BPF is
> written in preprocessor... :p)

Definitely true for BPF_CORE_READ macros :)

Re: [PATCH bpf-next 1/5] bpf: Add a ARG_PTR_TO_CONST_STR argument type

[Florent Revest]

On Tue, Mar 16, 2021 at 2:03 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
> On Wed, Mar 10, 2021 at 2:02 PM Florent Revest <revest@chromium.org> wrote:
> > +       } else if (arg_type == ARG_PTR_TO_CONST_STR) {
> > +               struct bpf_map *map = reg->map_ptr;
> > +               int map_off, i;
> > +               u64 map_addr;
> > +               char *map_ptr;
> > +
> > +               if (!map || !bpf_map_is_rdonly(map)) {
> > +                       verbose(env, "R%d does not point to a readonly map'\n", regno);
> > +                       return -EACCES;
> > +               }
> > +
> > +               if (!tnum_is_const(reg->var_off)) {
> > +                       verbose(env, "R%d is not a constant address'\n", regno);
> > +                       return -EACCES;
> > +               }
> > +
> > +               if (!map->ops->map_direct_value_addr) {
> > +                       verbose(env, "no direct value access support for this map type\n");
> > +                       return -EACCES;
> > +               }
> > +
> > +               err = check_helper_mem_access(env, regno,
> > +                                             map->value_size - reg->off,
> > +                                             false, meta);
>
> you expect reg to be PTR_TO_MAP_VALUE, so probably better to directly
> use check_map_access(). And double-check that register is of expected
> type. just the presence of ref->map_ptr might not be sufficient?

Sorry, just making sure I understand your comment correctly, are you
suggesting that we:
1- skip the check_map_access_type() currently done by
check_helper_mem_access()? or did you implicitly mean that we should
call it as well next to check_map_access() ?
2- enforce (reg->type == PTR_TO_MAP_VALUE) even if currently
guaranteed by compatible_reg_types, just to stay on the safe side ?

Re: [PATCH bpf-next 1/5] bpf: Add a ARG_PTR_TO_CONST_STR argument type

[Andrii Nakryiko]

On Tue, Mar 16, 2021 at 4:58 PM Florent Revest <revest@chromium.org> wrote:
>
> On Tue, Mar 16, 2021 at 2:03 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> > On Wed, Mar 10, 2021 at 2:02 PM Florent Revest <revest@chromium.org> wrote:
> > > +       } else if (arg_type == ARG_PTR_TO_CONST_STR) {
> > > +               struct bpf_map *map = reg->map_ptr;
> > > +               int map_off, i;
> > > +               u64 map_addr;
> > > +               char *map_ptr;
> > > +
> > > +               if (!map || !bpf_map_is_rdonly(map)) {
> > > +                       verbose(env, "R%d does not point to a readonly map'\n", regno);
> > > +                       return -EACCES;
> > > +               }
> > > +
> > > +               if (!tnum_is_const(reg->var_off)) {
> > > +                       verbose(env, "R%d is not a constant address'\n", regno);
> > > +                       return -EACCES;
> > > +               }
> > > +
> > > +               if (!map->ops->map_direct_value_addr) {
> > > +                       verbose(env, "no direct value access support for this map type\n");
> > > +                       return -EACCES;
> > > +               }
> > > +
> > > +               err = check_helper_mem_access(env, regno,
> > > +                                             map->value_size - reg->off,
> > > +                                             false, meta);
> >
> > you expect reg to be PTR_TO_MAP_VALUE, so probably better to directly
> > use check_map_access(). And double-check that register is of expected
> > type. just the presence of ref->map_ptr might not be sufficient?
>
> Sorry, just making sure I understand your comment correctly, are you
> suggesting that we:
> 1- skip the check_map_access_type() currently done by
> check_helper_mem_access()? or did you implicitly mean that we should
> call it as well next to check_map_access() ?

check_helper_mem_access() will call check_map_access() for
PTR_TO_MAP_VALUE and we expect only PTR_TO_MAP_VALUE, right? So why go
through check_helper_mem_access() if we know we need
check_map_access()? Less indirection, more explicit. So I meant
"replace check_helper_mem_access() with check_map_access()".

> 2- enforce (reg->type == PTR_TO_MAP_VALUE) even if currently
> guaranteed by compatible_reg_types, just to stay on the safe side ?

I can't follow compatible_reg_types :( If it does, then I guess it's
fine without this check.

Re: [PATCH bpf-next 1/5] bpf: Add a ARG_PTR_TO_CONST_STR argument type

[Florent Revest]

On Wed, Mar 17, 2021 at 1:35 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
> On Tue, Mar 16, 2021 at 4:58 PM Florent Revest <revest@chromium.org> wrote:
> > On Tue, Mar 16, 2021 at 2:03 AM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> > > On Wed, Mar 10, 2021 at 2:02 PM Florent Revest <revest@chromium.org> wrote:
> > > > +       } else if (arg_type == ARG_PTR_TO_CONST_STR) {
> > > > +               struct bpf_map *map = reg->map_ptr;
> > > > +               int map_off, i;
> > > > +               u64 map_addr;
> > > > +               char *map_ptr;
> > > > +
> > > > +               if (!map || !bpf_map_is_rdonly(map)) {
> > > > +                       verbose(env, "R%d does not point to a readonly map'\n", regno);
> > > > +                       return -EACCES;
> > > > +               }
> > > > +
> > > > +               if (!tnum_is_const(reg->var_off)) {
> > > > +                       verbose(env, "R%d is not a constant address'\n", regno);
> > > > +                       return -EACCES;
> > > > +               }
> > > > +
> > > > +               if (!map->ops->map_direct_value_addr) {
> > > > +                       verbose(env, "no direct value access support for this map type\n");
> > > > +                       return -EACCES;
> > > > +               }
> > > > +
> > > > +               err = check_helper_mem_access(env, regno,
> > > > +                                             map->value_size - reg->off,
> > > > +                                             false, meta);
> > >
> > > you expect reg to be PTR_TO_MAP_VALUE, so probably better to directly
> > > use check_map_access(). And double-check that register is of expected
> > > type. just the presence of ref->map_ptr might not be sufficient?
> >
> > Sorry, just making sure I understand your comment correctly, are you
> > suggesting that we:
> > 1- skip the check_map_access_type() currently done by
> > check_helper_mem_access()? or did you implicitly mean that we should
> > call it as well next to check_map_access() ?
>
> check_helper_mem_access() will call check_map_access() for
> PTR_TO_MAP_VALUE and we expect only PTR_TO_MAP_VALUE, right? So why go
> through check_helper_mem_access() if we know we need
> check_map_access()? Less indirection, more explicit. So I meant
> "replace check_helper_mem_access() with check_map_access()".

Mhh I suspect there's still a misunderstanding, these function names
are really confusing ahah.
What about check_map_access*_type*. which is also called by
check_helper_mem_access (before check_map_access):
https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/tree/kernel/bpf/verifier.c#n4329

Your message sounds like we should skip it so I was asking if that's
what you also implicitly meant or if you missed it?

> > 2- enforce (reg->type == PTR_TO_MAP_VALUE) even if currently
> > guaranteed by compatible_reg_types, just to stay on the safe side ?
>
> I can't follow compatible_reg_types :( If it does, then I guess it's
> fine without this check.

It's alright, I can keep an extra check just for safety. :)

Re: [PATCH bpf-next 1/5] bpf: Add a ARG_PTR_TO_CONST_STR argument type

[Andrii Nakryiko]

On Tue, Mar 16, 2021 at 5:46 PM Florent Revest <revest@chromium.org> wrote:
>
> On Wed, Mar 17, 2021 at 1:35 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> > On Tue, Mar 16, 2021 at 4:58 PM Florent Revest <revest@chromium.org> wrote:
> > > On Tue, Mar 16, 2021 at 2:03 AM Andrii Nakryiko
> > > <andrii.nakryiko@gmail.com> wrote:
> > > > On Wed, Mar 10, 2021 at 2:02 PM Florent Revest <revest@chromium.org> wrote:
> > > > > +       } else if (arg_type == ARG_PTR_TO_CONST_STR) {
> > > > > +               struct bpf_map *map = reg->map_ptr;
> > > > > +               int map_off, i;
> > > > > +               u64 map_addr;
> > > > > +               char *map_ptr;
> > > > > +
> > > > > +               if (!map || !bpf_map_is_rdonly(map)) {
> > > > > +                       verbose(env, "R%d does not point to a readonly map'\n", regno);
> > > > > +                       return -EACCES;
> > > > > +               }
> > > > > +
> > > > > +               if (!tnum_is_const(reg->var_off)) {
> > > > > +                       verbose(env, "R%d is not a constant address'\n", regno);
> > > > > +                       return -EACCES;
> > > > > +               }
> > > > > +
> > > > > +               if (!map->ops->map_direct_value_addr) {
> > > > > +                       verbose(env, "no direct value access support for this map type\n");
> > > > > +                       return -EACCES;
> > > > > +               }
> > > > > +
> > > > > +               err = check_helper_mem_access(env, regno,
> > > > > +                                             map->value_size - reg->off,
> > > > > +                                             false, meta);
> > > >
> > > > you expect reg to be PTR_TO_MAP_VALUE, so probably better to directly
> > > > use check_map_access(). And double-check that register is of expected
> > > > type. just the presence of ref->map_ptr might not be sufficient?
> > >
> > > Sorry, just making sure I understand your comment correctly, are you
> > > suggesting that we:
> > > 1- skip the check_map_access_type() currently done by
> > > check_helper_mem_access()? or did you implicitly mean that we should
> > > call it as well next to check_map_access() ?
> >
> > check_helper_mem_access() will call check_map_access() for
> > PTR_TO_MAP_VALUE and we expect only PTR_TO_MAP_VALUE, right? So why go
> > through check_helper_mem_access() if we know we need
> > check_map_access()? Less indirection, more explicit. So I meant
> > "replace check_helper_mem_access() with check_map_access()".
>
> Mhh I suspect there's still a misunderstanding, these function names
> are really confusing ahah.
> What about check_map_access*_type*. which is also called by
> check_helper_mem_access (before check_map_access):
> https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/tree/kernel/bpf/verifier.c#n4329
>
> Your message sounds like we should skip it so I was asking if that's
> what you also implicitly meant or if you missed it?

ah, you meant READ/WRITE access? ok, let's keep
check_helper_mem_access() then, never mind me

>
> > > 2- enforce (reg->type == PTR_TO_MAP_VALUE) even if currently
> > > guaranteed by compatible_reg_types, just to stay on the safe side ?
> >
> > I can't follow compatible_reg_types :( If it does, then I guess it's
> > fine without this check.
>
> It's alright, I can keep an extra check just for safety. :)

Re: [PATCH bpf-next 1/5] bpf: Add a ARG_PTR_TO_CONST_STR argument type

[Florent Revest]

On Wed, Mar 17, 2021 at 2:02 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
> On Tue, Mar 16, 2021 at 5:46 PM Florent Revest <revest@chromium.org> wrote:
> > On Wed, Mar 17, 2021 at 1:35 AM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> > > On Tue, Mar 16, 2021 at 4:58 PM Florent Revest <revest@chromium.org> wrote:
> > > > On Tue, Mar 16, 2021 at 2:03 AM Andrii Nakryiko
> > > > <andrii.nakryiko@gmail.com> wrote:
> > > > > On Wed, Mar 10, 2021 at 2:02 PM Florent Revest <revest@chromium.org> wrote:
> > > > > > +       } else if (arg_type == ARG_PTR_TO_CONST_STR) {
> > > > > > +               struct bpf_map *map = reg->map_ptr;
> > > > > > +               int map_off, i;
> > > > > > +               u64 map_addr;
> > > > > > +               char *map_ptr;
> > > > > > +
> > > > > > +               if (!map || !bpf_map_is_rdonly(map)) {
> > > > > > +                       verbose(env, "R%d does not point to a readonly map'\n", regno);
> > > > > > +                       return -EACCES;
> > > > > > +               }
> > > > > > +
> > > > > > +               if (!tnum_is_const(reg->var_off)) {
> > > > > > +                       verbose(env, "R%d is not a constant address'\n", regno);
> > > > > > +                       return -EACCES;
> > > > > > +               }
> > > > > > +
> > > > > > +               if (!map->ops->map_direct_value_addr) {
> > > > > > +                       verbose(env, "no direct value access support for this map type\n");
> > > > > > +                       return -EACCES;
> > > > > > +               }
> > > > > > +
> > > > > > +               err = check_helper_mem_access(env, regno,
> > > > > > +                                             map->value_size - reg->off,
> > > > > > +                                             false, meta);
> > > > >
> > > > > you expect reg to be PTR_TO_MAP_VALUE, so probably better to directly
> > > > > use check_map_access(). And double-check that register is of expected
> > > > > type. just the presence of ref->map_ptr might not be sufficient?
> > > >
> > > > Sorry, just making sure I understand your comment correctly, are you
> > > > suggesting that we:
> > > > 1- skip the check_map_access_type() currently done by
> > > > check_helper_mem_access()? or did you implicitly mean that we should
> > > > call it as well next to check_map_access() ?
> > >
> > > check_helper_mem_access() will call check_map_access() for
> > > PTR_TO_MAP_VALUE and we expect only PTR_TO_MAP_VALUE, right? So why go
> > > through check_helper_mem_access() if we know we need
> > > check_map_access()? Less indirection, more explicit. So I meant
> > > "replace check_helper_mem_access() with check_map_access()".
> >
> > Mhh I suspect there's still a misunderstanding, these function names
> > are really confusing ahah.
> > What about check_map_access*_type*. which is also called by
> > check_helper_mem_access (before check_map_access):
> > https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/tree/kernel/bpf/verifier.c#n4329
> >
> > Your message sounds like we should skip it so I was asking if that's
> > what you also implicitly meant or if you missed it?
>
> ah, you meant READ/WRITE access? ok, let's keep
> check_helper_mem_access() then, never mind me

Ah cool, then we are on the same page :)

Re: [PATCH bpf-next 2/5] bpf: Add a bpf_snprintf helper

[Alexei Starovoitov]

On Wed, Mar 10, 2021 at 11:02:08PM +0100, Florent Revest wrote:
>  
> +struct bpf_snprintf_buf {
> +	char buf[MAX_SNPRINTF_MEMCPY][MAX_SNPRINTF_STR_LEN];
> +};
> +static DEFINE_PER_CPU(struct bpf_snprintf_buf, bpf_snprintf_buf);
> +static DEFINE_PER_CPU(int, bpf_snprintf_buf_used);
> +
> +BPF_CALL_5(bpf_snprintf, char *, out, u32, out_size, char *, fmt, u64 *, args,
> +	   u32, args_len)
> +{
> +	int err, i, buf_used, copy_size, fmt_cnt = 0, memcpy_cnt = 0;
> +	u64 params[MAX_SNPRINTF_VARARGS];
> +	struct bpf_snprintf_buf *bufs;
> +
> +	buf_used = this_cpu_inc_return(bpf_snprintf_buf_used);
> +	if (WARN_ON_ONCE(buf_used > 1)) {

this can trigger only if the helper itself gets preempted and
another bpf prog will run on the same cpu and will call into this helper
again, right?
If so, how about adding preempt_disable here to avoid this case?
It won't prevent the case where kprobe is inside snprintf core,
so the counter is still needed, but it wouldn't trigger by accident.
Also since bufs are not used always, how about grabbing the
buffers only when %p or %s are seen in fmt?
After snprintf() is done it would conditionally do:
if (bufs_were_used) {
   this_cpu_dec(bpf_snprintf_buf_used);
   preempt_enable();
}
This way simple bpf_snprintf won't ever hit EBUSY.

> +		err = -EBUSY;
> +		goto out;
> +	}
> +
> +	bufs = this_cpu_ptr(&bpf_snprintf_buf);
> +
> +	/*
> +	 * The verifier has already done most of the heavy-work for us in
> +	 * check_bpf_snprintf_call. We know that fmt is well formatted and that
> +	 * args_len is valid. The only task left is to convert some of the
> +	 * arguments. For the %s and %pi* specifiers, we need to read buffers
> +	 * from a kernel address during the helper call.
> +	 */
> +	for (i = 0; fmt[i] != '\0'; i++) {
> +		if (fmt[i] != '%')
> +			continue;
> +
> +		if (fmt[i + 1] == '%') {
> +			i++;
> +			continue;
> +		}
> +
> +		/* 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 = (void *)(long)args[fmt_cnt];
> +
> +			err = strncpy_from_kernel_nofault(bufs->buf[memcpy_cnt],
> +							  unsafe_ptr,
> +							  MAX_SNPRINTF_STR_LEN);
> +			if (err < 0)
> +				bufs->buf[memcpy_cnt][0] = '\0';
> +			params[fmt_cnt] = (u64)(long)bufs->buf[memcpy_cnt];

how about:
char buf[512]; instead?
instead of memcpy_cnt++ remember how many bytes of the buf were used and
copy next arg after that.
The scratch space would be used more efficiently.
The helper would potentially return ENOSPC if the first string printed via %s
consumed most of the 512 space and the second string doesn't fit.
But the verifier-time if (memcpy_cnt >= MAX_SNPRINTF_MEMCPY) can be removed.
Ten small %s will work fine.

We can allocate a page per-cpu when this helper is used by prog and free
that page when all progs with bpf_snprintf are unloaded.
But extra complexity is probably not worth it. I would start with 512 per-cpu.
It's going to be enough for most users.

Overall looks great. Cannot wait for v2 :)

Re: [PATCH bpf-next 2/5] bpf: Add a bpf_snprintf helper

[Florent Revest]

On Tue, Mar 23, 2021 at 4:21 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Wed, Mar 10, 2021 at 11:02:08PM +0100, Florent Revest wrote:
> >
> > +struct bpf_snprintf_buf {
> > +     char buf[MAX_SNPRINTF_MEMCPY][MAX_SNPRINTF_STR_LEN];
> > +};
> > +static DEFINE_PER_CPU(struct bpf_snprintf_buf, bpf_snprintf_buf);
> > +static DEFINE_PER_CPU(int, bpf_snprintf_buf_used);
> > +
> > +BPF_CALL_5(bpf_snprintf, char *, out, u32, out_size, char *, fmt, u64 *, args,
> > +        u32, args_len)
> > +{
> > +     int err, i, buf_used, copy_size, fmt_cnt = 0, memcpy_cnt = 0;
> > +     u64 params[MAX_SNPRINTF_VARARGS];
> > +     struct bpf_snprintf_buf *bufs;
> > +
> > +     buf_used = this_cpu_inc_return(bpf_snprintf_buf_used);
> > +     if (WARN_ON_ONCE(buf_used > 1)) {
>
> this can trigger only if the helper itself gets preempted and
> another bpf prog will run on the same cpu and will call into this helper
> again, right?
> If so, how about adding preempt_disable here to avoid this case?

Ah, neat, that sounds like a good idea indeed. This was really just
cargo-culted from bpf_seq_printf but as part of my grand unification
attempt for the various printf-like helpers, I can try to make it use
preempt_disable as well yes.

> It won't prevent the case where kprobe is inside snprintf core,
> so the counter is still needed, but it wouldn't trigger by accident.

Good point, I will keep it around then.

> Also since bufs are not used always, how about grabbing the
> buffers only when %p or %s are seen in fmt?
> After snprintf() is done it would conditionally do:
> if (bufs_were_used) {
>    this_cpu_dec(bpf_snprintf_buf_used);
>    preempt_enable();
> }
> This way simple bpf_snprintf won't ever hit EBUSY.

Absolutely, it would be nice. :)

> > +             err = -EBUSY;
> > +             goto out;
> > +     }
> > +
> > +     bufs = this_cpu_ptr(&bpf_snprintf_buf);
> > +
> > +     /*
> > +      * The verifier has already done most of the heavy-work for us in
> > +      * check_bpf_snprintf_call. We know that fmt is well formatted and that
> > +      * args_len is valid. The only task left is to convert some of the
> > +      * arguments. For the %s and %pi* specifiers, we need to read buffers
> > +      * from a kernel address during the helper call.
> > +      */
> > +     for (i = 0; fmt[i] != '\0'; i++) {
> > +             if (fmt[i] != '%')
> > +                     continue;
> > +
> > +             if (fmt[i + 1] == '%') {
> > +                     i++;
> > +                     continue;
> > +             }
> > +
> > +             /* 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 = (void *)(long)args[fmt_cnt];
> > +
> > +                     err = strncpy_from_kernel_nofault(bufs->buf[memcpy_cnt],
> > +                                                       unsafe_ptr,
> > +                                                       MAX_SNPRINTF_STR_LEN);
> > +                     if (err < 0)
> > +                             bufs->buf[memcpy_cnt][0] = '\0';
> > +                     params[fmt_cnt] = (u64)(long)bufs->buf[memcpy_cnt];
>
> how about:
> char buf[512]; instead?
> instead of memcpy_cnt++ remember how many bytes of the buf were used and
> copy next arg after that.
> The scratch space would be used more efficiently.
> The helper would potentially return ENOSPC if the first string printed via %s
> consumed most of the 512 space and the second string doesn't fit.
> But the verifier-time if (memcpy_cnt >= MAX_SNPRINTF_MEMCPY) can be removed.
> Ten small %s will work fine.

Cool! That is also a good idea :)

> We can allocate a page per-cpu when this helper is used by prog and free
> that page when all progs with bpf_snprintf are unloaded.
> But extra complexity is probably not worth it. I would start with 512 per-cpu.
> It's going to be enough for most users.

Yes, let's maybe keep that for later. I think there is already enough
complexity going into the printf-like helpers unification patch.

> Overall looks great. Cannot wait for v2 :)

Ahah wait until you see that patch! :D