Re: [PATCH bpf] bpf, bpftool: Fix incorrect disasm pc

[Leon Hwang <leon.hwang@linux.dev>]

On 2024/10/31 08:27, Quentin Monnet wrote:
> 2024-10-30 17:47 UTC+0800 ~ Leon Hwang <hffilwlqm@gmail.com>
>> From: Leon Hwang <leon.hwang@linux.dev>
>>
>> This patch addresses the bpftool issue "Wrong callq address displayed"[0].
>>
>> The issue stemmed from an incorrect program counter (PC) value used during
>> disassembly with LLVM or libbfd. To calculate the correct address for
>> relative calls, the PC argument must reflect the actual address in the
>> kernel.
>>
>> [0] https://github.com/libbpf/bpftool/issues/109
>>
>> Fixes: e1947c750ffe ("bpftool: Refactor disassembler for JIT-ed programs")
>> Signed-off-by: Leon Hwang <leon.hwang@linux.dev>
>> ---
>>  tools/bpf/bpftool/jit_disasm.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/bpf/bpftool/jit_disasm.c b/tools/bpf/bpftool/jit_disasm.c
>> index 7b8d9ec89ebd3..fe8fabba4b05f 100644
>> --- a/tools/bpf/bpftool/jit_disasm.c
>> +++ b/tools/bpf/bpftool/jit_disasm.c
>> @@ -114,8 +114,7 @@ disassemble_insn(disasm_ctx_t *ctx, unsigned char *image, ssize_t len, int pc)
>>  	char buf[256];
>>  	int count;
>>  
>> -	count = LLVMDisasmInstruction(*ctx, image + pc, len - pc, pc,
>> -				      buf, sizeof(buf));
>> +	count = LLVMDisasmInstruction(*ctx, image, len, pc, buf, sizeof(buf));
>>  	if (json_output)
>>  		printf_json(buf);
>>  	else
>> @@ -360,7 +359,8 @@ int disasm_print_insn(unsigned char *image, ssize_t len, int opcodes,
>>  			printf("%4x:" DISASM_SPACER, pc);
>>  		}
>>  
>> -		count = disassemble_insn(&ctx, image, len, pc);
>> +		count = disassemble_insn(&ctx, image + pc, len - pc,
>> +					 func_ksym + pc);
> 
> Thanks a lot for looking into this! Your patch does solve the issue for
> the LLVM disassembler (nice!), but it breaks the libbfd one:
> 
> 
> 	$ ./bpftool version | grep features
> 	features: libbfd
> 	# ./bpftool prog dump j id 111 op
> 	int xdp_redirect_map_0(struct xdp_md * xdp):
> 	bpf_prog_a8f6f9c4be77b94c_xdp_redirect_map_0:
> 	; return bpf_redirect_map(&tx_port, 0, 0);
> 	   0:   Address 0xffffffffc01ae950 is out of bounds.
> 
> I don't think we can change the PC in the case of libbfd, as far as I
> can tell it needs to point to the first instruction to disassemble. Two
> of the arguments we pass to disassemble_insn(), image and len, are
> ignored by the libbfd disassembler; so it leaves only the ctx argument
> that we can maybe update to pass the func_ksym, but I haven't found how
> to do that yet (if possible at all).
> 
> Thanks,
> Quentin
> 

Hi Quentin,

After diving into the details of libbfd, I’ve found a way to correct the
callq address. By adjusting the relative addresses using func_ksym
within a custom info->print_addr_func, we can achieve accurate results.

Here’s the updated patch:

From 687f165fe79b67ba457672bb682bde3d916ce0cd Mon Sep 17 00:00:00 2001
From: Leon Hwang <leon.hwang@linux.dev>
Date: Thu, 31 Oct 2024 13:00:05 +0800
Subject: [PATCH bpf v2] bpf, bpftool: Fix incorrect disasm pc

This patch addresses the bpftool issue "Wrong callq address displayed"[0].

The issue stemmed from an incorrect program counter (PC) value used during
disassembly with LLVM or libbfd.

For LLVM: The PC argument must represent the actual address in the kernel
to compute the correct relative address.

For libbfd: The relative address can be adjusted by adding func_ksym within
the custom info->print_address_func to yield the correct address.

[0] https://github.com/libbpf/bpftool/issues/109

Fixes: e1947c750ffe ("bpftool: Refactor disassembler for JIT-ed programs")
Acked-by: Yonghong Song <yonghong.song@linux.dev>
Signed-off-by: Leon Hwang <leon.hwang@linux.dev>
---
 tools/bpf/bpftool/jit_disasm.c | 40 ++++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 11 deletions(-)

