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

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

[Leon Hwang]

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);
 
 		if (json_output) {
 			/* Operand array, was started in fprintf_json. Before
-- 
2.44.0

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

[Leon Hwang]

On 2024/10/30 17:47, Leon Hwang wrote:
> 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)

It seems we should update the type of pc from int to __u64, as the type
of func_ksym is __u64 and the type of pc argument in disassemble
function of LLVM and libbfd is __u64 for 64 bit arch.

Thanks,
Leon

>  	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);
>  
>  		if (json_output) {
>  			/* Operand array, was started in fprintf_json. Before

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

[Stanislav Fomichev]

On 10/30, Leon Hwang wrote:
> 
> 
> On 2024/10/30 17:47, Leon Hwang wrote:
> > 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)
> 
> It seems we should update the type of pc from int to __u64, as the type
> of func_ksym is __u64 and the type of pc argument in disassemble
> function of LLVM and libbfd is __u64 for 64 bit arch.

I'm assuming u32 is fine as long as the prog size is under 4G?

> >  	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));

For my understanding, another way to fix it would be:
	count = LLVMDisasmInstruction(*ctx, image + pc, len - pc, 0,
				      buf, sizeof(buf));

IOW, in the original code, using 0 instead of pc should fix it as well?
Or am I missing something?

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

[Leon Hwang]

On 2024/10/30 22:56, Stanislav Fomichev wrote:
> On 10/30, Leon Hwang wrote:
>>
>>
>> On 2024/10/30 17:47, Leon Hwang wrote:
>>> 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)
>>
>> It seems we should update the type of pc from int to __u64, as the type
>> of func_ksym is __u64 and the type of pc argument in disassemble
>> function of LLVM and libbfd is __u64 for 64 bit arch.
> 
> I'm assuming u32 is fine as long as the prog size is under 4G?
> 

It works well with int. So it's unnecessary to update its type.

>>>  	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));
> 
> For my understanding, another way to fix it would be:
> 	count = LLVMDisasmInstruction(*ctx, image + pc, len - pc, 0,
> 				      buf, sizeof(buf));
> 
> IOW, in the original code, using 0 instead of pc should fix it as well?
> Or am I missing something?

No. It does not work when using 0. I just tried it.

I think it's because LLVM is unable to infer the actual address of the
disassembling insn when we do not provide func_ksym to LLVM.

Thanks,
Leon

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

[Stanislav Fomichev]

On 10/30, Leon Hwang wrote:
> 
> 
> On 2024/10/30 22:56, Stanislav Fomichev wrote:
> > On 10/30, Leon Hwang wrote:
> >>
> >>
> >> On 2024/10/30 17:47, Leon Hwang wrote:
> >>> 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)
> >>
> >> It seems we should update the type of pc from int to __u64, as the type
> >> of func_ksym is __u64 and the type of pc argument in disassemble
> >> function of LLVM and libbfd is __u64 for 64 bit arch.
> > 
> > I'm assuming u32 is fine as long as the prog size is under 4G?
> > 
> 
> It works well with int. So it's unnecessary to update its type.
> 
> >>>  	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));
> > 
> > For my understanding, another way to fix it would be:
> > 	count = LLVMDisasmInstruction(*ctx, image + pc, len - pc, 0,
> > 				      buf, sizeof(buf));
> > 
> > IOW, in the original code, using 0 instead of pc should fix it as well?
> > Or am I missing something?
> 
> No. It does not work when using 0. I just tried it.
> 
> I think it's because LLVM is unable to infer the actual address of the
> disassembling insn when we do not provide func_ksym to LLVM.

Hmm, thanks for checking! I'll leave it up to Quentin to run and confirm
because I clearly don't understand how that LLVMDisasmInstruction works
:-D (and you two have been chatting on GH).

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

[Yonghong Song]

On 10/30/24 2:47 AM, Leon Hwang wrote:
> 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>

The following is the LLVMDisasmInstruction() description in
llvm-c/Disassembler.h:

/**
  * Disassemble a single instruction using the disassembler context specified in
  * the parameter DC.  The bytes of the instruction are specified in the
  * parameter Bytes, and contains at least BytesSize number of bytes.  The
  * instruction is at the address specified by the PC parameter.  If a valid
  * instruction can be disassembled, its string is returned indirectly in
  * OutString whose size is specified in the parameter OutStringSize.  This
  * function returns the number of bytes in the instruction or zero if there was
  * no valid instruction.
  */
size_t LLVMDisasmInstruction(LLVMDisasmContextRef DC, uint8_t *Bytes,
                              uint64_t BytesSize, uint64_t PC,
                              char *OutString, size_t OutStringSize);

In the above, it has
   The instruction is at the address specified by the PC parameter.

To call insn itself only encodes the difference between
helper address and 'bpf_prog + call_insn pc within prog'.
So to calculate proper final call address, the bpf_prog entry address
must be provided. So we need to supply 'prog_entry_addr + pc' instead
of 'pc'.

32bit should be okay since addr is within the first 4G.

Acked-by: Yonghong Song <yonghong.song@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);
>   
>   		if (json_output) {
>   			/* Operand array, was started in fprintf_json. Before

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

[Quentin Monnet]

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


pw-bot: cr

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

[Leon Hwang]

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

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

[Leon Hwang]

On 2024/10/31 13:27, Leon Hwang wrote:
> 
> 
> 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,

NIT: type of pc should keep int

Thanks,
Leon

> +		 __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

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

[Quentin Monnet]

2024-10-31 13:36 UTC+0800 ~ Leon Hwang <hffilwlqm@gmail.com>
> 
> 
> On 2024/10/31 13:27, Leon Hwang wrote:
>>
>>
>> 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;


I don't think we need a typdef for this one?


>> +
>> +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,
> 
> NIT: type of pc should keep int


Yep. Can you please send a v3 with this fix? So that patchwork can pick
it up properly.

I tested your v2 and it works for both disassemblers, thanks a lot!

Quentin