[bpf] bpf, bpftool: Fix incorrect disasm pc

Commit Message

Leon Hwang Oct. 30, 2024, 9:47 a.m. UTC

From: Leon Hwang <[e-mail]>

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 <[e-mail]>
---
 tools/bpf/bpftool/jit_disasm.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Leon Hwang Oct. 30, 2024, 10:10 a.m. UTC | #1

On 2024/10/30 17:47, Leon Hwang wrote:
> From: Leon Hwang <[e-mail]>
> 
> 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 <[e-mail]>
> ---
>  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

Stanislav Fomichev Oct. 30, 2024, 2:56 p.m. UTC | #2

On 10/30, Leon Hwang wrote:
> 
> 
> On 2024/10/30 17:47, Leon Hwang wrote:
> > From: Leon Hwang <[e-mail]>
> > 
> > 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 <[e-mail]>
> > ---
> >  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?

Leon Hwang Oct. 30, 2024, 3:13 p.m. UTC | #3

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 <[e-mail]>
>>>
>>> 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 <[e-mail]>
>>> ---
>>>  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

Stanislav Fomichev Oct. 30, 2024, 3:35 p.m. UTC | #4

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 <[e-mail]>
> >>>
> >>> 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 <[e-mail]>
> >>> ---
> >>>  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).

Yonghong Song Oct. 30, 2024, 5:28 p.m. UTC | #5

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

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 <[e-mail]>

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

Patch

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