[PATCH bpf-next 1/2] tools/lib/bpf: bpf_program__insns allow to retrieve insns in libbpf

[PATCH bpf-next 1/2] tools/lib/bpf: bpf_program__insns allow to retrieve insns in libbpf

[Lorenzo Fontana]

This allows consumers of libbpf to iterate trough the insns
of a program without loading it first directly after the ELF parsing.

Being able to do that is useful to create tooling that can show
the structure of a BPF program using libbpf without having to
parse the ELF separately.

Usage:
  struct bpf_insn *insn;
  insn = bpf_program__insns(prog);

Signed-off-by: Lorenzo Fontana <fontanalorenz@gmail.com>
---
 tools/lib/bpf/libbpf.c | 5 +++++
 tools/lib/bpf/libbpf.h | 1 +
 2 files changed, 6 insertions(+)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 1e04ce724240..67d51531f6b6 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -8866,6 +8866,11 @@ void *bpf_program__priv(const struct bpf_program *prog)
 	return prog ? prog->priv : libbpf_err_ptr(-EINVAL);
 }
 
+struct bpf_insn *bpf_program__insns(const struct bpf_program *prog)
+{
+	return prog ? prog->insns : libbpf_err_ptr(-EINVAL);
+}
+
 void bpf_program__set_ifindex(struct bpf_program *prog, __u32 ifindex)
 {
 	prog->prog_ifindex = ifindex;
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index 6e61342ba56c..e4a1c98ae6d9 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -195,6 +195,7 @@ typedef void (*bpf_program_clear_priv_t)(struct bpf_program *, void *);
 LIBBPF_API int bpf_program__set_priv(struct bpf_program *prog, void *priv,
 				     bpf_program_clear_priv_t clear_priv);
 
+LIBBPF_API struct bpf_insn *bpf_program__insns(const struct bpf_program *prog);
 LIBBPF_API void *bpf_program__priv(const struct bpf_program *prog);
 LIBBPF_API void bpf_program__set_ifindex(struct bpf_program *prog,
 					 __u32 ifindex);
-- 
2.32.0

[PATCH bpf-next 2/2] tools/bpf/bpftool: xlated dump from ELF file directly

[Lorenzo Fontana]

bpftool can dump an xlated or jitted representation
of the programs already loaded into the kernel.
That capability is very useful for understanding what
are the instructions the kernel will execute for that program.

However, sometimes the verifier does not load the program and
one cannot use this feature until changes are made to make the
verifier happy again.

This patch reuses the same dump function to dump the program
from an ELF file directly instead of loading the instructions
from a loaded file descriptor. In this way, the user
can use all the bpftool features for "xlated" without loading.

In particular, the "visual" command is very useful when combined
to this because the dot graph makes easy to spot bad instruction
sequences.

Usage:

  bpftool prog dump xlated elf program.o

It also works with the other commands like 'visual' to print
an dot representation of the program.

  bpftool prog dump xlated elf program.o visual

Signed-off-by: Lorenzo Fontana <fontanalorenz@gmail.com>
---
 tools/bpf/bpftool/common.c | 15 ++++++++++++---
 tools/bpf/bpftool/main.h   |  2 +-
 tools/bpf/bpftool/prog.c   | 26 +++++++++++++++++++++++---
 3 files changed, 36 insertions(+), 7 deletions(-)

diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
index 1828bba19020..b28d15505705 100644
--- a/tools/bpf/bpftool/common.c
+++ b/tools/bpf/bpftool/common.c
@@ -703,7 +703,7 @@ static int prog_fd_by_nametag(void *nametag, int **fds, bool tag)
 	return -1;
 }
 
-int prog_parse_fds(int *argc, char ***argv, int **fds)
+int prog_parse_fds(int *argc, char ***argv, int **fds, char **elf_filepath)
 {
 	if (is_prefix(**argv, "id")) {
 		unsigned int id;
@@ -763,9 +763,18 @@ int prog_parse_fds(int *argc, char ***argv, int **fds)
 		if ((*fds)[0] < 0)
 			return -1;
 		return 1;
+	} else if (is_prefix(**argv, "elf")) {
+		NEXT_ARGP();
+		if (!argc) {
+			p_err("expected ELF file path");
+			return -1;
+		}
+		*elf_filepath = **argv;
+		NEXT_ARGP();
+		return 1;
 	}
 
-	p_err("expected 'id', 'tag', 'name' or 'pinned', got: '%s'?", **argv);
+	p_err("expected 'id', 'tag', 'name', 'elf' or 'pinned', got: '%s'?", **argv);
 	return -1;
 }
 
@@ -779,7 +788,7 @@ int prog_parse_fd(int *argc, char ***argv)
 		p_err("mem alloc failed");
 		return -1;
 	}
