[PATCH net-next 0/8] tools: bpftool: add a "version" command, and fix several items

[PATCH net-next 0/8] tools: bpftool: add a "version" command, and fix several items

[Jakub Kicinski]

Quentin says:

The first seven patches of this series bring several minor fixes to
bpftool. Please see individual commit logs for details.

Last patch adds a "version" commands to bpftool, which is in fact the
version of the kernel from which it was compiled.


Quentin Monnet (8):
  tools: bpftool: add pointer to file argument to print_hex()
  tools: bpftool: fix return value when all eBPF programs have been
    shown
  tools: bpftool: use err() instead of info() if there are too many
    insns
  tools: bpftool: add `bpftool prog help` as real command i.r.t exit
    code
  tools: bpftool: print only one error message on byte parsing failure
  tools: bpftool: print all relevant byte opcodes for "load double word"
  tools: bpftool: show that `opcodes` or `file FILE` should be exclusive
  tools: bpftool: add a command to display bpftool version

 tools/bpf/bpftool/Documentation/bpftool-prog.rst |  8 +++----
 tools/bpf/bpftool/Documentation/bpftool.rst      |  2 ++
 tools/bpf/bpftool/main.c                         | 22 +++++++++++++----
 tools/bpf/bpftool/main.h                         |  2 +-
 tools/bpf/bpftool/map.c                          | 22 ++++++++---------
 tools/bpf/bpftool/prog.c                         | 30 +++++++++++++++++-------
 6 files changed, 56 insertions(+), 30 deletions(-)

-- 
2.14.1

[PATCH net-next 1/8] tools: bpftool: add pointer to file argument to print_hex()

[Jakub Kicinski]

From: Quentin Monnet <quentin.monnet@netronome.com>

Make print_hex() able to print to any file instead of standard output
only, and rename it to fprint_hex(). The function can now be called with
the info() macro, for example, without splitting the output between
standard and error outputs.

Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 tools/bpf/bpftool/main.c |  8 ++++----
 tools/bpf/bpftool/main.h |  2 +-
 tools/bpf/bpftool/map.c  | 20 ++++++++++----------
 tools/bpf/bpftool/prog.c |  4 ++--
 4 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
index e02d00d6e00b..8662199ee050 100644
--- a/tools/bpf/bpftool/main.c
+++ b/tools/bpf/bpftool/main.c
@@ -100,7 +100,7 @@ bool is_prefix(const char *pfx, const char *str)
 	return !memcmp(str, pfx, strlen(pfx));
 }
 
-void print_hex(void *arg, unsigned int n, const char *sep)
+void fprint_hex(FILE *f, void *arg, unsigned int n, const char *sep)
 {
 	unsigned char *data = arg;
 	unsigned int i;
@@ -111,13 +111,13 @@ void print_hex(void *arg, unsigned int n, const char *sep)
 		if (!i)
 			/* nothing */;
 		else if (!(i % 16))
-			printf("\n");
+			fprintf(f, "\n");
 		else if (!(i % 8))
-			printf("  ");
+			fprintf(f, "  ");
 		else
 			pfx = sep;
 
-		printf("%s%02hhx", i ? pfx : "", data[i]);
+		fprintf(f, "%s%02hhx", i ? pfx : "", data[i]);
 	}
 }
 
diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
index 844e4ef6db56..41e6c7d3fcad 100644
--- a/tools/bpf/bpftool/main.h
+++ b/tools/bpf/bpftool/main.h
@@ -67,7 +67,7 @@ enum bpf_obj_type {
 extern const char *bin_name;
 
 bool is_prefix(const char *pfx, const char *str);
-void print_hex(void *arg, unsigned int n, const char *sep);
+void fprint_hex(FILE *f, void *arg, unsigned int n, const char *sep);
 void usage(void) __attribute__((noreturn));
 
 struct cmd {
diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index 0528a5379e6c..b1dad76215ed 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -216,12 +216,12 @@ static void print_entry(struct bpf_map_info *info, unsigned char *key,
 			!break_names;
 
 		printf("key:%c", break_names ? '\n' : ' ');
-		print_hex(key, info->key_size, " ");
+		fprint_hex(stdout, key, info->key_size, " ");
 
 		printf(single_line ? "  " : "\n");
 
 		printf("value:%c", break_names ? '\n' : ' ');
-		print_hex(value, info->value_size, " ");
+		fprint_hex(stdout, value, info->value_size, " ");
 
 		printf("\n");
 	} else {
@@ -230,13 +230,13 @@ static void print_entry(struct bpf_map_info *info, unsigned char *key,
 		n = get_possible_cpus();
 
 		printf("key:\n");
-		print_hex(key, info->key_size, " ");
+		fprint_hex(stdout, key, info->key_size, " ");
 		printf("\n");
 		for (i = 0; i < n; i++) {
 			printf("value (CPU %02d):%c",
 			       i, info->value_size > 16 ? '\n' : ' ');
-			print_hex(value + i * info->value_size,
-				  info->value_size, " ");
+			fprint_hex(stdout, value + i * info->value_size,
+				   info->value_size, " ");
 			printf("\n");
 		}
 	}
@@ -492,8 +492,8 @@ static int do_dump(int argc, char **argv)
 			print_entry(&info, key, value);
 		} else {
 			info("can't lookup element with key: ");
-			print_hex(key, info.key_size, " ");
-			printf("\n");
+			fprint_hex(stderr, key, info.key_size, " ");
+			fprintf(stderr, "\n");
 		}
 
 		prev_key = key;
@@ -587,7 +587,7 @@ static int do_lookup(int argc, char **argv)
 		print_entry(&info, key, value);
 	} else if (errno == ENOENT) {
 		printf("key:\n");
-		print_hex(key, info.key_size, " ");
+		fprint_hex(stdout, key, info.key_size, " ");
 		printf("\n\nNot found\n");
 	} else {
 		err("lookup failed: %s\n", strerror(errno));
@@ -642,14 +642,14 @@ static int do_getnext(int argc, char **argv)
 
 	if (key) {
 		printf("key:\n");
-		print_hex(key, info.key_size, " ");
+		fprint_hex(stdout, key, info.key_size, " ");
 		printf("\n");
 	} else {
 		printf("key: None\n");
 	}
 
 	printf("next key:\n");
-	print_hex(nextkey, info.key_size, " ");
+	fprint_hex(stdout, nextkey, info.key_size, " ");
 	printf("\n");
 
 exit_free:
diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index d60f5307b6e2..aa6d72ea3807 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -224,7 +224,7 @@ static int show_prog(int fd)
 		printf("name %s  ", info.name);
 
 	printf("tag ");
-	print_hex(info.tag, BPF_TAG_SIZE, "");
+	fprint_hex(stdout, info.tag, BPF_TAG_SIZE, "");
 	printf("\n");
 
 	if (info.load_time) {
@@ -319,7 +319,7 @@ static void dump_xlated(void *buf, unsigned int len, bool opcodes)
 
 		if (opcodes) {
 			printf("       ");
-			print_hex(insn + i, 8, " ");
+			fprint_hex(stdout, insn + i, 8, " ");
 			printf("\n");
 		}
 
-- 
2.14.1

[PATCH net-next 2/8] tools: bpftool: fix return value when all eBPF programs have been shown

[Jakub Kicinski]

From: Quentin Monnet <quentin.monnet@netronome.com>

Change the program to have a more consistent return code. Specifically,
do not make bpftool return an error code simply because it reaches the
end of the list of the eBPF programs to show.

Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 tools/bpf/bpftool/prog.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index aa6d72ea3807..ede7957adcd9 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -275,8 +275,10 @@ static int do_show(int argc, char **argv)
 	while (true) {
 		err = bpf_prog_get_next_id(id, &id);
 		if (err) {
-			if (errno == ENOENT)
+			if (errno == ENOENT) {
+				err = 0;
 				break;
+			}
 			err("can't get next program: %s\n", strerror(errno));
 			if (errno == EINVAL)
 				err("kernel too old?\n");
-- 
2.14.1

[PATCH net-next 3/8] tools: bpftool: use err() instead of info() if there are too many insns

[Jakub Kicinski]

From: Quentin Monnet <quentin.monnet@netronome.com>

Make error messages and return codes more consistent. Specifically,
replace the use of info() macro with err() when too many eBPF
instructions are received to be dumped, given that bpftool returns with
a non-null exit value in that case.

Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 tools/bpf/bpftool/prog.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index ede7957adcd9..6c03d2ea3f79 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -416,7 +416,7 @@ static int do_dump(int argc, char **argv)
 	}
 
 	if (*member_len > buf_size) {
-		info("too many instructions returned\n");
+		err("too many instructions returned\n");
 		goto err_free;
 	}
 
-- 
2.14.1

[PATCH net-next 4/8] tools: bpftool: add `bpftool prog help` as real command i.r.t exit code

[Jakub Kicinski]

From: Quentin Monnet <quentin.monnet@netronome.com>

Make error messages and return codes more consistent. Specifically, make
`bpftool prog help` a real command, instead of printing usage by default
for a non-recognized "help" command. Output is the same, but this makes
bpftool return with a success value instead of an error.

Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 tools/bpf/bpftool/prog.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index 6c03d2ea3f79..355c14325622 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -475,6 +475,7 @@ static int do_help(int argc, char **argv)
 
 static const struct cmd cmds[] = {
 	{ "show",	do_show },
+	{ "help",	do_help },
 	{ "dump",	do_dump },
 	{ "pin",	do_pin },
 	{ 0 }
-- 
2.14.1

[PATCH net-next 5/8] tools: bpftool: print only one error message on byte parsing failure

[Jakub Kicinski]

From: Quentin Monnet <quentin.monnet@netronome.com>

Make error messages more consistent. Specifically, when bpftool fails at
parsing map key bytes, make it print a single error message to stderr
and return from the function, instead of (always) printing a second
error message afterwards.

Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 tools/bpf/bpftool/map.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index b1dad76215ed..e1004d825392 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -252,7 +252,7 @@ static char **parse_bytes(char **argv, const char *name, unsigned char *val,
 		val[i] = strtoul(argv[i], &endptr, 0);
 		if (*endptr) {
 			err("error parsing byte: %s\n", argv[i]);
-			break;
+			return NULL;
 		}
 		i++;
 	}
-- 
2.14.1

[PATCH net-next 6/8] tools: bpftool: print all relevant byte opcodes for "load double word"

[Jakub Kicinski]

From: Quentin Monnet <quentin.monnet@netronome.com>

The eBPF instruction permitting to load double words (8 bytes) into a
register need 8-byte long "immediate" field, and thus occupy twice the
space of other instructions. bpftool was aware of this and would
increment the instruction counter only once on meeting such instruction,
but it would only print the first four bytes of the immediate value to
load. Make it able to dump the whole 16 byte-long double instruction
instead (as would `llvm-objdump -d <program>`).

Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 tools/bpf/bpftool/prog.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index 355c14325622..57edbea2fbe8 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -313,20 +313,29 @@ static void print_insn(struct bpf_verifier_env *env, const char *fmt, ...)
 static void dump_xlated(void *buf, unsigned int len, bool opcodes)
 {
 	struct bpf_insn *insn = buf;
+	bool double_insn = false;
 	unsigned int i;
 
 	for (i = 0; i < len / sizeof(*insn); i++) {
+		if (double_insn) {
+			double_insn = false;
+			continue;
+		}
+
+		double_insn = insn[i].code == (BPF_LD | BPF_IMM | BPF_DW);
+
 		printf("% 4d: ", i);
 		print_bpf_insn(print_insn, NULL, insn + i, true);
 
 		if (opcodes) {
 			printf("       ");
 			fprint_hex(stdout, insn + i, 8, " ");
+			if (double_insn && i < len - 1) {
+				printf(" ");
+				fprint_hex(stdout, insn + i + 1, 8, " ");
+			}
 			printf("\n");
 		}
-
-		if (insn[i].code == (BPF_LD | BPF_IMM | BPF_DW))
-			i++;
 	}
 }
 
-- 
2.14.1

[PATCH net-next 7/8] tools: bpftool: show that `opcodes` or `file FILE` should be exclusive

[Jakub Kicinski]

From: Quentin Monnet <quentin.monnet@netronome.com>

For the `bpftool prog dump { jited | xlated } ...` command, adding
`opcodes` keyword (to request opcodes to be printed) will have no effect
if `file FILE` (to write binary output to FILE) is provided.

The manual page and the help message to be displayed in the terminal
should reflect that, and indicate that these options should be mutually
exclusive.

Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 tools/bpf/bpftool/Documentation/bpftool-prog.rst | 8 ++++----
 tools/bpf/bpftool/prog.c                         | 4 ++--
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
index 3968f0bd37db..69b3770370c8 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
@@ -11,8 +11,8 @@ SYNOPSIS
 ========
 
 |	**bpftool** prog show [*PROG*]
-|	**bpftool** prog dump xlated *PROG* [file *FILE*] [opcodes]
-|	**bpftool** prog dump jited  *PROG* [file *FILE*] [opcodes]
+|	**bpftool** prog dump xlated *PROG* [{file *FILE* | opcodes }]
+|	**bpftool** prog dump jited  *PROG* [{file *FILE* | opcodes }]
 |	**bpftool** prog pin *PROG* *FILE*
 |	**bpftool** prog help
 |
@@ -28,14 +28,14 @@ DESCRIPTION
 		  Output will start with program ID followed by program type and
 		  zero or more named attributes (depending on kernel version).
 
-	**bpftool prog dump xlated** *PROG* [**file** *FILE*] [**opcodes**]
+	**bpftool prog dump xlated** *PROG* [{ **file** *FILE* | **opcodes** }]
 		  Dump eBPF instructions of the program from the kernel.
 		  If *FILE* is specified image will be written to a file,
 		  otherwise it will be disassembled and printed to stdout.
 
 		  **opcodes** controls if raw opcodes will be printed.
 
-	**bpftool prog dump jited**  *PROG* [**file** *FILE*] [**opcodes**]
+	**bpftool prog dump jited**  *PROG* [{ **file** *FILE* | **opcodes** }]
 		  Dump jited image (host machine code) of the program.
 		  If *FILE* is specified image will be written to a file,
 		  otherwise it will be disassembled and printed to stdout.
diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index 57edbea2fbe8..7838206a455b 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -469,8 +469,8 @@ static int do_help(int argc, char **argv)
 {
 	fprintf(stderr,
 		"Usage: %s %s show [PROG]\n"
-		"       %s %s dump xlated PROG [file FILE] [opcodes]\n"
-		"       %s %s dump jited  PROG [file FILE] [opcodes]\n"
+		"       %s %s dump xlated PROG [{ file FILE | opcodes }]\n"
+		"       %s %s dump jited  PROG [{ file FILE | opcodes }]\n"
 		"       %s %s pin   PROG FILE\n"
 		"       %s %s help\n"
 		"\n"
-- 
2.14.1

[PATCH net-next 8/8] tools: bpftool: add a command to display bpftool version

[Jakub Kicinski]

From: Quentin Monnet <quentin.monnet@netronome.com>

This command can be used to print the version of the tool, which is in
fact the version from Linux taken from usr/include/linux/version.h.

Example usage:

    $ bpftool version
    bpftool v4.14.0

Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 tools/bpf/bpftool/Documentation/bpftool.rst |  2 ++
 tools/bpf/bpftool/main.c                    | 14 +++++++++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/tools/bpf/bpftool/Documentation/bpftool.rst b/tools/bpf/bpftool/Documentation/bpftool.rst
index f1df1893fb54..45ad8baf1915 100644
--- a/tools/bpf/bpftool/Documentation/bpftool.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool.rst
@@ -14,6 +14,8 @@ SYNOPSIS
 
 	**bpftool** batch file *FILE*
 
+	**bpftool** version
+
 	*OBJECT* := { **map** | **program** }
 
 	*MAP-COMMANDS* :=
diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
index 8662199ee050..814d19e1b53f 100644
--- a/tools/bpf/bpftool/main.c
+++ b/tools/bpf/bpftool/main.c
@@ -37,6 +37,7 @@
 #include <ctype.h>
 #include <errno.h>
 #include <linux/bpf.h>
+#include <linux/version.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -62,13 +63,23 @@ static int do_help(int argc, char **argv)
 	fprintf(stderr,
 		"Usage: %s OBJECT { COMMAND | help }\n"
 		"       %s batch file FILE\n"
+		"       %s version\n"
 		"\n"
 		"       OBJECT := { prog | map }\n",
-		bin_name, bin_name);
+		bin_name, bin_name, bin_name);
 
 	return 0;
 }
 
+static int do_version(int argc, char **argv)
+{
+	printf("%s v%d.%d.%d\n", bin_name,
+	       LINUX_VERSION_CODE >> 16,
+	       LINUX_VERSION_CODE >> 8 & 0xf,
+	       LINUX_VERSION_CODE & 0xf);
+	return 0;
+}
+
 int cmd_select(const struct cmd *cmds, int argc, char **argv,
 	       int (*help)(int argc, char **argv))
 {
@@ -128,6 +139,7 @@ static const struct cmd cmds[] = {
 	{ "batch",	do_batch },
 	{ "prog",	do_prog },
 	{ "map",	do_map },
+	{ "version",	do_version },
 	{ 0 }
 };
 
-- 
2.14.1

Re: [PATCH net-next 0/8] tools: bpftool: add a "version" command, and fix several items

[Alexei Starovoitov]

On Thu, Oct 19, 2017 at 03:46:18PM -0700, Jakub Kicinski wrote:
> Quentin says:
> 
> The first seven patches of this series bring several minor fixes to
> bpftool. Please see individual commit logs for details.
> 
> Last patch adds a "version" commands to bpftool, which is in fact the
> version of the kernel from which it was compiled.

lgtm
Acked-by: Alexei Starovoitov <ast@kernel.org>

RE: [PATCH net-next 6/8] tools: bpftool: print all relevant byte opcodes for "load double word"

[David Laight]

From: Jakub Kicinski
> Sent: 19 October 2017 23:46
> The eBPF instruction permitting to load double words (8 bytes) into a
> register need 8-byte long "immediate" field, and thus occupy twice the
> space of other instructions. bpftool was aware of this and would
> increment the instruction counter only once on meeting such instruction,
> but it would only print the first four bytes of the immediate value to
> load. Make it able to dump the whole 16 byte-long double instruction
> instead (as would `llvm-objdump -d <program>`).

Guess why most modern instruction sets use a 'load high' instruction
to generate big constants...

Interestingly, is there anything special in the rest of the
second instruction in order to make it an identifiable no-op?

...
> diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> index 355c14325622..57edbea2fbe8 100644
> --- a/tools/bpf/bpftool/prog.c
> +++ b/tools/bpf/bpftool/prog.c
> @@ -313,20 +313,29 @@ static void print_insn(struct bpf_verifier_env *env, const char *fmt, ...)
>  static void dump_xlated(void *buf, unsigned int len, bool opcodes)
>  {
>  	struct bpf_insn *insn = buf;
> +	bool double_insn = false;
>  	unsigned int i;
> 
>  	for (i = 0; i < len / sizeof(*insn); i++) {
> +		if (double_insn) {
> +			double_insn = false;
> +			continue;
> +		}

Why not just:
	for (i = 0; i < len / sizeof(*insn); i += 1 + double_insn) {

...

	David

Re: [PATCH net-next 1/8] tools: bpftool: add pointer to file argument to print_hex()

[Daniel Borkmann]

On 10/20/2017 12:46 AM, Jakub Kicinski wrote:
> From: Quentin Monnet <quentin.monnet@netronome.com>
>
> Make print_hex() able to print to any file instead of standard output
> only, and rename it to fprint_hex(). The function can now be called with
> the info() macro, for example, without splitting the output between
> standard and error outputs.
>
> Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

Re: [PATCH net-next 2/8] tools: bpftool: fix return value when all eBPF programs have been shown

[Daniel Borkmann]

On 10/20/2017 12:46 AM, Jakub Kicinski wrote:
> From: Quentin Monnet <quentin.monnet@netronome.com>
>
> Change the program to have a more consistent return code. Specifically,
> do not make bpftool return an error code simply because it reaches the
> end of the list of the eBPF programs to show.
>
> Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

Re: [PATCH net-next 3/8] tools: bpftool: use err() instead of info() if there are too many insns

[Daniel Borkmann]

On 10/20/2017 12:46 AM, Jakub Kicinski wrote:
> From: Quentin Monnet <quentin.monnet@netronome.com>
>
> Make error messages and return codes more consistent. Specifically,
> replace the use of info() macro with err() when too many eBPF
> instructions are received to be dumped, given that bpftool returns with
> a non-null exit value in that case.
>
> Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

Re: [PATCH net-next 4/8] tools: bpftool: add `bpftool prog help` as real command i.r.t exit code

[Daniel Borkmann]

On 10/20/2017 12:46 AM, Jakub Kicinski wrote:
> From: Quentin Monnet <quentin.monnet@netronome.com>
>
> Make error messages and return codes more consistent. Specifically, make
> `bpftool prog help` a real command, instead of printing usage by default
> for a non-recognized "help" command. Output is the same, but this makes
> bpftool return with a success value instead of an error.
>
> Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

Re: [PATCH net-next 5/8] tools: bpftool: print only one error message on byte parsing failure

[Daniel Borkmann]

On 10/20/2017 12:46 AM, Jakub Kicinski wrote:
> From: Quentin Monnet <quentin.monnet@netronome.com>
>
> Make error messages more consistent. Specifically, when bpftool fails at
> parsing map key bytes, make it print a single error message to stderr
> and return from the function, instead of (always) printing a second
> error message afterwards.
>
> Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

Re: [PATCH net-next 6/8] tools: bpftool: print all relevant byte opcodes for "load double word"

[Daniel Borkmann]

On 10/20/2017 12:46 AM, Jakub Kicinski wrote:
> From: Quentin Monnet <quentin.monnet@netronome.com>
>
> The eBPF instruction permitting to load double words (8 bytes) into a
> register need 8-byte long "immediate" field, and thus occupy twice the
> space of other instructions. bpftool was aware of this and would
> increment the instruction counter only once on meeting such instruction,
> but it would only print the first four bytes of the immediate value to
> load. Make it able to dump the whole 16 byte-long double instruction
> instead (as would `llvm-objdump -d <program>`).
>
> Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

Re: [PATCH net-next 7/8] tools: bpftool: show that `opcodes` or `file FILE` should be exclusive

[Daniel Borkmann]

On 10/20/2017 12:46 AM, Jakub Kicinski wrote:
> From: Quentin Monnet <quentin.monnet@netronome.com>
>
> For the `bpftool prog dump { jited | xlated } ...` command, adding
> `opcodes` keyword (to request opcodes to be printed) will have no effect
> if `file FILE` (to write binary output to FILE) is provided.
>
> The manual page and the help message to be displayed in the terminal
> should reflect that, and indicate that these options should be mutually
> exclusive.
>
> Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

Re: [PATCH net-next 8/8] tools: bpftool: add a command to display bpftool version

[Daniel Borkmann]

On 10/20/2017 12:46 AM, Jakub Kicinski wrote:
> From: Quentin Monnet <quentin.monnet@netronome.com>
>
> This command can be used to print the version of the tool, which is in
> fact the version from Linux taken from usr/include/linux/version.h.
>
> Example usage:
>
>      $ bpftool version
>      bpftool v4.14.0
>
> Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

Re: [PATCH net-next 6/8] tools: bpftool: print all relevant byte opcodes for "load double word"

[Quentin Monnet]

Hi David,

On 20 October 2017 at 10:59, David Laight <David.Laight@aculab.com> wrote:
> From: Jakub Kicinski
>> Sent: 19 October 2017 23:46
>> The eBPF instruction permitting to load double words (8 bytes) into a
>> register need 8-byte long "immediate" field, and thus occupy twice the
>> space of other instructions. bpftool was aware of this and would
>> increment the instruction counter only once on meeting such instruction,
>> but it would only print the first four bytes of the immediate value to
>> load. Make it able to dump the whole 16 byte-long double instruction
>> instead (as would `llvm-objdump -d <program>`).
>
> Guess why most modern instruction sets use a 'load high' instruction
> to generate big constants...
>
> Interestingly, is there anything special in the rest of the
> second instruction in order to make it an identifiable no-op?

The remaining four bytes are taken from the "immediate" field of the second
instruction, which leaves the first four fields (offset, source and destination
registers, and in particular opcode) unused. As far as I know, these fields
remain at zero, and this makes it the only “instruction” to have a null code
(although I am not sure this is a strict requirement, because I did not find
the code in the verifier that would reject a program having a non-null opcode
right after a "load double word immediate" instruction).

>

[…]

> Why not just:
>         for (i = 0; i < len / sizeof(*insn); i += 1 + double_insn) {
>
> ...
>
>         David
>

Yes, we could use that instead, although I am not sure this makes the code
more readable. So I do not believe this is worth a respin, tell me if you think
otherwise.

Thanks for the review!
Quentin

Re: [PATCH net-next 6/8] tools: bpftool: print all relevant byte opcodes for "load double word"

[Daniel Borkmann]

On 10/20/2017 05:50 PM, Quentin Monnet wrote:
[...]
> The remaining four bytes are taken from the "immediate" field of the second
> instruction, which leaves the first four fields (offset, source and destination
> registers, and in particular opcode) unused. As far as I know, these fields
> remain at zero, and this makes it the only “instruction” to have a null code
> (although I am not sure this is a strict requirement, because I did not find
> the code in the verifier that would reject a program having a non-null opcode
> right after a "load double word immediate" instruction).

It's in replace_map_fd_with_map_ptr(), invalid insns for the 2nd part
are rejected there, they have to be otherwise it's not extendable anymore
from abi pov; check also 'test1* ld_imm64' in the verifier test cases.

Cheers,
Daniel

Re: [PATCH net-next 6/8] tools: bpftool: print all relevant byte opcodes for "load double word"

[Quentin Monnet]

On 20 October 2017 at 17:12, Daniel Borkmann <daniel@iogearbox.net> wrote:
> On 10/20/2017 05:50 PM, Quentin Monnet wrote:
> [...]
>>
>> The remaining four bytes are taken from the "immediate" field of the
>> second
>> instruction, which leaves the first four fields (offset, source and
>> destination
>> registers, and in particular opcode) unused. As far as I know, these
>> fields
>> remain at zero, and this makes it the only “instruction” to have a null
>> code
>> (although I am not sure this is a strict requirement, because I did not
>> find
>> the code in the verifier that would reject a program having a non-null
>> opcode
>> right after a "load double word immediate" instruction).
>
>
> It's in replace_map_fd_with_map_ptr(), invalid insns for the 2nd part
> are rejected there, they have to be otherwise it's not extendable anymore
> from abi pov; check also 'test1* ld_imm64' in the verifier test cases.
>
> Cheers,
> Daniel

Indeed, thanks Daniel!
Quentin

Re: [PATCH net-next 0/8] tools: bpftool: add a "version" command, and fix several items

[David Miller]

From: Jakub Kicinski <jakub.kicinski@netronome.com>
Date: Thu, 19 Oct 2017 15:46:18 -0700

> Quentin says:
> 
> The first seven patches of this series bring several minor fixes to
> bpftool. Please see individual commit logs for details.
> 
> Last patch adds a "version" commands to bpftool, which is in fact the
> version of the kernel from which it was compiled.

Series applied, thank you.