bpf: Remove struct bpf_verifier_env argument from print_bpf_insn

Change bpftool to skip the removed struct bpf_verifier_env argument in print_bpf_insn. It was passed as NULL anyway.

No functional change intended.

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/bpf/bpftool/prog.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

tools/bpf/bpftool/prog.c +3 -3

index e549e329be82..108001d974ee 100644
@@ -489,7 +489,7 @@ static struct kernel_sym *kernel_syms_search(struct dump_data *dd,
 		       sizeof(*dd->sym_mapping), kernel_syms_cmp) : NULL;
 }
 
-static void print_insn(struct bpf_verifier_env *env, const char *fmt, ...)
+static void print_insn(void *private_data, const char *fmt, ...)
 {
 	va_list args;
 

@@ -576,7 +576,7 @@ static void dump_xlated_plain(struct dump_data *dd, void *buf,
 		double_insn = insn[i].code == (BPF_LD | BPF_IMM | BPF_DW);
 
 		printf("% 4d: ", i);
-		print_bpf_insn(&cbs, NULL, insn + i, true);
+		print_bpf_insn(&cbs, insn + i, true);
 
 		if (opcodes) {
 			printf("       ");

@@ -590,7 +590,7 @@ static void dump_xlated_plain(struct dump_data *dd, void *buf,
 	}
 }
 
-static void print_insn_json(struct bpf_verifier_env *env, const char *fmt, ...)
+static void print_insn_json(void *private_data, const char *fmt, ...)
 {
 	unsigned int l = strlen(fmt);
 	char chomped_fmt[l];

2018-03-21 16:02 UTC+0100 ~ Jiri Olsa <[email protected]>

Quoted 48 lines

Change bpftool to skip the removed struct bpf_verifier_env argument in print_bpf_insn. It was passed as NULL anyway.

No functional change intended.

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/bpf/bpftool/prog.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

tools/bpf/bpftool/prog.c +4 -4

index e549e329be82..108001d974ee 100644
@@ -489,7 +489,7 @@ static struct kernel_sym *kernel_syms_search(struct dump_data *dd,
 		       sizeof(*dd->sym_mapping), kernel_syms_cmp) : NULL;
 }
 
-static void print_insn(struct bpf_verifier_env *env, const char *fmt, ...)
+static void print_insn(void *private_data, const char *fmt, ...)
 {
 	va_list args;
 

@@ -576,7 +576,7 @@ static void dump_xlated_plain(struct dump_data *dd, void *buf,
 		double_insn = insn[i].code == (BPF_LD | BPF_IMM | BPF_DW);
 
 		printf("% 4d: ", i);
-		print_bpf_insn(&cbs, NULL, insn + i, true);
+		print_bpf_insn(&cbs, insn + i, true);
 
 		if (opcodes) {
 			printf("       ");

@@ -590,7 +590,7 @@ static void dump_xlated_plain(struct dump_data *dd, void *buf,
 	}
 }
 
-static void print_insn_json(struct bpf_verifier_env *env, const char *fmt, ...)
+static void print_insn_json(void *private_data, const char *fmt, ...)
 {
 	unsigned int l = strlen(fmt);
 	char chomped_fmt[l];

@@ -628,7 +628,7 @@ static void dump_xlated_json(struct dump_data *dd, void *buf,
 
 		jsonw_start_object(json_wtr);
 		jsonw_name(json_wtr, "disasm");
-		print_bpf_insn(&cbs, NULL, insn + i, true);
+		print_bpf_insn(&cbs, insn + i, true);
 

tools/bpf/bpftool/prog.c static void dump_xlated_json(struct dump_data *dd, void *buf, · line 634

Quoted 3 lines

tools/bpf/bpftool/prog.c +0 -0

 		if (opcodes) {
 			jsonw_name(json_wtr, "opcodes");

Hi Jiri, this code has changed in the tree. It was moved to tools/bpf/bpftool/xlated_dumper.c, and there is now a third function to
update: print_insn_for_graph(). Could you please rebase the patch?

Quoted 49 lines · Quentin Monnet

2018-03-21 16:02 UTC+0100 ~ Jiri Olsa <[email protected]>

Quoted 48 lines

Change bpftool to skip the removed struct bpf_verifier_env argument in print_bpf_insn. It was passed as NULL anyway.

No functional change intended.

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/bpf/bpftool/prog.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

tools/bpf/bpftool/prog.c +4 -4

index e549e329be82..108001d974ee 100644
@@ -489,7 +489,7 @@ static struct kernel_sym *kernel_syms_search(struct dump_data *dd,
 		       sizeof(*dd->sym_mapping), kernel_syms_cmp) : NULL;
 }
 
-static void print_insn(struct bpf_verifier_env *env, const char *fmt, ...)
+static void print_insn(void *private_data, const char *fmt, ...)
 {
 	va_list args;
 

@@ -576,7 +576,7 @@ static void dump_xlated_plain(struct dump_data *dd, void *buf,
 		double_insn = insn[i].code == (BPF_LD | BPF_IMM | BPF_DW);
 
 		printf("% 4d: ", i);
-		print_bpf_insn(&cbs, NULL, insn + i, true);
+		print_bpf_insn(&cbs, insn + i, true);
 
 		if (opcodes) {
 			printf("       ");

@@ -590,7 +590,7 @@ static void dump_xlated_plain(struct dump_data *dd, void *buf,
 	}
 }
 
-static void print_insn_json(struct bpf_verifier_env *env, const char *fmt, ...)
+static void print_insn_json(void *private_data, const char *fmt, ...)
 {
 	unsigned int l = strlen(fmt);
 	char chomped_fmt[l];

@@ -628,7 +628,7 @@ static void dump_xlated_json(struct dump_data *dd, void *buf,
 
 		jsonw_start_object(json_wtr);
 		jsonw_name(json_wtr, "disasm");
-		print_bpf_insn(&cbs, NULL, insn + i, true);
+		print_bpf_insn(&cbs, insn + i, true);
 

Quoted 7 lines

tools/bpf/bpftool/prog.c static void dump_xlated_json(struct dump_data *dd, void *buf, · line 634

Quoted 3 lines

tools/bpf/bpftool/prog.c +0 -0

 		if (opcodes) {
 			jsonw_name(json_wtr, "opcodes");

Hi Jiri, this code has changed in the tree. It was moved to tools/bpf/bpftool/xlated_dumper.c, and there is now a third function to
update: print_insn_for_graph(). Could you please rebase the patch?

sure.. I was over perf tree, I'll check on bpf tree

Quoted 5 lines

Quoted 3 lines

Hi Jiri, this code has changed in the tree. It was moved to tools/bpf/bpftool/xlated_dumper.c, and there is now a third function to
update: print_insn_for_graph(). Could you please rebase the patch?

sure.. I was over perf tree, I'll check on bpf tree

Just to be sure, it should be bpf-next. bpf is for fixes only.

SNIP

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

Hi Jiri, this code has changed in the tree. It was moved to tools/bpf/bpftool/xlated_dumper.c, and there is now a third function to
update: print_insn_for_graph(). Could you please rebase the patch?

sure.. I was over perf tree, I'll check on bpf tree

Just to be sure, it should be bpf-next. bpf is for fixes only.

v2 attached

thanks,
jirka

---
Change bpftool to skip the removed struct bpf_verifier_env argument in print_bpf_insn. It was passed as NULL anyway.

No functional change intended.

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/bpf/bpftool/xlated_dumper.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

We use print_bpf_insn in user space (bpftool and soon perf), so it'd be nice to keep it generic and strip it off the kernel struct bpf_verifier_env argument.

This argument can be safely removed, because its users can use the struct bpf_insn_cbs::private_data to pass it.

Signed-off-by Jiri Olsa <[email protected]>

---
 kernel/bpf/disasm.c   | 52 +++++++++++++++++++++++++--------------------------
 kernel/bpf/disasm.h   |  5 +----
 kernel/bpf/verifier.c |  6 +++---
 3 files changed, 30 insertions(+), 33 deletions(-)

kernel/bpf/verifier.c +2 -1

 static bool type_is_pkt_pointer(enum bpf_reg_type type)

@@ -4601,10 +4600,11 @@ static int do_check(struct bpf_verifier_env *env)
 		if (env->log.level) {
 			const struct bpf_insn_cbs cbs = {
 				.cb_print	= verbose,
+				.private_data	= env,
 			};
 
 			verbose(env, "%d: ", insn_idx);
-			print_bpf_insn(&cbs, env, insn, env->allow_ptr_leaks);
+			print_bpf_insn(&cbs, insn, env->allow_ptr_leaks);
 		}
 
 		if (bpf_prog_is_dev_bound(env->prog->aux)) {

2018-03-21 16:02 UTC+0100 ~ Jiri Olsa <[email protected]>

Quoted 14 lines

We use print_bpf_insn in user space (bpftool and soon perf), so it'd be nice to keep it generic and strip it off the kernel struct bpf_verifier_env argument.

This argument can be safely removed, because its users can use the struct bpf_insn_cbs::private_data to pass it.

Signed-off-by Jiri Olsa <[email protected]>

---
 kernel/bpf/disasm.c   | 52 +++++++++++++++++++++++++--------------------------
 kernel/bpf/disasm.h   |  5 +----
 kernel/bpf/verifier.c |  6 +++---
 3 files changed, 30 insertions(+), 33 deletions(-)

[...]

Quoted 9 lines

kernel/bpf/verifier.c +0 -1

index c6eff108aa99..9f27d3fa7259 100644
@@ -202,8 +202,7 @@ EXPORT_SYMBOL_GPL(bpf_verifier_log_write);
  * generic for symbol export. The function was renamed, but not the calls in
  * the verifier to avoid complicating backports. Hence the alias below.
  */
-static __printf(2, 3) void verbose(struct bpf_verifier_env *env,

kernel/bpf/verifier.c EXPORT_SYMBOL_GPL(bpf_verifier_log_write); · line 206

Quoted 3 lines

kernel/bpf/verifier.c +1 -1

-				   const char *fmt, ...)
+static __printf(2, 3) void verbose(void *private_data, const char *fmt, ...)
 	__attribute__((alias("bpf_verifier_log_write")));

Just as a note, verbose() will be aliased to a function whose prototype differs (bpf_verifier_log_write() still expects a struct bpf_verifier_env as its first argument). I am not so familiar with function aliases, could this change be a concern?

Quoted 26 lines · Quentin Monnet

2018-03-21 16:02 UTC+0100 ~ Jiri Olsa <[email protected]>

Quoted 13 lines

We use print_bpf_insn in user space (bpftool and soon perf), so it'd be nice to keep it generic and strip it off the kernel struct bpf_verifier_env argument.

This argument can be safely removed, because its users can use the struct bpf_insn_cbs::private_data to pass it.

Signed-off-by Jiri Olsa <[email protected]>

---
 kernel/bpf/disasm.c   | 52 +++++++++++++++++++++++++--------------------------
 kernel/bpf/disasm.h   |  5 +----
 kernel/bpf/verifier.c |  6 +++---
 3 files changed, 30 insertions(+), 33 deletions(-)

[...]

Quoted 9 lines

kernel/bpf/verifier.c +0 -1

index c6eff108aa99..9f27d3fa7259 100644
@@ -202,8 +202,7 @@ EXPORT_SYMBOL_GPL(bpf_verifier_log_write);
  * generic for symbol export. The function was renamed, but not the calls in
  * the verifier to avoid complicating backports. Hence the alias below.
  */
-static __printf(2, 3) void verbose(struct bpf_verifier_env *env,

Quoted 8 lines

kernel/bpf/verifier.c EXPORT_SYMBOL_GPL(bpf_verifier_log_write); · line 206

Quoted 3 lines

kernel/bpf/verifier.c +1 -1

-				   const char *fmt, ...)
+static __printf(2, 3) void verbose(void *private_data, const char *fmt, ...)
 	__attribute__((alias("bpf_verifier_log_write")));

Just as a note, verbose() will be aliased to a function whose prototype differs (bpf_verifier_log_write() still expects a struct bpf_verifier_env as its first argument). I am not so familiar with function aliases, could this change be a concern?

yea, but as it was pointer for pointer switch I did not see any problem with that.. I'll check more

Quoted 36 lines · Jiri Olsa

Quoted 34 lines · Quentin Monnet

2018-03-21 16:02 UTC+0100 ~ Jiri Olsa <[email protected]>

Quoted 13 lines

We use print_bpf_insn in user space (bpftool and soon perf), so it'd be nice to keep it generic and strip it off the kernel struct bpf_verifier_env argument.

This argument can be safely removed, because its users can use the struct bpf_insn_cbs::private_data to pass it.

Signed-off-by Jiri Olsa <[email protected]>

---
 kernel/bpf/disasm.c   | 52 +++++++++++++++++++++++++--------------------------
 kernel/bpf/disasm.h   |  5 +----
 kernel/bpf/verifier.c |  6 +++---
 3 files changed, 30 insertions(+), 33 deletions(-)

[...]

kernel/bpf/verifier.c EXPORT_SYMBOL_GPL(bpf_verifier_log_write); · line 206

Quoted 12 lines

kernel/bpf/verifier.c +1 -2

index c6eff108aa99..9f27d3fa7259 100644
@@ -202,8 +202,7 @@ EXPORT_SYMBOL_GPL(bpf_verifier_log_write);
  * generic for symbol export. The function was renamed, but not the calls in
  * the verifier to avoid complicating backports. Hence the alias below.
  */
-static __printf(2, 3) void verbose(struct bpf_verifier_env *env,
-				   const char *fmt, ...)
+static __printf(2, 3) void verbose(void *private_data, const char *fmt, ...)
 	__attribute__((alias("bpf_verifier_log_write")));

Just as a note, verbose() will be aliased to a function whose prototype differs (bpf_verifier_log_write() still expects a struct bpf_verifier_env as its first argument). I am not so familiar with function aliases, could this change be a concern?

Quoted 2 lines

yea, but as it was pointer for pointer switch I did not see any problem with that.. I'll check more

Ok, holding off for now until we have clarification. Other option could also be to make it void *private_data everywhere and for the kernel writer then do struct bpf_verifier_env *env = private_data.

Quoted 40 lines · Jiri Olsa

Quoted 38 lines · Quentin Monnet

Quoted 26 lines

2018-03-21 16:02 UTC+0100 ~ Jiri Olsa <[email protected]>

Quoted 13 lines

We use print_bpf_insn in user space (bpftool and soon perf), so it'd be nice to keep it generic and strip it off the kernel struct bpf_verifier_env argument.

This argument can be safely removed, because its users can use the struct bpf_insn_cbs::private_data to pass it.

Signed-off-by Jiri Olsa <[email protected]>

---
 kernel/bpf/disasm.c   | 52 +++++++++++++++++++++++++--------------------------
 kernel/bpf/disasm.h   |  5 +----
 kernel/bpf/verifier.c |  6 +++---
 3 files changed, 30 insertions(+), 33 deletions(-)

[...]

Quoted 9 lines

kernel/bpf/verifier.c +0 -1

index c6eff108aa99..9f27d3fa7259 100644
@@ -202,8 +202,7 @@ EXPORT_SYMBOL_GPL(bpf_verifier_log_write);
  * generic for symbol export. The function was renamed, but not the calls in
  * the verifier to avoid complicating backports. Hence the alias below.
  */
-static __printf(2, 3) void verbose(struct bpf_verifier_env *env,

Quoted 8 lines

kernel/bpf/verifier.c EXPORT_SYMBOL_GPL(bpf_verifier_log_write); · line 206

Quoted 3 lines

kernel/bpf/verifier.c +1 -1

-				   const char *fmt, ...)
+static __printf(2, 3) void verbose(void *private_data, const char *fmt, ...)
 	__attribute__((alias("bpf_verifier_log_write")));

Just as a note, verbose() will be aliased to a function whose prototype differs (bpf_verifier_log_write() still expects a struct bpf_verifier_env as its first argument). I am not so familiar with function aliases, could this change be a concern?

yea, but as it was pointer for pointer switch I did not see any problem with that.. I'll check more

Quoted 3 lines

Ok, holding off for now until we have clarification. Other option could also be to make it void *private_data everywhere and for the kernel writer then do struct bpf_verifier_env *env = private_data.

can't find much info about the alias behaviour for this case.. so how about having separate function for the print_cb like below.. I still need to test it

thanks,
jirka

---
 kernel/bpf/disasm.c   | 52 +++++++++++++++++++++++++--------------------------
 kernel/bpf/disasm.h   |  5 +----
 kernel/bpf/verifier.c | 41 +++++++++++++++++++++++++++++-----------
 3 files changed, 57 insertions(+), 41 deletions(-)

kernel/bpf/verifier.c +10 -9

index e9f7c20691c1..69bf7590877c 100644
@@ -168,23 +168,16 @@ struct bpf_call_arg_meta {
 
 static DEFINE_MUTEX(bpf_verifier_lock);
 
-/* log_level controls verbosity level of eBPF verifier.
- * bpf_verifier_log_write() is used to dump the verification trace to the log,
- * so the user can figure out what's wrong with the program
- */
-__printf(2, 3) void bpf_verifier_log_write(struct bpf_verifier_env *env,
-					   const char *fmt, ...)
+static void log_write(struct bpf_verifier_env *env, const char *fmt,
+		      va_list args)
 {
 	struct bpf_verifer_log *log = &env->log;
 	unsigned int n;
-	va_list args;
 
 	if (!log->level || !log->ubuf || bpf_verifier_log_full(log))
 		return;
 
-	va_start(args, fmt);
 	n = vscnprintf(log->kbuf, BPF_VERIFIER_TMP_LOG_SIZE, fmt, args);
-	va_end(args);
 
 	WARN_ONCE(n >= BPF_VERIFIER_TMP_LOG_SIZE - 1,
 		  "verifier log line truncated - local buffer too short\n");

@@ -197,7 +190,32 @@ __printf(2, 3) void bpf_verifier_log_write(struct bpf_verifier_env *env,
 	else
 		log->ubuf = NULL;
 }
+
+/* log_level controls verbosity level of eBPF verifier.
+ * bpf_verifier_log_write() is used to dump the verification trace to the log,
+ * so the user can figure out what's wrong with the program
+ */
+__printf(2, 3) void bpf_verifier_log_write(struct bpf_verifier_env *env,
+					   const char *fmt, ...)
+{

kernel/bpf/verifier.c +11 -2

+{
+	va_list args;
+
+	va_start(args, fmt);
+	log_write(private_data, fmt, args);
+	va_end(args);
+}
+
 /* Historically bpf_verifier_log_write was called verbose, but the name was too
  * generic for symbol export. The function was renamed, but not the calls in
  * the verifier to avoid complicating backports. Hence the alias below.

@@ -4599,11 +4617,12 @@ static int do_check(struct bpf_verifier_env *env)
 
 		if (env->log.level) {
 			const struct bpf_insn_cbs cbs = {
-				.cb_print	= verbose,
+				.cb_print	= print_ins,
+				.private_data	= env,
 			};
 
 			verbose(env, "%d: ", insn_idx);
-			print_bpf_insn(&cbs, env, insn, env->allow_ptr_leaks);
+			print_bpf_insn(&cbs, insn, env->allow_ptr_leaks);
 		}
 
 		if (bpf_prog_is_dev_bound(env->prog->aux)) {

2018-03-22 14:32 UTC+0100 ~ Jiri Olsa <[email protected]>

Quoted 41 lines · Daniel Borkmann

Quoted 40 lines · Jiri Olsa

Quoted 38 lines · Quentin Monnet

Quoted 26 lines

2018-03-21 16:02 UTC+0100 ~ Jiri Olsa <[email protected]>

Quoted 13 lines

We use print_bpf_insn in user space (bpftool and soon perf), so it'd be nice to keep it generic and strip it off the kernel struct bpf_verifier_env argument.

This argument can be safely removed, because its users can use the struct bpf_insn_cbs::private_data to pass it.

Signed-off-by Jiri Olsa <[email protected]>

---
 kernel/bpf/disasm.c   | 52 +++++++++++++++++++++++++--------------------------
 kernel/bpf/disasm.h   |  5 +----
 kernel/bpf/verifier.c |  6 +++---
 3 files changed, 30 insertions(+), 33 deletions(-)

[...]

Quoted 9 lines

kernel/bpf/verifier.c +0 -1

index c6eff108aa99..9f27d3fa7259 100644
@@ -202,8 +202,7 @@ EXPORT_SYMBOL_GPL(bpf_verifier_log_write);
  * generic for symbol export. The function was renamed, but not the calls in
  * the verifier to avoid complicating backports. Hence the alias below.
  */
-static __printf(2, 3) void verbose(struct bpf_verifier_env *env,

Quoted 8 lines

kernel/bpf/verifier.c EXPORT_SYMBOL_GPL(bpf_verifier_log_write); · line 206

Quoted 3 lines

kernel/bpf/verifier.c +1 -1

-				   const char *fmt, ...)
+static __printf(2, 3) void verbose(void *private_data, const char *fmt, ...)
 	__attribute__((alias("bpf_verifier_log_write")));

Just as a note, verbose() will be aliased to a function whose prototype differs (bpf_verifier_log_write() still expects a struct bpf_verifier_env as its first argument). I am not so familiar with function aliases, could this change be a concern?

yea, but as it was pointer for pointer switch I did not see any problem with that.. I'll check more

Quoted 7 lines

Quoted 3 lines

Ok, holding off for now until we have clarification. Other option could also be to make it void *private_data everywhere and for the kernel writer then do struct bpf_verifier_env *env = private_data.

can't find much info about the alias behaviour for this case.. so how about having separate function for the print_cb like below.. I still need to test it

I have nothing against splitting the function. This is a solution I considered for verbose() and bpf_verifier_log_write(), before switching to function alias (on Daniel's suggestion) because the code would be identical for the two separate functions at that time. But with your change they would not have the same signature anymore, so it sounds reasonable. Just a note below...

Quoted 11 lines

thanks,
jirka

---
 kernel/bpf/disasm.c   | 52 +++++++++++++++++++++++++--------------------------
 kernel/bpf/disasm.h   |  5 +----
 kernel/bpf/verifier.c | 41 +++++++++++++++++++++++++++++-----------
 3 files changed, 57 insertions(+), 41 deletions(-)

[...]

Quoted 47 lines

kernel/bpf/verifier.c +15 -9

index e9f7c20691c1..69bf7590877c 100644
@@ -168,23 +168,16 @@ struct bpf_call_arg_meta {
 
 static DEFINE_MUTEX(bpf_verifier_lock);
 
-/* log_level controls verbosity level of eBPF verifier.
- * bpf_verifier_log_write() is used to dump the verification trace to the log,
- * so the user can figure out what's wrong with the program
- */
-__printf(2, 3) void bpf_verifier_log_write(struct bpf_verifier_env *env,
-					   const char *fmt, ...)
+static void log_write(struct bpf_verifier_env *env, const char *fmt,
+		      va_list args)
 {
 	struct bpf_verifer_log *log = &env->log;
 	unsigned int n;
-	va_list args;
 
 	if (!log->level || !log->ubuf || bpf_verifier_log_full(log))
 		return;
 
-	va_start(args, fmt);
 	n = vscnprintf(log->kbuf, BPF_VERIFIER_TMP_LOG_SIZE, fmt, args);
-	va_end(args);
 
 	WARN_ONCE(n >= BPF_VERIFIER_TMP_LOG_SIZE - 1,
 		  "verifier log line truncated - local buffer too short\n");

@@ -197,7 +190,32 @@ __printf(2, 3) void bpf_verifier_log_write(struct bpf_verifier_env *env,
 	else
 		log->ubuf = NULL;
 }
+
+/* log_level controls verbosity level of eBPF verifier.
+ * bpf_verifier_log_write() is used to dump the verification trace to the log,
+ * so the user can figure out what's wrong with the program
+ */
+__printf(2, 3) void bpf_verifier_log_write(struct bpf_verifier_env *env,
+					   const char *fmt, ...)
+{
+	va_list args;
+
+	va_start(args, fmt);
+	log_write(env, fmt, args);
+	va_end(args);

kernel/bpf/verifier.c __printf(2, 3) void bpf_verifier_log_write(struct bpf_verifier_env *env, · line 210

Quoted 5 lines

kernel/bpf/verifier.c +4 -0

+}
 EXPORT_SYMBOL_GPL(bpf_verifier_log_write);
+
+__printf(2, 3) static void print_ins(void *private_data,
+				     const char *fmt, ...)

Unless I am mistaken, you could just call this one "verbose()" and simply remove the function alias?

Quoted 118 lines · Quentin Monnet

2018-03-22 14:32 UTC+0100 ~ Jiri Olsa <[email protected]>

Quoted 41 lines · Daniel Borkmann

Quoted 40 lines · Jiri Olsa

Quoted 38 lines · Quentin Monnet

Quoted 26 lines

2018-03-21 16:02 UTC+0100 ~ Jiri Olsa <[email protected]>

Quoted 13 lines

We use print_bpf_insn in user space (bpftool and soon perf), so it'd be nice to keep it generic and strip it off the kernel struct bpf_verifier_env argument.

This argument can be safely removed, because its users can use the struct bpf_insn_cbs::private_data to pass it.

Signed-off-by Jiri Olsa <[email protected]>

---
 kernel/bpf/disasm.c   | 52 +++++++++++++++++++++++++--------------------------
 kernel/bpf/disasm.h   |  5 +----
 kernel/bpf/verifier.c |  6 +++---
 3 files changed, 30 insertions(+), 33 deletions(-)

[...]

Quoted 9 lines

kernel/bpf/verifier.c +0 -1

index c6eff108aa99..9f27d3fa7259 100644
@@ -202,8 +202,7 @@ EXPORT_SYMBOL_GPL(bpf_verifier_log_write);
  * generic for symbol export. The function was renamed, but not the calls in
  * the verifier to avoid complicating backports. Hence the alias below.
  */
-static __printf(2, 3) void verbose(struct bpf_verifier_env *env,

Quoted 8 lines

kernel/bpf/verifier.c EXPORT_SYMBOL_GPL(bpf_verifier_log_write); · line 206

Quoted 3 lines

kernel/bpf/verifier.c +1 -1

-				   const char *fmt, ...)
+static __printf(2, 3) void verbose(void *private_data, const char *fmt, ...)
 	__attribute__((alias("bpf_verifier_log_write")));

Just as a note, verbose() will be aliased to a function whose prototype differs (bpf_verifier_log_write() still expects a struct bpf_verifier_env as its first argument). I am not so familiar with function aliases, could this change be a concern?

yea, but as it was pointer for pointer switch I did not see any problem with that.. I'll check more

Quoted 7 lines

Quoted 3 lines

Ok, holding off for now until we have clarification. Other option could also be to make it void *private_data everywhere and for the kernel writer then do struct bpf_verifier_env *env = private_data.

can't find much info about the alias behaviour for this case.. so how about having separate function for the print_cb like below.. I still need to test it

I have nothing against splitting the function. This is a solution I considered for verbose() and bpf_verifier_log_write(), before switching to function alias (on Daniel's suggestion) because the code would be identical for the two separate functions at that time. But with your change they would not have the same signature anymore, so it sounds reasonable. Just a note below...

Quoted 11 lines

thanks,
jirka

---
 kernel/bpf/disasm.c   | 52 +++++++++++++++++++++++++--------------------------
 kernel/bpf/disasm.h   |  5 +----
 kernel/bpf/verifier.c | 41 +++++++++++++++++++++++++++++-----------
 3 files changed, 57 insertions(+), 41 deletions(-)

[...]

Quoted 47 lines

kernel/bpf/verifier.c +15 -9

index e9f7c20691c1..69bf7590877c 100644
@@ -168,23 +168,16 @@ struct bpf_call_arg_meta {
 
 static DEFINE_MUTEX(bpf_verifier_lock);
 
-/* log_level controls verbosity level of eBPF verifier.
- * bpf_verifier_log_write() is used to dump the verification trace to the log,
- * so the user can figure out what's wrong with the program
- */
-__printf(2, 3) void bpf_verifier_log_write(struct bpf_verifier_env *env,
-					   const char *fmt, ...)
+static void log_write(struct bpf_verifier_env *env, const char *fmt,
+		      va_list args)
 {
 	struct bpf_verifer_log *log = &env->log;
 	unsigned int n;
-	va_list args;
 
 	if (!log->level || !log->ubuf || bpf_verifier_log_full(log))
 		return;
 
-	va_start(args, fmt);
 	n = vscnprintf(log->kbuf, BPF_VERIFIER_TMP_LOG_SIZE, fmt, args);
-	va_end(args);
 
 	WARN_ONCE(n >= BPF_VERIFIER_TMP_LOG_SIZE - 1,
 		  "verifier log line truncated - local buffer too short\n");

@@ -197,7 +190,32 @@ __printf(2, 3) void bpf_verifier_log_write(struct bpf_verifier_env *env,
 	else
 		log->ubuf = NULL;
 }
+
+/* log_level controls verbosity level of eBPF verifier.
+ * bpf_verifier_log_write() is used to dump the verification trace to the log,
+ * so the user can figure out what's wrong with the program
+ */
+__printf(2, 3) void bpf_verifier_log_write(struct bpf_verifier_env *env,
+					   const char *fmt, ...)
+{
+	va_list args;
+
+	va_start(args, fmt);
+	log_write(env, fmt, args);
+	va_end(args);

Quoted 8 lines

kernel/bpf/verifier.c __printf(2, 3) void bpf_verifier_log_write(struct bpf_verifier_env *env, · line 210

Quoted 5 lines

kernel/bpf/verifier.c +4 -0

+}
 EXPORT_SYMBOL_GPL(bpf_verifier_log_write);
+
+__printf(2, 3) static void print_ins(void *private_data,
+				     const char *fmt, ...)

Unless I am mistaken, you could just call this one "verbose()" and simply remove the function alias?

right you are ;-) I haven't realized that struct bpf_verifier_env *env will cleanly cast to void *

new version attached.. I'll make some tests and send full patch

Quoted 10 lines

Quoted 9 lines

kernel/bpf/verifier.c __printf(2, 3) void bpf_verifier_log_write(struct bpf_verifier_env *env,

Quoted 6 lines

kernel/bpf/verifier.c +5 -0

+	va_end(args);
+}
 EXPORT_SYMBOL_GPL(bpf_verifier_log_write);
+
+__printf(2, 3) static void print_ins(void *private_data,
+				     const char *fmt, ...)

Unless I am mistaken, you could just call this one "verbose()" and simply remove the function alias?

Quoted 4 lines

right you are ;-) I haven't realized that struct bpf_verifier_env *env will cleanly cast to void *

new version attached.. I'll make some tests and send full patch

When you do so, please make sure to send a full fresh series with the two patches and also cover letter included, otherwise it's more fragile wrt potentially applying the wrong patch from one of the replies. :-)