| Message ID | 6034e8b7dc0c6954be35b6735a9b04b35903ed5f.1788154635.git.skb99@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | powerpc/bpf: address missing verifier selftest coverage | expand |
> selftests/bpf: Fix powerpc JIT disassembly failure > > Ensure that the trampoline stubs JITed at the tail of the > epilogue do not expose the dummy trampoline address stored > in the last 8 bytes(64-bit) and last 4 bytes(32-bit) > to the disassembly flow. Prevent the disassembler from > ingesting this memory address, as it may occasionally decode > into a seemingly valid but incorrect instruction. Fix this > issue by truncating the last 8/4 bytes from JITed buffers > before supplying them for disassembly. Does the changelog accurately describe when this fix takes effect? Looking at this commit, get_jited_program_text() is only called from run_subtest() in tools/testing/selftests/bpf/test_loader.c when subspec->jited.cnt != 0. The __jited expectations are only recorded when the preceding __arch_* tag matches the running architecture: } else if ((val = str_has_pfx(s, "test_arch="))) { ... collect_jit = get_current_arch() == arch; Since get_current_arch() in this tree has no powerpc case, it returns ARCH_UNKNOWN, no __arch_* tag ever matches on powerpc, jited.cnt stays 0, and get_jited_program_text() is never invoked on powerpc. The path only becomes reachable with the later patches in the same series, 422d7386c6a2b (which adds ARCH_POWERPC64 / __arch_powerpc64) and b787dda9a979d (which adds tailcall verifier selftest). Would it be more accurate to describe this as a prerequisite for that enablement rather than as a fix for a current failure? Also, the changelog says the address "may occasionally decode into a seemingly valid but incorrect instruction", whereas the comment added by this same hunk says the bytes "can't disassemble to a valid instruction", and the companion patch 0cd8bd7da278d quotes the concrete symptom: Can't disasm instruction at offset 104: c0 34 1d 00 00 00 00 c0 A hard "Can't disasm" abort (what disasm_insn() reports via PRINT_FAIL) is a different problem from a mis-decode into a plausible-looking instruction. Could the commit message be more precise about which failure mode actually occurs? --- AI reviewed your patch. Please fix the bug or email reply why it's not a bug. See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md CI run summary: https://github.com/kernel-patches/bpf/actions/runs/33368376983
diff --git a/tools/testing/selftests/bpf/jit_disasm_helpers.c b/tools/testing/selftests/bpf/jit_disasm_helpers.c index 3558fe10e28c..b291d934dd1a 100644 --- a/tools/testing/selftests/bpf/jit_disasm_helpers.c +++ b/tools/testing/selftests/bpf/jit_disasm_helpers.c @@ -179,9 +179,11 @@ int get_jited_program_text(int fd, char *text, size_t text_sz) struct bpf_prog_info info = {}; __u32 info_len = sizeof(info); __u32 jited_funcs, len, pc; + __u32 trunc_len = 0, disasm_len; __u32 *func_lens = NULL; FILE *text_out = NULL; uint8_t *image = NULL; + char *triple = NULL; int i, err = 0; if (!llvm_initialized) { @@ -225,9 +227,30 @@ int get_jited_program_text(int fd, char *text, size_t text_sz) if (!ASSERT_OK(err, "bpf_prog_get_info_by_fd #2")) goto out; + /* + * last 8 bytes contains dummy_trampoline address in JIT + * output on 64-bit and last 4 bytes on 32-bit powerpc, + * which can't disassemble to a valid instruction. + */ + triple = LLVMGetDefaultTargetTriple(); + if (triple) { + if (strstr(triple, "powerpc64") || strstr(triple, "ppc64")) + trunc_len = 8; + else if (strstr(triple, "powerpc") || strstr(triple, "ppc")) + trunc_len = 4; + LLVMDisposeMessage(triple); + } + for (pc = 0, i = 0; i < jited_funcs; ++i) { fprintf(text_out, "func #%d:\n", i); - disasm_one_func(text_out, image + pc, func_lens[i]); + + /* + * Disabled JIT have zero func_lens, hence underflow + */ + disasm_len = func_lens[i] > trunc_len ? + func_lens[i] - trunc_len : 0; + disasm_one_func(text_out, image + pc, disasm_len); + fprintf(text_out, "\n"); pc += func_lens[i]; }