netdev - Re: [PATCH bpf-next 7/7] bpf: libbpf: bpftool: Print bpf_line

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20181209171859.7cd367a8@cakuba.netronome.com>
Date:   Sun, 9 Dec 2018 17:18:59 -0800
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Martin KaFai Lau <kafai@...com>
Cc:     <netdev@...r.kernel.org>, Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>, <kernel-team@...com>
Subject: Re: [PATCH bpf-next 7/7] bpf: libbpf: bpftool: Print bpf_line_info
 during prog dump

On Fri, 7 Dec 2018 16:42:32 -0800, Martin KaFai Lau wrote:

> diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> index 5524b6dccd85..7c30731a9b73 100644
> --- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> +++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> @@ -22,8 +22,8 @@ MAP COMMANDS
>  =============
>  
>  |	**bpftool** **prog { show | list }** [*PROG*]
> -|	**bpftool** **prog dump xlated** *PROG* [{**file** *FILE* | **opcodes** | **visual**}]
> -|	**bpftool** **prog dump jited**  *PROG* [{**file** *FILE* | **opcodes**}]
> +|	**bpftool** **prog dump xlated** *PROG* [{**file** *FILE* | **opcodes** | **visual** | **linum**}]
> +|	**bpftool** **prog dump jited**  *PROG* [{**file** *FILE* | **opcodes** | **linum**}]
>  |	**bpftool** **prog pin** *PROG* *FILE*
>  |	**bpftool** **prog { load | loadall }** *OBJ* *PATH* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
>  |	**bpftool** **prog attach** *PROG* *ATTACH_TYPE* [*MAP*]
> @@ -56,7 +56,7 @@ 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** | **visual** }]
> +	**bpftool prog dump xlated** *PROG* [{ **file** *FILE* | **opcodes** | **visual** | **linum** }]
>  		  Dump eBPF instructions of the program from the kernel. By
>  		  default, eBPF will be disassembled and printed to standard
>  		  output in human-readable format. In this case, **opcodes**
> @@ -69,13 +69,21 @@ DESCRIPTION
>  		  built instead, and eBPF instructions will be presented with
>  		  CFG in DOT format, on standard output.
>  
> -	**bpftool prog dump jited**  *PROG* [{ **file** *FILE* | **opcodes** }]
> +		  If the prog has line_info available, the source line will
> +		  be displayed by default.  If **linum** is specified,
> +		  the filename, line number and line column will also be
> +		  displayed on top of the source line.
> +	**bpftool prog dump jited**  *PROG* [{ **file** *FILE* | **opcodes** | **linum** }]

why eat the new line?

>  		  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.
>  
>  		  **opcodes** controls if raw opcodes will be printed.
>  
> +		  If the prog has line_info available, the source line will
> +		  be displayed by default.  If **linum** is specified,
> +		  the filename, line number and line column will also be
> +		  displayed on top of the source line.
>  	**bpftool prog pin** *PROG* *FILE*

why eat the new line?

>  		  Pin program *PROG* as *FILE*.
>  

> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
> index 0be0dd8f467f..d9393abdba78 100644
> --- a/tools/bpf/bpftool/main.h
> +++ b/tools/bpf/bpftool/main.h
> @@ -138,6 +138,9 @@ struct pinned_obj {
>  	struct hlist_node hash;
>  };
>  
> +struct btf;
> +struct bpf_line_info;
> +
>  int build_pinned_obj_table(struct pinned_obj_table *table,
>  			   enum bpf_obj_type type);
>  void delete_pinned_obj_table(struct pinned_obj_table *tab);
> @@ -175,13 +178,23 @@ int map_parse_fd(int *argc, char ***argv);
>  int map_parse_fd_and_info(int *argc, char ***argv, void *info, __u32 *info_len);
>  
>  #ifdef HAVE_LIBBFD_SUPPORT
> +struct bpf_prog_linfo;
>  void disasm_print_insn(unsigned char *image, ssize_t len, int opcodes,
> -		       const char *arch, const char *disassembler_options);
> +		       const char *arch, const char *disassembler_options,
> +		       const struct btf *btf,
> +		       const struct bpf_prog_linfo *prog_linfo,
> +		       __u64 func_ksym, unsigned int func_idx,
> +		       bool linum);
>  int disasm_init(void);
>  #else
>  static inline
>  void disasm_print_insn(unsigned char *image, ssize_t len, int opcodes,
> -		       const char *arch, const char *disassembler_options)
> +		       const char *arch, const char *disassembler_options,
> +		       const struct btf *btf,
> +		       const struct bpf_prog_linfo *prog_linfo,
> +		       __u64 func_ksym, unsigned int func_idx,
> +		       bool linum)
> +

extra new line

>  {
>  }
>  static inline int disasm_init(void)
> @@ -554,6 +585,13 @@ static int do_dump(int argc, char **argv)
>  	info.func_info_cnt = finfo_cnt;
>  	info.func_info_rec_size = finfo_rec_size;
>  	info.func_info = ptr_to_u64(func_info);
> +	info.line_info_cnt = linfo_cnt;
> +	info.line_info_rec_size = linfo_rec_size;
> +	info.line_info = ptr_to_u64(linfo);
> +	info.jited_line_info_cnt = jited_linfo_cnt;
> +	info.jited_line_info_rec_size = jited_linfo_rec_size;
> +	info.jited_line_info = ptr_to_u64(jited_linfo);
> +
>  

extra new line

>  	err = bpf_obj_get_info_by_fd(fd, &info, &len);
>  	close(fd);
> @@ -596,6 +634,30 @@ static int do_dump(int argc, char **argv)
>  		finfo_cnt = 0;
>  	}
>  
> +	if (linfo && info.line_info_cnt != linfo_cnt) {
> +		p_err("incorrect line_info_cnt %u vs. expected %u",
> +		      info.line_info_cnt, linfo_cnt);
> +		goto err_free;
> +	}
> +
> +	if (info.line_info_rec_size != linfo_rec_size) {
> +		p_err("incorrect line_info_rec_size %u vs. expected %u",
> +		      info.line_info_rec_size, linfo_rec_size);
> +		goto err_free;
> +	}
> +
> +	if (jited_linfo && info.jited_line_info_cnt != jited_linfo_cnt) {
> +		p_err("incorrect jited_line_info_cnt %u vs. expected %u",
> +		      info.jited_line_info_cnt, jited_linfo_cnt);
> +		goto err_free;
> +	}
> +
> +	if (info.jited_line_info_rec_size != jited_linfo_rec_size) {
> +		p_err("incorrect jited_line_info_rec_size %u vs. expected %u",
> +		      info.jited_line_info_rec_size, jited_linfo_rec_size);
> +		goto err_free;
> +	}
> +
>  	if ((member_len == &info.jited_prog_len &&
>  	     info.jited_prog_insns == 0) ||
>  	    (member_len == &info.xlated_prog_len &&
> @@ -609,6 +671,12 @@ static int do_dump(int argc, char **argv)
>  		goto err_free;
>  	}
>  
> +	if (linfo_cnt) {
> +		prog_linfo = bpf_prog_linfo__new(&info);
> +		if (!prog_linfo)
> +			p_err("error in processing bpf_line_info.  continue without it.");

double p_err() will cause trouble in JSON output, p_info() is better in
such cases, but what kind of errors are expected here that we'd want to
continue with?

> +	}
> +
>  	if (filepath) {
>  		fd = open(filepath, O_WRONLY | O_CREAT | O_TRUNC, 0600);
>  		if (fd < 0) {

Please fix, thanks!