Re: [RFC bpf-next v1 4/8] selftests/bpf: extract utility function for BPF disassembly

Andrii Nakryiko
2024-07-02T00:42:04Z

On Sat, Jun 29, 2024 at 2:48 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> uint32_t disasm_insn(struct bpf_insn *insn, char *buf, size_t buf_sz);

or you can return `struct bpf_insn *` which will point to the next
hypothetical instruction?

>
> Disassembles instruction 'insn' to a text buffer 'buf'.
> Removes insn->code hex prefix added by kernel disassemly routine.
> Returns the length of decoded instruction (either 1 or 2).
>
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
> tools/testing/selftests/bpf/Makefile | 1 +
> tools/testing/selftests/bpf/disasm_helpers.c | 50 +++++++++++++
> tools/testing/selftests/bpf/disasm_helpers.h | 12 ++++
> .../selftests/bpf/prog_tests/ctx_rewrite.c | 71 +++----------------
> tools/testing/selftests/bpf/testing_helpers.c | 1 +
> 5 files changed, 72 insertions(+), 63 deletions(-)
> create mode 100644 tools/testing/selftests/bpf/disasm_helpers.c
> create mode 100644 tools/testing/selftests/bpf/disasm_helpers.h
>

[...]

> +uint32_t disasm_insn(struct bpf_insn *insn, char *buf, size_t buf_sz)
> +{
> + struct print_insn_context ctx = {
> + .buf = buf,
> + .sz = buf_sz,
> + };
> + struct bpf_insn_cbs cbs = {
> + .cb_print = print_insn_cb,
> + .private_data = &ctx,
> + };
> + int pfx_end, sfx_start, len;
> + bool double_insn;
> +
> + print_bpf_insn(&cbs, insn, true);
> + /* We share code with kernel BPF disassembler, it adds '(FF) ' prefix
> + * for each instruction (FF stands for instruction `code` byte).
> + * Remove the prefix inplace, and also simplify call instructions.
> + * E.g.: "(85) call foo#10" -> "call foo".
> + */
> + pfx_end = 0;
> + sfx_start = max((int)strlen(buf) - 1, 0);
> + /* For whatever reason %n is not counted in sscanf return value */
> + sscanf(buf, "(%*[^)]) %n", &pfx_end);

let me simplify this a bit ;)

pfx_end = 5;

not as sophisticated, but equivalent

> + sscanf(buf, "(%*[^)]) call %*[^#]%n", &sfx_start);

is it documented that sfx_start won't be updated if sscanf() doesn't
successfully match?

if not, maybe let's do something like below

if (strcmp(buf + 5, "call ", 5) == 0 && (tmp = strrchr(buf, '#')))
sfx_start = tmp - buf;

> + len = sfx_start - pfx_end;
> + memmove(buf, buf + pfx_end, len);
> + buf[len] = 0;
> + double_insn = insn->code == (BPF_LD | BPF_IMM | BPF_DW);
> + return double_insn ? 2 : 1;
> +}

[...]

> @@ -739,12 +685,11 @@ static void match_program(struct btf *btf,
> PRINT_FAIL("Can't open memory stream\n");
> goto out;
> }
> - if (skip_first_insn)
> - print_xlated(prog_out, buf + 1, cnt - 1);
> - else
> - print_xlated(prog_out, buf, cnt);
> + for (i = skip_first_insn ? 1 : 0; i < cnt;) {
> + i += disasm_insn(buf + i, insn_buf, sizeof(insn_buf));
> + fprintf(prog_out, "%s\n", insn_buf);
> + }
> fclose(prog_out);
> - remove_insn_prefix(text, MAX_PROG_TEXT_SZ);
>
> ASSERT_TRUE(match_pattern(btf, pattern, text, reg_map),
> pinfo->prog_kind);
> diff --git a/tools/testing/selftests/bpf/testing_helpers.c b/tools/testing/selftests/bpf/testing_helpers.c
> index d5379a0e6da8..ac7c66f4fc7b 100644
> --- a/tools/testing/selftests/bpf/testing_helpers.c
> +++ b/tools/testing/selftests/bpf/testing_helpers.c
> @@ -7,6 +7,7 @@
> #include <errno.h>
> #include <bpf/bpf.h>
> #include <bpf/libbpf.h>
> +#include "disasm.h"
> #include "test_progs.h"
> #include "testing_helpers.h"
> #include <linux/membarrier.h>
> --
> 2.45.2
>