-	nb_fds = prog_parse_fds(argc, argv, &fds);
+	nb_fds = prog_parse_fds(argc, argv, &fds, NULL);
 	if (nb_fds != 1) {
 		if (nb_fds > 1) {
 			p_err("several programs match this handle");
diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
index c1cf29798b99..f4e426d03b4a 100644
--- a/tools/bpf/bpftool/main.h
+++ b/tools/bpf/bpftool/main.h
@@ -187,7 +187,7 @@ int do_iter(int argc, char **argv) __weak;
 
 int parse_u32_arg(int *argc, char ***argv, __u32 *val, const char *what);
 int prog_parse_fd(int *argc, char ***argv);
-int prog_parse_fds(int *argc, char ***argv, int **fds);
+int prog_parse_fds(int *argc, char ***argv, int **fds, char **elf_filepath);
 int map_parse_fd(int *argc, char ***argv);
 int map_parse_fds(int *argc, char ***argv, int **fds);
 int map_parse_fd_and_info(int *argc, char ***argv, void *info, __u32 *info_len);
diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index cc48726740ad..04fa9a83ef7e 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -537,7 +537,7 @@ static int do_show_subset(int argc, char **argv)
 		p_err("mem alloc failed");
 		return -1;
 	}
-	nb_fds = prog_parse_fds(&argc, &argv, &fds);
+	nb_fds = prog_parse_fds(&argc, &argv, &fds, NULL);
 	if (nb_fds < 1)
 		goto exit_free;
 
@@ -787,7 +787,10 @@ prog_dump(struct bpf_prog_info *info, enum dump_mode mode,
 static int do_dump(int argc, char **argv)
 {
 	struct bpf_prog_info_linear *info_linear;
+	struct bpf_object *obj;
+	struct bpf_program *prog;
 	char *filepath = NULL;
+	char *elf_filepath = NULL;
 	bool opcodes = false;
 	bool visual = false;
 	enum dump_mode mode;
@@ -817,7 +820,8 @@ static int do_dump(int argc, char **argv)
 		p_err("mem alloc failed");
 		return -1;
 	}
-	nb_fds = prog_parse_fds(&argc, &argv, &fds);
+	elf_filepath = malloc(sizeof(char) * PATH_MAX);
+	nb_fds = prog_parse_fds(&argc, &argv, &fds, &elf_filepath);
 	if (nb_fds < 1)
 		goto exit_free;
 
@@ -849,7 +853,6 @@ static int do_dump(int argc, char **argv)
 		linum = true;
 		NEXT_ARG();
 	}
-
 	if (argc) {
 		usage();
 		goto exit_close;
@@ -866,9 +869,26 @@ static int do_dump(int argc, char **argv)
 	arrays |= 1UL << BPF_PROG_INFO_LINE_INFO;
 	arrays |= 1UL << BPF_PROG_INFO_JITED_LINE_INFO;
 
+	if (elf_filepath != NULL) {
+		obj = bpf_object__open(elf_filepath); 
+		if (libbpf_get_error(obj)) {
+			p_err("ERROR: opening BPF object file failed");
+			return 0;
+		}
+
+		bpf_object__for_each_program(prog, obj) {
+			struct bpf_prog_info pinfo;
+			pinfo.xlated_prog_insns = ptr_to_u64(bpf_program__insns(prog));
+			pinfo.xlated_prog_len = bpf_program__size(prog);
+			err = prog_dump(&pinfo, mode, filepath, opcodes, visual, linum);
+		}
+		return 0;
+	}
+
 	if (json_output && nb_fds > 1)
 		jsonw_start_array(json_wtr);	/* root array */
 	for (i = 0; i < nb_fds; i++) {
+		printf("uno\n");
 		info_linear = bpf_program__get_prog_info_linear(fds[i], arrays);
 		if (IS_ERR_OR_NULL(info_linear)) {
 			p_err("can't get prog info: %s", strerror(errno));
-- 
2.32.0

Re: [PATCH bpf-next 2/2] tools/bpf/bpftool: xlated dump from ELF file directly

[Lorenzo Fontana]

On 7/13/21 8:35 PM, Lorenzo Fontana wrote:

> +		printf("uno\n");
>  		info_linear = bpf_program__get_prog_info_linear(fds[i], arrays);
>  		if (IS_ERR_OR_NULL(info_linear)) {
>  			p_err("can't get prog info: %s", strerror(errno));
> 

This printf was not supposed to get here.
I'm sure I'll get feedback to do other changes so I'll revert that in the
next batch with the review changes.

Thanks Leo Di Donato for the finding!

Regards,

Lorenzo

RE: [PATCH bpf-next 2/2] tools/bpf/bpftool: xlated dump from ELF file directly

[John Fastabend]

Lorenzo Fontana wrote:
> bpftool can dump an xlated or jitted representation
> of the programs already loaded into the kernel.
> That capability is very useful for understanding what
> are the instructions the kernel will execute for that program.
> 
> However, sometimes the verifier does not load the program and
> one cannot use this feature until changes are made to make the
> verifier happy again.
> 
> This patch reuses the same dump function to dump the program
> from an ELF file directly instead of loading the instructions
> from a loaded file descriptor. In this way, the user
> can use all the bpftool features for "xlated" without loading.
> 
> In particular, the "visual" command is very useful when combined
> to this because the dot graph makes easy to spot bad instruction
> sequences.
> 
> Usage:
> 
>   bpftool prog dump xlated elf program.o
> 
> It also works with the other commands like 'visual' to print
> an dot representation of the program.
> 
>   bpftool prog dump xlated elf program.o visual
> 
> Signed-off-by: Lorenzo Fontana <fontanalorenz@gmail.com>

Seeing we need another spin anyways can we get documentation
updates as well in ./tools/bpf/bpftool/Documentation.

> ---
>  tools/bpf/bpftool/common.c | 15 ++++++++++++---
>  tools/bpf/bpftool/main.h   |  2 +-
>  tools/bpf/bpftool/prog.c   | 26 +++++++++++++++++++++++---
>  3 files changed, 36 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
> index 1828bba19020..b28d15505705 100644
> --- a/tools/bpf/bpftool/common.c
> +++ b/tools/bpf/bpftool/common.c
> @@ -703,7 +703,7 @@ static int prog_fd_by_nametag(void *nametag, int **fds, bool tag)
>  	return -1;
>  }
>  
> -int prog_parse_fds(int *argc, char ***argv, int **fds)
> +int prog_parse_fds(int *argc, char ***argv, int **fds, char **elf_filepath)
>  {
>  	if (is_prefix(**argv, "id")) {
>  		unsigned int id;
> @@ -763,9 +763,18 @@ int prog_parse_fds(int *argc, char ***argv, int **fds)
>  		if ((*fds)[0] < 0)
>  			return -1;
>  		return 1;
> +	} else if (is_prefix(**argv, "elf")) {
> +		NEXT_ARGP();
> +		if (!argc) {
> +			p_err("expected ELF file path");
> +			return -1;
> +		}
> +		*elf_filepath = **argv;
> +		NEXT_ARGP();
> +		return 1;
>  	}
>  
> -	p_err("expected 'id', 'tag', 'name' or 'pinned', got: '%s'?", **argv);
> +	p_err("expected 'id', 'tag', 'name', 'elf' or 'pinned', got: '%s'?", **argv);
>  	return -1;
>  }
>  
> @@ -779,7 +788,7 @@ int prog_parse_fd(int *argc, char ***argv)
>  		p_err("mem alloc failed");
>  		return -1;
>  	}
> -	nb_fds = prog_parse_fds(argc, argv, &fds);
> +	nb_fds = prog_parse_fds(argc, argv, &fds, NULL);
>  	if (nb_fds != 1) {
>  		if (nb_fds > 1) {
>  			p_err("several programs match this handle");
> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
> index c1cf29798b99..f4e426d03b4a 100644
> --- a/tools/bpf/bpftool/main.h
> +++ b/tools/bpf/bpftool/main.h
> @@ -187,7 +187,7 @@ int do_iter(int argc, char **argv) __weak;
>  
>  int parse_u32_arg(int *argc, char ***argv, __u32 *val, const char *what);
>  int prog_parse_fd(int *argc, char ***argv);
> -int prog_parse_fds(int *argc, char ***argv, int **fds);
> +int prog_parse_fds(int *argc, char ***argv, int **fds, char **elf_filepath);
>  int map_parse_fd(int *argc, char ***argv);
>  int map_parse_fds(int *argc, char ***argv, int **fds);
>  int map_parse_fd_and_info(int *argc, char ***argv, void *info, __u32 *info_len);
> diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> index cc48726740ad..04fa9a83ef7e 100644
> --- a/tools/bpf/bpftool/prog.c
> +++ b/tools/bpf/bpftool/prog.c
> @@ -537,7 +537,7 @@ static int do_show_subset(int argc, char **argv)
>  		p_err("mem alloc failed");
>  		return -1;
>  	}
> -	nb_fds = prog_parse_fds(&argc, &argv, &fds);
> +	nb_fds = prog_parse_fds(&argc, &argv, &fds, NULL);
>  	if (nb_fds < 1)
>  		goto exit_free;
>  
> @@ -787,7 +787,10 @@ prog_dump(struct bpf_prog_info *info, enum dump_mode mode,
>  static int do_dump(int argc, char **argv)
>  {
>  	struct bpf_prog_info_linear *info_linear;
> +	struct bpf_object *obj;
> +	struct bpf_program *prog;
>  	char *filepath = NULL;
> +	char *elf_filepath = NULL;
>  	bool opcodes = false;
>  	bool visual = false;
>  	enum dump_mode mode;
> @@ -817,7 +820,8 @@ static int do_dump(int argc, char **argv)
>  		p_err("mem alloc failed");
>  		return -1;
>  	}
> -	nb_fds = prog_parse_fds(&argc, &argv, &fds);
> +	elf_filepath = malloc(sizeof(char) * PATH_MAX);
> +	nb_fds = prog_parse_fds(&argc, &argv, &fds, &elf_filepath);
>  	if (nb_fds < 1)
>  		goto exit_free;
>  
> @@ -849,7 +853,6 @@ static int do_dump(int argc, char **argv)
>  		linum = true;
>  		NEXT_ARG();
>  	}
> -

nit...

No reason to delete the line here IMO. Just adds noise to the
patch.

>  	if (argc) {
>  		usage();
>  		goto exit_close;
> @@ -866,9 +869,26 @@ static int do_dump(int argc, char **argv)
>  	arrays |= 1UL << BPF_PROG_INFO_LINE_INFO;
>  	arrays |= 1UL << BPF_PROG_INFO_JITED_LINE_INFO;
>  
> +	if (elf_filepath != NULL) {
> +		obj = bpf_object__open(elf_filepath); 
> +		if (libbpf_get_error(obj)) {
> +			p_err("ERROR: opening BPF object file failed");
> +			return 0;
> +		}
> +
> +		bpf_object__for_each_program(prog, obj) {
> +			struct bpf_prog_info pinfo;
> +			pinfo.xlated_prog_insns = ptr_to_u64(bpf_program__insns(prog));
> +			pinfo.xlated_prog_len = bpf_program__size(prog);
> +			err = prog_dump(&pinfo, mode, filepath, opcodes, visual, linum);
> +		}
> +		return 0;

goto exit_close?

> +	}
> +
>  	if (json_output && nb_fds > 1)
>  		jsonw_start_array(json_wtr);	/* root array */
>  	for (i = 0; i < nb_fds; i++) {
> +		printf("uno\n");

As noted, remove.

>  		info_linear = bpf_program__get_prog_info_linear(fds[i], arrays);
>  		if (IS_ERR_OR_NULL(info_linear)) {
>  			p_err("can't get prog info: %s", strerror(errno));
> -- 
> 2.32.0
> 

Re: [PATCH bpf-next 2/2] tools/bpf/bpftool: xlated dump from ELF file directly

[liwei (GF)]

On 2021/7/14 2:35, Lorenzo Fontana wrote:
> bpftool can dump an xlated or jitted representation
> of the programs already loaded into the kernel.
> That capability is very useful for understanding what
> are the instructions the kernel will execute for that program.
> 
> However, sometimes the verifier does not load the program and
> one cannot use this feature until changes are made to make the
> verifier happy again.
> 
> This patch reuses the same dump function to dump the program
> from an ELF file directly instead of loading the instructions
> from a loaded file descriptor. In this way, the user
> can use all the bpftool features for "xlated" without loading.
> 
> In particular, the "visual" command is very useful when combined
> to this because the dot graph makes easy to spot bad instruction
> sequences.
> 
> Usage:
> 
>   bpftool prog dump xlated elf program.o
> 
> It also works with the other commands like 'visual' to print
> an dot representation of the program.
> 
>   bpftool prog dump xlated elf program.o visual
> 
> Signed-off-by: Lorenzo Fontana <fontanalorenz@gmail.com>
> ---
>  tools/bpf/bpftool/common.c | 15 ++++++++++++---
>  tools/bpf/bpftool/main.h   |  2 +-
>  tools/bpf/bpftool/prog.c   | 26 +++++++++++++++++++++++---
>  3 files changed, 36 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
> index 1828bba19020..b28d15505705 100644
> --- a/tools/bpf/bpftool/common.c
> +++ b/tools/bpf/bpftool/common.c
> @@ -703,7 +703,7 @@ static int prog_fd_by_nametag(void *nametag, int **fds, bool tag)
>  	return -1;
>  }
>  
> -int prog_parse_fds(int *argc, char ***argv, int **fds)
> +int prog_parse_fds(int *argc, char ***argv, int **fds, char **elf_filepath)
>  {
>  	if (is_prefix(**argv, "id")) {
>  		unsigned int id;
> @@ -763,9 +763,18 @@ int prog_parse_fds(int *argc, char ***argv, int **fds)
>  		if ((*fds)[0] < 0)
>  			return -1;
>  		return 1;
> +	} else if (is_prefix(**argv, "elf")) {
> +		NEXT_ARGP();
> +		if (!argc) {
> +			p_err("expected ELF file path");
> +			return -1;
> +		}
> +		*elf_filepath = **argv;
> +		NEXT_ARGP();
> +		return 1;
>  	}
>  
> -	p_err("expected 'id', 'tag', 'name' or 'pinned', got: '%s'?", **argv);
> +	p_err("expected 'id', 'tag', 'name', 'elf' or 'pinned', got: '%s'?", **argv);

Should we update the 'HELP_SPEC_PROGRAM' info as well?

Thanks,
Wei

>  	return -1;
>  }
>  
> @@ -779,7 +788,7 @@ int prog_parse_fd(int *argc, char ***argv)
>  		p_err("mem alloc failed");
>  		return -1;
>  	}
> -	nb_fds = prog_parse_fds(argc, argv, &fds);
> +	nb_fds = prog_parse_fds(argc, argv, &fds, NULL);
>  	if (nb_fds != 1) {
>  		if (nb_fds > 1) {
>  			p_err("several programs match this handle");
> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
> index c1cf29798b99..f4e426d03b4a 100644
> --- a/tools/bpf/bpftool/main.h
> +++ b/tools/bpf/bpftool/main.h
> @@ -187,7 +187,7 @@ int do_iter(int argc, char **argv) __weak;
>  
>  int parse_u32_arg(int *argc, char ***argv, __u32 *val, const char *what);
>  int prog_parse_fd(int *argc, char ***argv);
> -int prog_parse_fds(int *argc, char ***argv, int **fds);
> +int prog_parse_fds(int *argc, char ***argv, int **fds, char **elf_filepath);
>  int map_parse_fd(int *argc, char ***argv);
>  int map_parse_fds(int *argc, char ***argv, int **fds);
>  int map_parse_fd_and_info(int *argc, char ***argv, void *info, __u32 *info_len);
> diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> index cc48726740ad..04fa9a83ef7e 100644
> --- a/tools/bpf/bpftool/prog.c
> +++ b/tools/bpf/bpftool/prog.c
> @@ -537,7 +537,7 @@ static int do_show_subset(int argc, char **argv)
>  		p_err("mem alloc failed");
>  		return -1;
>  	}
> -	nb_fds = prog_parse_fds(&argc, &argv, &fds);
> +	nb_fds = prog_parse_fds(&argc, &argv, &fds, NULL);
>  	if (nb_fds < 1)
>  		goto exit_free;
>  
> @@ -787,7 +787,10 @@ prog_dump(struct bpf_prog_info *info, enum dump_mode mode,
>  static int do_dump(int argc, char **argv)
>  {
>  	struct bpf_prog_info_linear *info_linear;
> +	struct bpf_object *obj;
> +	struct bpf_program *prog;
>  	char *filepath = NULL;
> +	char *elf_filepath = NULL;
>  	bool opcodes = false;
>  	bool visual = false;
>  	enum dump_mode mode;
> @@ -817,7 +820,8 @@ static int do_dump(int argc, char **argv)
>  		p_err("mem alloc failed");
>  		return -1;
>  	}
> -	nb_fds = prog_parse_fds(&argc, &argv, &fds);
> +	elf_filepath = malloc(sizeof(char) * PATH_MAX);
> +	nb_fds = prog_parse_fds(&argc, &argv, &fds, &elf_filepath);
>  	if (nb_fds < 1)
>  		goto exit_free;
>  
> @@ -849,7 +853,6 @@ static int do_dump(int argc, char **argv)
>  		linum = true;
>  		NEXT_ARG();
>  	}
> -
>  	if (argc) {
>  		usage();
>  		goto exit_close;
> @@ -866,9 +869,26 @@ static int do_dump(int argc, char **argv)
>  	arrays |= 1UL << BPF_PROG_INFO_LINE_INFO;
>  	arrays |= 1UL << BPF_PROG_INFO_JITED_LINE_INFO;
>  
> +	if (elf_filepath != NULL) {
> +		obj = bpf_object__open(elf_filepath); 
> +		if (libbpf_get_error(obj)) {
> +			p_err("ERROR: opening BPF object file failed");
> +			return 0;
> +		}
> +
> +		bpf_object__for_each_program(prog, obj) {
> +			struct bpf_prog_info pinfo;
> +			pinfo.xlated_prog_insns = ptr_to_u64(bpf_program__insns(prog));
> +			pinfo.xlated_prog_len = bpf_program__size(prog);
> +			err = prog_dump(&pinfo, mode, filepath, opcodes, visual, linum);
> +		}
> +		return 0;
> +	}
> +
>  	if (json_output && nb_fds > 1)
>  		jsonw_start_array(json_wtr);	/* root array */
>  	for (i = 0; i < nb_fds; i++) {
> +		printf("uno\n");
>  		info_linear = bpf_program__get_prog_info_linear(fds[i], arrays);
>  		if (IS_ERR_OR_NULL(info_linear)) {
>  			p_err("can't get prog info: %s", strerror(errno));
> 

Re: [PATCH bpf-next 2/2] tools/bpf/bpftool: xlated dump from ELF file directly

[Quentin Monnet]

2021-07-13 20:35 UTC+0200 ~ Lorenzo Fontana <fontanalorenz@gmail.com>
> bpftool can dump an xlated or jitted representation
> of the programs already loaded into the kernel.
> That capability is very useful for understanding what
> are the instructions the kernel will execute for that program.
> 
> However, sometimes the verifier does not load the program and
> one cannot use this feature until changes are made to make the
> verifier happy again.
> 
> This patch reuses the same dump function to dump the program
> from an ELF file directly instead of loading the instructions
> from a loaded file descriptor. In this way, the user
> can use all the bpftool features for "xlated" without loading.
> 
> In particular, the "visual" command is very useful when combined
> to this because the dot graph makes easy to spot bad instruction
> sequences.
> 
> Usage:
> 
>   bpftool prog dump xlated elf program.o

Hi Lorenzo,

"elf" is not a bad keyword, but seeing that we use "file" for dumping
BTF from ELF object files with "bpftool btf dump file foo.o", I'd rather
have the same keyword here, for consistency.

> 
> It also works with the other commands like 'visual' to print
> an dot representation of the program.
> 
>   bpftool prog dump xlated elf program.o visual
> 
> Signed-off-by: Lorenzo Fontana <fontanalorenz@gmail.com>
> ---
>  tools/bpf/bpftool/common.c | 15 ++++++++++++---
>  tools/bpf/bpftool/main.h   |  2 +-
>  tools/bpf/bpftool/prog.c   | 26 +++++++++++++++++++++++---
>  3 files changed, 36 insertions(+), 7 deletions(-)
> 

> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
> index c1cf29798b99..f4e426d03b4a 100644
> --- a/tools/bpf/bpftool/main.h
> +++ b/tools/bpf/bpftool/main.h

> @@ -787,7 +787,10 @@ prog_dump(struct bpf_prog_info *info, enum dump_mode mode,
>  static int do_dump(int argc, char **argv)
>  {
>  	struct bpf_prog_info_linear *info_linear;
> +	struct bpf_object *obj;
> +	struct bpf_program *prog;
>  	char *filepath = NULL;
> +	char *elf_filepath = NULL;
>  	bool opcodes = false;
>  	bool visual = false;
>  	enum dump_mode mode;

Nit: Could you preserve the reverse-Christmas-tree order for the
declarations?

> @@ -817,7 +820,8 @@ static int do_dump(int argc, char **argv)
>  		p_err("mem alloc failed");
>  		return -1;
>  	}
> -	nb_fds = prog_parse_fds(&argc, &argv, &fds);
> +	elf_filepath = malloc(sizeof(char) * PATH_MAX);
> +	nb_fds = prog_parse_fds(&argc, &argv, &fds, &elf_filepath);
>  	if (nb_fds < 1)
>  		goto exit_free;
>  
> @@ -849,7 +853,6 @@ static int do_dump(int argc, char **argv)
>  		linum = true;
>  		NEXT_ARG();
>  	}
> -
>  	if (argc) {
>  		usage();
>  		goto exit_close;
> @@ -866,9 +869,26 @@ static int do_dump(int argc, char **argv)
>  	arrays |= 1UL << BPF_PROG_INFO_LINE_INFO;
>  	arrays |= 1UL << BPF_PROG_INFO_JITED_LINE_INFO;
>  
> +	if (elf_filepath != NULL) {

It would normally be just "if (elf_filepath) {". Checkpatch complains
about it, by the way.

But you don't want to enter here just if elf_filepath is non-NULL,
because you always malloc it (whether the "elf" keyword was passed or
not), so your pointer is always non-NULL if there's no allocation error.
This means that you always enter the block, and it breaks the command
for dumping instructions for loaded programs. You need another check.

Also before this block, we may also want to error out with a better
message error if the user attempts to dump "jited" instructions from an
ELF file? Right now bpftool simply answers "no instructions returned",
which is not very indicative of why it fails.

And since the block does not use the "arrays" variable necessary for
bpf_program__get_prog_info_linear(), it could be moved a bit higher,
right after the argument parsing.

> +		obj = bpf_object__open(elf_filepath); 
> +		if (libbpf_get_error(obj)) {
> +			p_err("ERROR: opening BPF object file failed");
> +			return 0;
> +		}
> +
> +		bpf_object__for_each_program(prog, obj) {
> +			struct bpf_prog_info pinfo;

Checkpatch complains about a missing blank line here after the
declaration, and about a few other things, please make sure to address it.

In addition to the documentation (.../Documentation/bpftool-prog.rst)
and the help message (although we don't want to change HELP_SPEC_PROGRAM
directly, as it is used in many context where the new handle does not
apply) as reported by John and Wei, can you please update the bash
completion (.../bash-completion/bpftool)?

The patch is a nice addition to bpftool, thanks for this work!
Quentin

Re: [PATCH bpf-next 1/2] tools/lib/bpf: bpf_program__insns allow to retrieve insns in libbpf

[kernel test robot]

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

Hi Lorenzo,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on bpf-next/master]

url:    https://github.com/0day-ci/linux/commits/Lorenzo-Fontana/tools-lib-bpf-bpf_program__insns-allow-to-retrieve-insns-in-libbpf/20210714-023628
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: x86_64-rhel-8.3-kselftests (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/999d9928ec3dd02a08da121d1377392681c84e51
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Lorenzo-Fontana/tools-lib-bpf-bpf_program__insns-allow-to-retrieve-insns-in-libbpf/20210714-023628
        git checkout 999d9928ec3dd02a08da121d1377392681c84e51
        # save the attached .config to linux build tree
        make W=1 ARCH=x86_64 

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 >>):

   Warning: Kernel ABI header at 'tools/include/uapi/linux/bpf.h' differs from latest version at 'include/uapi/linux/bpf.h'
   Warning: Kernel ABI header at 'tools/include/uapi/linux/netlink.h' differs from latest version at 'include/uapi/linux/netlink.h'
   Warning: Kernel ABI header at 'tools/include/uapi/linux/if_link.h' differs from latest version at 'include/uapi/linux/if_link.h'
   In file included from netlink.c:9:
   usr/include/linux/if_ether.h:169:1: warning: packed attribute is unnecessary for 'ethhdr' [-Wpacked]
     169 | } __attribute__((packed));
         | ^
   In file included from tools/include/uapi/linux/ethtool.h:19,
                    from xsk.c:18:
   usr/include/linux/if_ether.h:169:1: warning: packed attribute is unnecessary for 'ethhdr' [-Wpacked]
     169 | } __attribute__((packed));
         | ^
   In file included from tools/include/uapi/linux/ethtool.h:19,
                    from xsk.c:18:
   usr/include/linux/if_ether.h:169:1: warning: packed attribute is unnecessary for 'ethhdr' [-Wpacked]
     169 | } __attribute__((packed));
         | ^
   In file included from netlink.c:9:
   usr/include/linux/if_ether.h:169:1: warning: packed attribute is unnecessary for 'ethhdr' [-Wpacked]
     169 | } __attribute__((packed));
         | ^
>> Warning: Num of global symbols in /tools/build/libbpf/sharedobjs/libbpf-in.o (319) does NOT match with num of versioned symbols in /tools/build/libbpf/libbpf.so (318). Please make sure all LIBBPF_API symbols are versioned in libbpf.map.
   make[2]: *** [Makefile:186: check_abi] Error 1
   make[2]: Target 'all_cmd' not remade because of errors.

---
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: 42002 bytes --]

RE: [PATCH bpf-next 1/2] tools/lib/bpf: bpf_program__insns allow to retrieve insns in libbpf

[John Fastabend]

Lorenzo Fontana wrote:
> This allows consumers of libbpf to iterate trough the insns
> of a program without loading it first directly after the ELF parsing.
> 
> Being able to do that is useful to create tooling that can show
> the structure of a BPF program using libbpf without having to
> parse the ELF separately.
> 
> Usage:
>   struct bpf_insn *insn;
>   insn = bpf_program__insns(prog);
> 
> Signed-off-by: Lorenzo Fontana <fontanalorenz@gmail.com>
> ---

Seems reasonable to me. Couple comments on the 2/2 patch.

Acked-by: John Fastabend <john.fastabend@gmail.com>

Re: [PATCH bpf-next 1/2] tools/lib/bpf: bpf_program__insns allow to retrieve insns in libbpf

[Quentin Monnet]

2021-07-13 20:33 UTC+0200 ~ Lorenzo Fontana <fontanalorenz@gmail.com>
> This allows consumers of libbpf to iterate trough the insns
> of a program without loading it first directly after the ELF parsing.
> 
> Being able to do that is useful to create tooling that can show
> the structure of a BPF program using libbpf without having to
> parse the ELF separately.
> 
> Usage:
>   struct bpf_insn *insn;
>   insn = bpf_program__insns(prog);
> 
> Signed-off-by: Lorenzo Fontana <fontanalorenz@gmail.com>
> ---
>  tools/lib/bpf/libbpf.c | 5 +++++
>  tools/lib/bpf/libbpf.h | 1 +
>  2 files changed, 6 insertions(+)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 1e04ce724240..67d51531f6b6 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -8866,6 +8866,11 @@ void *bpf_program__priv(const struct bpf_program *prog)
>  	return prog ? prog->priv : libbpf_err_ptr(-EINVAL);
>  }
>  
> +struct bpf_insn *bpf_program__insns(const struct bpf_program *prog)
> +{
> +	return prog ? prog->insns : libbpf_err_ptr(-EINVAL);
> +}
> +
>  void bpf_program__set_ifindex(struct bpf_program *prog, __u32 ifindex)
>  {
>  	prog->prog_ifindex = ifindex;
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index 6e61342ba56c..e4a1c98ae6d9 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -195,6 +195,7 @@ typedef void (*bpf_program_clear_priv_t)(struct bpf_program *, void *);
>  LIBBPF_API int bpf_program__set_priv(struct bpf_program *prog, void *priv,
>  				     bpf_program_clear_priv_t clear_priv);
>  
> +LIBBPF_API struct bpf_insn *bpf_program__insns(const struct bpf_program *prog);
>  LIBBPF_API void *bpf_program__priv(const struct bpf_program *prog);
>  LIBBPF_API void bpf_program__set_ifindex(struct bpf_program *prog,
>  					 __u32 ifindex);
> 

If you make it part of the API, the new function should also have an
entry in tools/lib/bpf/libbpf.map.

Quentin

Re: [PATCH bpf-next 1/2] tools/lib/bpf: bpf_program__insns allow to retrieve insns in libbpf

[Andrii Nakryiko]

On Tue, Jul 13, 2021 at 11:34 AM Lorenzo Fontana
<fontanalorenz@gmail.com> wrote:
>
> This allows consumers of libbpf to iterate trough the insns
> of a program without loading it first directly after the ELF parsing.
>
> Being able to do that is useful to create tooling that can show
> the structure of a BPF program using libbpf without having to
> parse the ELF separately.
>

So I wonder how useful is getting raw BPF instructions before libbpf
processed them and resolved map references, subprogram calls, etc?
You'll have lots of zeroes or meaningless constants in ldimm64
instructions, etc. I always felt that being able to get instructions
after libbpf processed them is more useful. The problem is that
currently libbpf frees prog->insns after successful bpf_program__load.
There is one extra (advanced) scenario where having those instructions
preserved after load would be really nice -- cloning BPF program (I
had use case for fentry/fexit). So the question is whether we should
just leave those prog->insns around until the object is closed or not?
And if we do, should bpftool dump instructions before or after load?
Let's see what folks think.

> Usage:
>   struct bpf_insn *insn;
>   insn = bpf_program__insns(prog);
>
> Signed-off-by: Lorenzo Fontana <fontanalorenz@gmail.com>
> ---
>  tools/lib/bpf/libbpf.c | 5 +++++
>  tools/lib/bpf/libbpf.h | 1 +
>  2 files changed, 6 insertions(+)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 1e04ce724240..67d51531f6b6 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -8866,6 +8866,11 @@ void *bpf_program__priv(const struct bpf_program *prog)
>         return prog ? prog->priv : libbpf_err_ptr(-EINVAL);
>  }
>
> +struct bpf_insn *bpf_program__insns(const struct bpf_program *prog)

Definitely needs to be const, we don't want anyone accidentally to
modify it (though it's C and they can always force they way, but
that's a separate issue)

> +{
> +       return prog ? prog->insns : libbpf_err_ptr(-EINVAL);
> +}
> +
>  void bpf_program__set_ifindex(struct bpf_program *prog, __u32 ifindex)
>  {
>         prog->prog_ifindex = ifindex;
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index 6e61342ba56c..e4a1c98ae6d9 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -195,6 +195,7 @@ typedef void (*bpf_program_clear_priv_t)(struct bpf_program *, void *);
>  LIBBPF_API int bpf_program__set_priv(struct bpf_program *prog, void *priv,
>                                      bpf_program_clear_priv_t clear_priv);
>
> +LIBBPF_API struct bpf_insn *bpf_program__insns(const struct bpf_program *prog);

BTW, I find bpf_program__size() is very ambiguous (is it number of
bytes or number of instructions)? I'd also add bpf_program__insn_cnt()
in the same patch.

And as Quentin mentioned, you need to update libbpf.map (and please
keep everything alphabetically sorted).

>  LIBBPF_API void *bpf_program__priv(const struct bpf_program *prog);
>  LIBBPF_API void bpf_program__set_ifindex(struct bpf_program *prog,
>                                          __u32 ifindex);
> --
> 2.32.0
>

Re: [PATCH bpf-next 1/2] tools/lib/bpf: bpf_program__insns allow to retrieve insns in libbpf

[Alexei Starovoitov]

On Thu, Jul 15, 2021 at 2:40 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Jul 13, 2021 at 11:34 AM Lorenzo Fontana
> <fontanalorenz@gmail.com> wrote:
> >
> > This allows consumers of libbpf to iterate trough the insns
> > of a program without loading it first directly after the ELF parsing.
> >
> > Being able to do that is useful to create tooling that can show
> > the structure of a BPF program using libbpf without having to
> > parse the ELF separately.
> >
>
> So I wonder how useful is getting raw BPF instructions before libbpf
> processed them and resolved map references, subprogram calls, etc?
> You'll have lots of zeroes or meaningless constants in ldimm64
> instructions, etc. I always felt that being able to get instructions
> after libbpf processed them is more useful. The problem is that
> currently libbpf frees prog->insns after successful bpf_program__load.
> There is one extra (advanced) scenario where having those instructions
> preserved after load would be really nice -- cloning BPF program (I
> had use case for fentry/fexit). So the question is whether we should
> just leave those prog->insns around until the object is closed or not?
> And if we do, should bpftool dump instructions before or after load?
> Let's see what folks think.

Same here. I understand the desire, but the approach to expose half baked
instructions isn't addressing the need.

Re: [PATCH bpf-next 1/2] tools/lib/bpf: bpf_program__insns allow to retrieve insns in libbpf

[Lorenzo Fontana]

On 7/16/21 3:51 AM, Alexei Starovoitov wrote:
> On Thu, Jul 15, 2021 at 2:40 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
>> On Tue, Jul 13, 2021 at 11:34 AM Lorenzo Fontana
>> <fontanalorenz@gmail.com> wrote:
>>> This allows consumers of libbpf to iterate trough the insns
>>> of a program without loading it first directly after the ELF parsing.
>>>
>>> Being able to do that is useful to create tooling that can show
>>> the structure of a BPF program using libbpf without having to
>>> parse the ELF separately.
>>>
>> So I wonder how useful is getting raw BPF instructions before libbpf
>> processed them and resolved map references, subprogram calls, etc?
>> You'll have lots of zeroes or meaningless constants in ldimm64
>> instructions, etc. I always felt that being able to get instructions
>> after libbpf processed them is more useful. The problem is that
>> currently libbpf frees prog->insns after successful bpf_program__load.
>> There is one extra (advanced) scenario where having those instructions
>> preserved after load would be really nice -- cloning BPF program (I
>> had use case for fentry/fexit). So the question is whether we should
>> just leave those prog->insns around until the object is closed or not?
>> And if we do, should bpftool dump instructions before or after load?
>> Let's see what folks think.
> Same here. I understand the desire, but the approach to expose half baked
> instructions isn't addressing the need.

Thanks taking the time to go trough this and understanding the use case.
You all certainly know the scope of this better than I do.
I'll study a bit more to understand how that can be achieved and try to send another patch if I find a solution.