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

[Quentin Monnet <quentin@isovalent.com>]

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