Re: [PATCH] bpftool: fix line info in multi-function CFG dumps

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] bpftool: function-relative bpf_func_info offset in dump_xlated_for_graph()
--

commit 880c182e492fbe51e0cdd7cbcdc7fcfdb2fe9c94
Author: Eduard Zateev <hackerowskiy@gmail.com>

bpftool: fix line info in multi-function CFG dumps

This commit passes the program start pointer into the CFG graph dumper so that
BTF line-info lookups and displayed instruction indices use the global
instruction offset rather than a function-relative offset, while keeping
function-info matching relative to each function. It also adds a BPF selftest
for multi-function visual CFG output.

Link: https://github.com/libbpf/bpftool/issues/178

> diff --git a/tools/bpf/bpftool/cfg.c b/tools/bpf/bpftool/cfg.c
> index e3785f9a697de..65ec45469743b 100644
> --- a/tools/bpf/bpftool/cfg.c
> +++ b/tools/bpf/bpftool/cfg.c
> @@ -383,7 +383,7 @@ static void cfg_destroy(struct cfg *cfg)
>  
>  static void
>  draw_bb_node(struct func_node *func, struct bb_node *bb, struct dump_data *dd,
> -	     bool opcodes, bool linum)
> +	     struct bpf_insn *prog_start, bool opcodes, bool linum)
>  {
>  	const char *shape;
>  
> @@ -400,11 +400,11 @@ draw_bb_node(struct func_node *func, struct bb_node *bb, struct dump_data *dd,
>  	} else if (bb->idx == EXIT_BLOCK_INDEX) {
>  		printf("EXIT");
>  	} else {
> -		unsigned int start_idx;
> +		unsigned int start_idx = bb->head - func->start;
> +
>  		printf("{\\\n");
> -		start_idx = bb->head - func->start;
>  		dump_xlated_for_graph(dd, bb->head, bb->tail, start_idx,
> -				      opcodes, linum);
> +				      prog_start, opcodes, linum);
>  		printf("}");
>  	}
>  

[ ... ]

> diff --git a/tools/bpf/bpftool/xlated_dumper.c b/tools/bpf/bpftool/xlated_dumper.c
> index 5579173a61e3b..847241bbe91e2 100644
> --- a/tools/bpf/bpftool/xlated_dumper.c
> +++ b/tools/bpf/bpftool/xlated_dumper.c
> @@ -353,7 +353,7 @@ void dump_xlated_plain(struct dump_data *dd, void *buf, unsigned int len,
>  }
>  
>  void dump_xlated_for_graph(struct dump_data *dd, void *buf_start, void *buf_end,
> -			   unsigned int start_idx,
> +			   unsigned int start_idx, struct bpf_insn *prog_start,
>  			   bool opcodes, bool linum)
>  {
>  	const struct bpf_insn_cbs cbs = {
> @@ -374,6 +374,7 @@ void dump_xlated_for_graph(struct dump_data *dd, void *buf_start, void *buf_end,
>  
>  	for (; cur <= insn_end; cur++) {
>  		unsigned int insn_off;
> +		unsigned int linfo_offset;
>  
>  		if (double_insn) {
>  			double_insn = false;
> @@ -381,7 +382,8 @@ void dump_xlated_for_graph(struct dump_data *dd, void *buf_start, void *buf_end,
>  		}
>  		double_insn = cur->code == (BPF_LD | BPF_IMM | BPF_DW);
>  
> -		insn_off = (unsigned int)(cur - insn_start + start_idx);
> +		insn_off = (unsigned int)(cur - insn_start) + start_idx;
> +		linfo_offset = (unsigned int)(cur - prog_start);
>  		if (btf && record) {
>  			if (record->insn_off == insn_off) {

[Severity: Medium]
Should struct bpf_func_info matching in dump_xlated_for_graph() also use the
global instruction offset from the start of the program rather than the
function-relative insn_off?

In the kernel verifier and in dump_xlated_plain(), record->insn_off is the
global instruction offset from the start of the whole program (0 for the main
program, and the subprogram start offset for each subprogram).

In dump_xlated_for_graph(), record is initialized to dd->func_info at the
start of each basic block:

    struct bpf_func_info *record = dd->func_info;
    ...
        insn_off = (unsigned int)(cur - insn_start) + start_idx;
        linfo_offset = (unsigned int)(cur - prog_start);
        if (btf && record) {
            if (record->insn_off == insn_off) {
                btf_dumper_type_only(btf, record->type_id,
                                     func_sig,
                                     sizeof(func_sig));
                if (func_sig[0] != '\0')
                    printf("; %s:\\l\\\n", func_sig);
                record = (void *)record + dd->finfo_rec_size;
            }
        }

Because draw_bb_node() computes start_idx as bb->head - func->start, every
subprogram's entry basic block starts at insn_off == 0. When
dump_xlated_cfg() -> cfg_dump() -> func_output_bb_def() -> draw_bb_node() ->
dump_xlated_for_graph() runs on the second function (cluster_1), insn_off == 0
matches func_info[0] (record->insn_off == 0) and prints the first function's
prototype at the entry of the second function.

Then record advances to func_info[1], whose record->insn_off is the global
instruction offset of the second function (for example, 2 in bpftool_cfg.c).
When the loop reaches function-relative insn_off == 2 inside the second
function's entry block (global instruction 4), record->insn_off == insn_off
matches mid-block and emits the second function's prototype before instruction
4 instead of instruction 2 (or omits it if the entry basic block is shorter
than the global offset).

If draw_bb_node() passes start_idx as bb->head - prog_start (or if
dump_xlated_for_graph() computes the global offset from prog_start and walks
dd->func_info up to that offset), wouldn't that fix the function prototype
annotations in cluster_1 as well and avoid needing both start_idx and
prog_start?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261024062329.264086-1-hackerowskiy@gmail.com?part=1