diff mbox series

[bpf] bpf, bpftool: Fix incorrect disasm pc

Message ID 20241030094741.22929-1-hffilwlqm@gmail.com (mailing list archive)
State New
Delegated to: BPF
Headers show
Series [bpf] bpf, bpftool: Fix incorrect disasm pc | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for bpf
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 5 this patch: 5
netdev/build_tools success Errors and warnings before: 0 (+0) this patch: 0 (+0)
netdev/cc_maintainers fail 1 blamed authors not CCed: song@kernel.org; 14 maintainers not CCed: sdf@fomichev.me ndesaulniers@google.com kpsingh@kernel.org nathan@kernel.org morbo@google.com john.fastabend@gmail.com eddyz87@gmail.com llvm@lists.linux.dev martin.lau@linux.dev song@kernel.org haoluo@google.com yonghong.song@linux.dev justinstitt@google.com jolsa@kernel.org
netdev/build_clang success Errors and warnings before: 3 this patch: 3
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 3 this patch: 3
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 18 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-PR success PR summary
bpf/vmtest-bpf-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-VM_Test-16 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-VM_Test-18 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-VM_Test-17 success Logs for set-matrix
bpf/vmtest-bpf-VM_Test-19 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-VM_Test-28 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-14 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-VM_Test-15 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-VM_Test-27 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-33 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-VM_Test-34 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-35 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-VM_Test-41 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-VM_Test-20 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-21 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-22 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-26 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-25 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-13 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-29 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-36 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18

Commit Message

Leon Hwang Oct. 30, 2024, 9:47 a.m. UTC
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(-)

Comments

Leon Hwang Oct. 30, 2024, 10:10 a.m. UTC | #1
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
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 <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?
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 <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
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 <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).
Yonghong Song Oct. 30, 2024, 5:28 p.m. UTC | #5
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
diff mbox series

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