diff --git a/tools/bpf/bpftool/jit_disasm.c b/tools/bpf/bpftool/jit_disasm.c
index 7b8d9ec89..f76d4bf0c 100644
--- a/tools/bpf/bpftool/jit_disasm.c
+++ b/tools/bpf/bpftool/jit_disasm.c
@@ -80,7 +80,8 @@ symbol_lookup_callback(__maybe_unused void *disasm_info,
 static int
 init_context(disasm_ctx_t *ctx, const char *arch,
 	     __maybe_unused const char *disassembler_options,
-	     __maybe_unused unsigned char *image, __maybe_unused ssize_t len)
+	     __maybe_unused unsigned char *image, __maybe_unused ssize_t len,
+	     __maybe_unused __u64 func_ksym)
 {
 	char *triple;

@@ -109,12 +110,13 @@ static void destroy_context(disasm_ctx_t *ctx)
 }

 static int
-disassemble_insn(disasm_ctx_t *ctx, unsigned char *image, ssize_t len,
int pc)
+disassemble_insn(disasm_ctx_t *ctx, unsigned char *image, ssize_t len,
int pc,
+		 __u64 func_ksym)
 {
 	char buf[256];
 	int count;

-	count = LLVMDisasmInstruction(*ctx, image + pc, len - pc, pc,
+	count = LLVMDisasmInstruction(*ctx, image + pc, len - pc, func_ksym + pc,
 				      buf, sizeof(buf));
 	if (json_output)
 		printf_json(buf);
@@ -137,7 +139,20 @@ int disasm_init(void)
 #define DISASM_SPACER "\t"

 typedef struct {
-	struct disassemble_info *info;
+	struct disassemble_info info;
+	__u64 func_ksym;
+} disasm_info;
+
+static void disasm_print_addr(bfd_vma addr, struct disassemble_info *info)
+{
+	disasm_info *dinfo = container_of(info, disasm_info, info);
+
+	addr += dinfo->func_ksym;
+	generic_print_address(addr, info);
+}
+
+typedef struct {
+	disasm_info *info;
 	disassembler_ftype disassemble;
 	bfd *bfdf;
 } disasm_ctx_t;
@@ -215,7 +230,7 @@ static int fprintf_json_styled(void *out,

 static int init_context(disasm_ctx_t *ctx, const char *arch,
 			const char *disassembler_options,
-			unsigned char *image, ssize_t len)
+			unsigned char *image, ssize_t len, __u64 func_ksym)
 {
 	struct disassemble_info *info;
 	char tpath[PATH_MAX];
@@ -238,12 +253,13 @@ static int init_context(disasm_ctx_t *ctx, const
char *arch,
 	}
 	bfdf = ctx->bfdf;

-	ctx->info = malloc(sizeof(struct disassemble_info));
+	ctx->info = malloc(sizeof(disasm_info));
 	if (!ctx->info) {
 		p_err("mem alloc failed");
 		goto err_close;
 	}
-	info = ctx->info;
+	ctx->info->func_ksym = func_ksym;
+	info = &ctx->info->info;

 	if (json_output)
 		init_disassemble_info_compat(info, stdout,
@@ -272,6 +288,7 @@ static int init_context(disasm_ctx_t *ctx, const
char *arch,
 		info->disassembler_options = disassembler_options;
 	info->buffer = image;
 	info->buffer_length = len;
+	info->print_address_func = disasm_print_addr;

 	disassemble_init_for_target(info);

@@ -304,9 +321,10 @@ static void destroy_context(disasm_ctx_t *ctx)

 static int
 disassemble_insn(disasm_ctx_t *ctx, __maybe_unused unsigned char *image,
-		 __maybe_unused ssize_t len, int pc)
+		 __maybe_unused ssize_t len, __u64 pc,
+		 __maybe_unused __u64 func_ksym)
 {
-	return ctx->disassemble(pc, ctx->info);
+	return ctx->disassemble(pc, &ctx->info->info);
 }

 int disasm_init(void)
@@ -331,7 +349,7 @@ int disasm_print_insn(unsigned char *image, ssize_t
len, int opcodes,
 	if (!len)
 		return -1;

-	if (init_context(&ctx, arch, disassembler_options, image, len))
+	if (init_context(&ctx, arch, disassembler_options, image, len, func_ksym))
 		return -1;

 	if (json_output)
@@ -360,7 +378,7 @@ int disasm_print_insn(unsigned char *image, ssize_t
len, int opcodes,
 			printf("%4x:" DISASM_SPACER, pc);
 		}

-		count = disassemble_insn(&ctx, image, len, pc);
+		count = disassemble_insn(&ctx, image, len, pc, func_ksym);

 		if (json_output) {
 			/* Operand array, was started in fprintf_json. Before
-- 
2.44.0


Thanks,
Leon