[Re: v5.15.57 regression - boot panic after retbleed backports with CONFIG_KPROBES_SANITY_TEST=y] On 05/08/2022 (Fri 18:13) Thadeu Lima de Souza Cascardo wrote:

On Fri, Aug 05, 2022 at 04:04:38PM -0400, Paul Gortmaker wrote:

...

The panic comes from the sanity test code, but after trying to boil down the .config differences between the kitchen sink our test team uses, and a "defconfig", it seems there are at least a couple extra dependencies for creating a reproducer:

[...]

...

rcu: Hierarchical SRCU implementation. Kprobe smoke test: started BUG: unable to handle page fault for address: ffffffffc110f3e7 #PF: supervisor instruction fetch in kernel mode #PF: error_code(0x0010) - not-present page PGD b2c60f067 P4D b2c60f067 PUD b2c611067 PMD 0 Oops: 0010 [#1] SMP NOPTI CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.15.57 #33

[...]

Can you try the patch below?

[ 2.529263] rcu: Hierarchical SRCU implementation. [ 2.530393] Kprobe smoke test: started [ 2.555965] Kprobe smoke test: passed successfully [ 2.556454] smp: Bringing up secondary CPUs ...

As per above, the same spot in the kprobe test seems to manage to not panic anymore and the remainder of the boot looks clean and normal.

I tested directly on vanilla v5.15.57.

Thanks for the quick response! Paul. --

Thanks. Cascardo.

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c index 74c2f88a43d0..6bb479ce1ae4 100644 --- a/arch/x86/kernel/ftrace.c +++ b/arch/x86/kernel/ftrace.c @@ -321,12 +321,12 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size) unsigned long offset; unsigned long npages; unsigned long size;

• unsigned long retq; unsigned long *ptr; void *trampoline; void *ip; /* 48 8b 15 <offset> is movq <offset>(%rip), %rdx */ unsigned const char op_ref[] = { 0x48, 0x8b, 0x15 };

• unsigned const char retq[] = { RET_INSN_OPCODE, INT3_INSN_OPCODE }; union ftrace_op_code_union op_ptr; int ret;

@@ -364,15 +364,10 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size) goto fail; ip = trampoline + size;

• /* The trampoline ends with ret(q) */
• retq = (unsigned long)ftrace_stub; if (cpu_feature_enabled(X86_FEATURE_RETHUNK)) memcpy(ip, text_gen_insn(JMP32_INSN_OPCODE, ip, &__x86_return_thunk), JMP32_INSN_SIZE); else

• ret = copy_from_kernel_nofault(ip, (void *)retq, RET_SIZE);
  

• if (WARN_ON(ret < 0))

• goto fail;
  

• memcpy(ip, retq, sizeof(retq));
  

/* No need to test direct calls on created trampolines */ if (ops->flags & FTRACE_OPS_FL_SAVE_REGS) {

From: Peter Zijlstra peterz@infradead.org

commit e52fc2cf3f662828cc0d51c4b73bed73ad275fce upstream.

Return trampoline must not use indirect branch to return; while this preserves the RSB, it is fundamentally incompatible with IBT. Instead use a retpoline like ROP gadget that defeats IBT while not unbalancing the RSB.

And since ftrace_stub is no longer a plain RET, don't use it to copy from. Since RET is a trivial instruction, poke it directly.

Signed-off-by: Peter Zijlstra (Intel) peterz@infradead.org Acked-by: Josh Poimboeuf jpoimboe@redhat.com Link: https://lore.kernel.org/r/20220308154318.347296408@infradead.org [cascardo: remove ENDBR] Signed-off-by: Thadeu Lima de Souza Cascardo cascardo@canonical.com --- arch/x86/kernel/ftrace.c | 9 ++------- arch/x86/kernel/ftrace_64.S | 19 +++++++++++++++---- 2 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c index 5080f578236a..8160d1dc6ed3 100644 --- a/arch/x86/kernel/ftrace.c +++ b/arch/x86/kernel/ftrace.c @@ -322,12 +322,12 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size) unsigned long offset; unsigned long npages; unsigned long size; - unsigned long retq; unsigned long *ptr; void *trampoline; void *ip; /* 48 8b 15 <offset> is movq <offset>(%rip), %rdx */ unsigned const char op_ref[] = { 0x48, 0x8b, 0x15 }; + unsigned const char retq[] = { RET_INSN_OPCODE, INT3_INSN_OPCODE }; union ftrace_op_code_union op_ptr; int ret;

@@ -365,12 +365,7 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size) goto fail;

ip = trampoline + size; - - /* The trampoline ends with ret(q) */ - retq = (unsigned long)ftrace_stub; - ret = copy_from_kernel_nofault(ip, (void *)retq, RET_SIZE); - if (WARN_ON(ret < 0)) - goto fail; + memcpy(ip, retq, RET_SIZE);

/* No need to test direct calls on created trampolines */ if (ops->flags & FTRACE_OPS_FL_SAVE_REGS) { diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S index d6af81d1b788..6cc14a835991 100644 --- a/arch/x86/kernel/ftrace_64.S +++ b/arch/x86/kernel/ftrace_64.S @@ -181,7 +181,6 @@ SYM_INNER_LABEL(ftrace_graph_call, SYM_L_GLOBAL)

/* * This is weak to keep gas from relaxing the jumps. - * It is also used to copy the RET for trampolines. */ SYM_INNER_LABEL_ALIGN(ftrace_stub, SYM_L_WEAK) UNWIND_HINT_FUNC @@ -335,7 +334,7 @@ SYM_FUNC_START(ftrace_graph_caller) SYM_FUNC_END(ftrace_graph_caller)

SYM_FUNC_START(return_to_handler) - subq $24, %rsp + subq $16, %rsp

/* Save the return values */ movq %rax, (%rsp) @@ -347,7 +346,19 @@ SYM_FUNC_START(return_to_handler) movq %rax, %rdi movq 8(%rsp), %rdx movq (%rsp), %rax - addq $24, %rsp - JMP_NOSPEC rdi + + addq $16, %rsp + /* + * Jump back to the old return address. This cannot be JMP_NOSPEC rdi + * since IBT would demand that contain ENDBR, which simply isn't so for + * return addresses. Use a retpoline here to keep the RSB balanced. + */ + ANNOTATE_INTRA_FUNCTION_CALL + call .Ldo_rop + int3 +.Ldo_rop: + mov %rdi, (%rsp) + UNWIND_HINT_FUNC + RET SYM_FUNC_END(return_to_handler) #endif