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

[Stanislav Fomichev <stfomichev@gmail.com>]

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