Messages in this thread |  | | Date | Sun, 22 Jan 2023 17:46:03 +0900 | From | Masami Hiramatsu (Google) <> | Subject | Re: [bisected] clang 15 built kernel fails to boot, stuck at "Loading Linux 6.1.1 ...", gcc 12 built kernel with same config boots fine |
| |
On Fri, 20 Jan 2023 15:57:47 +0100 Peter Zijlstra <peterz@infradead.org> wrote:
> On Fri, Jan 20, 2023 at 12:16:26PM +0100, Peter Zijlstra wrote: > > On Fri, Jan 20, 2023 at 11:20:53AM +0530, Sandipan Das wrote: > > > > > With the combination of defconfig+kvm_guest.config+localyesconfig, the only > > > thing that made a difference was the compiler optimization choice. The kernel > > > boots up with CONFIG_CC_OPTIMIZE_FOR_PERFORMANCE=y but not with > > > CONFIG_CC_OPTIMIZE_FOR_SIZE=y. Both Clang 15 and 16 kernel builds are affected. > > > > *groan*.. > > > > 0000000000000350 <amd_pmu_add_event>: > > 350: 0f 1f 44 00 00 nopl 0x0(%rax,%rax,1) 351: R_X86_64_NONE __fentry__-0x4 > > 355: 48 83 bf 20 01 00 00 00 cmpq $0x0,0x120(%rdi) > > 35d: 0f 85 00 00 00 00 jne 363 <amd_pmu_add_event+0x13> 35f: R_X86_64_PLT32 __SCT__amd_pmu_branch_add-0x4 > > 363: e9 00 00 00 00 jmp 368 <amd_pmu_add_event+0x18> 364: R_X86_64_PLT32 __x86_return_thunk-0x4 > > > > and static_call() can't deal with Jcc, I wonder why we've not seen that > > before -- this isn't totally idiotic code-gen and esp. clang is known to > > do this (see the retpoline thing). > > > > Let me see if I can do something about that. > > Here, this makes my qemu/kvm happy enough to complain about not having a > root device to mount :-) > > Let me go try booting it for real.
Thanks Peter, this fix looks good to me too.
Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Thanks,
> > --- > > diff --git a/arch/x86/include/asm/text-patching.h b/arch/x86/include/asm/text-patching.h > index f4b87f08f5c5..29832c338cdc 100644 > --- a/arch/x86/include/asm/text-patching.h > +++ b/arch/x86/include/asm/text-patching.h > @@ -184,6 +184,37 @@ void int3_emulate_ret(struct pt_regs *regs) > unsigned long ip = int3_emulate_pop(regs); > int3_emulate_jmp(regs, ip); > } > + > +static __always_inline > +void int3_emulate_jcc(struct pt_regs *regs, u8 cc, unsigned long ip, unsigned long disp) > +{ > + static const unsigned long jcc_mask[6] = { > + [0] = X86_EFLAGS_OF, > + [1] = X86_EFLAGS_CF, > + [2] = X86_EFLAGS_ZF, > + [3] = X86_EFLAGS_CF | X86_EFLAGS_ZF, > + [4] = X86_EFLAGS_SF, > + [5] = X86_EFLAGS_PF, > + }; > + > + bool invert = cc & 1; > + bool match; > + > + if (cc < 0xc) { > + match = regs->flags & jcc_mask[cc >> 1]; > + } else { > + match = ((regs->flags & X86_EFLAGS_SF) >> X86_EFLAGS_SF_BIT) ^ > + ((regs->flags & X86_EFLAGS_OF) >> X86_EFLAGS_OF_BIT); > + if (cc >= 0xe) > + match = match || (regs->flags & X86_EFLAGS_ZF); > + } > + > + if ((match && !invert) || (!match && invert)) > + ip += disp; > + > + int3_emulate_jmp(regs, ip); > +} > + > #endif /* !CONFIG_UML_X86 */ > > #endif /* _ASM_X86_TEXT_PATCHING_H */ > diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c > index 7d8c3cbde368..cd632a0171b4 100644 > --- a/arch/x86/kernel/alternative.c > +++ b/arch/x86/kernel/alternative.c > @@ -1772,6 +1772,11 @@ void text_poke_sync(void) > on_each_cpu(do_sync_core, NULL, 1); > } > > +/* > + * NOTE: crazy scheme to allow patching Jcc.d32 but not increase the size of > + * this thing. When len == 6 everything is prefixed with 0x0f and we map > + * opcode to Jcc.d8, using len to distinguish. > + */ > struct text_poke_loc { > /* addr := _stext + rel_addr */ > s32 rel_addr; > @@ -1893,6 +1898,10 @@ noinstr int poke_int3_handler(struct pt_regs *regs) > int3_emulate_jmp(regs, (long)ip + tp->disp); > break; > > + case 0x70 ... 0x7f: /* Jcc */ > + int3_emulate_jcc(regs, tp->opcode & 0xf, (long)ip, tp->disp); > + break; > + > default: > BUG(); > } > @@ -1966,16 +1975,26 @@ static void text_poke_bp_batch(struct text_poke_loc *tp, unsigned int nr_entries > * Second step: update all but the first byte of the patched range. > */ > for (do_sync = 0, i = 0; i < nr_entries; i++) { > - u8 old[POKE_MAX_OPCODE_SIZE] = { tp[i].old, }; > + u8 old[POKE_MAX_OPCODE_SIZE+1] = { tp[i].old, }; > + u8 _new[POKE_MAX_OPCODE_SIZE+1]; > + const u8 *new = tp[i].text; > int len = tp[i].len; > > if (len - INT3_INSN_SIZE > 0) { > memcpy(old + INT3_INSN_SIZE, > text_poke_addr(&tp[i]) + INT3_INSN_SIZE, > len - INT3_INSN_SIZE); > + > + if (len == 6) { > + _new[0] = 0x0f; > + memcpy(_new + 1, new, 5); > + new = _new; > + } > + > text_poke(text_poke_addr(&tp[i]) + INT3_INSN_SIZE, > - (const char *)tp[i].text + INT3_INSN_SIZE, > + new + INT3_INSN_SIZE, > len - INT3_INSN_SIZE); > + > do_sync++; > } > > @@ -2003,8 +2022,7 @@ static void text_poke_bp_batch(struct text_poke_loc *tp, unsigned int nr_entries > * The old instruction is recorded so that the event can be > * processed forwards or backwards. > */ > - perf_event_text_poke(text_poke_addr(&tp[i]), old, len, > - tp[i].text, len); > + perf_event_text_poke(text_poke_addr(&tp[i]), old, len, new, len); > } > > if (do_sync) { > @@ -2021,10 +2039,15 @@ static void text_poke_bp_batch(struct text_poke_loc *tp, unsigned int nr_entries > * replacing opcode. > */ > for (do_sync = 0, i = 0; i < nr_entries; i++) { > - if (tp[i].text[0] == INT3_INSN_OPCODE) > + u8 byte = tp[i].text[0]; > + > + if (tp[i].len == 6) > + byte = 0x0f; > + > + if (byte == INT3_INSN_OPCODE) > continue; > > - text_poke(text_poke_addr(&tp[i]), tp[i].text, INT3_INSN_SIZE); > + text_poke(text_poke_addr(&tp[i]), &byte, INT3_INSN_SIZE); > do_sync++; > } > > @@ -2042,9 +2065,11 @@ static void text_poke_loc_init(struct text_poke_loc *tp, void *addr, > const void *opcode, size_t len, const void *emulate) > { > struct insn insn; > - int ret, i; > + int ret, i = 0; > > - memcpy((void *)tp->text, opcode, len); > + if (len == 6) > + i = 1; > + memcpy((void *)tp->text, opcode+i, len-i); > if (!emulate) > emulate = opcode; > > @@ -2055,6 +2080,13 @@ static void text_poke_loc_init(struct text_poke_loc *tp, void *addr, > tp->len = len; > tp->opcode = insn.opcode.bytes[0]; > > + if (is_jcc32(&insn)) { > + /* > + * Map Jcc.d32 onto Jcc.d8 and use len to distinguish. > + */ > + tp->opcode = insn.opcode.bytes[1] - 0x10; > + } > + > switch (tp->opcode) { > case RET_INSN_OPCODE: > case JMP32_INSN_OPCODE: > @@ -2071,7 +2103,6 @@ static void text_poke_loc_init(struct text_poke_loc *tp, void *addr, > BUG_ON(len != insn.length); > } > > - > switch (tp->opcode) { > case INT3_INSN_OPCODE: > case RET_INSN_OPCODE: > @@ -2080,6 +2111,7 @@ static void text_poke_loc_init(struct text_poke_loc *tp, void *addr, > case CALL_INSN_OPCODE: > case JMP32_INSN_OPCODE: > case JMP8_INSN_OPCODE: > + case 0x70 ... 0x7f: /* Jcc */ > tp->disp = insn.immediate.value; > break; > > diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c > index b36f3c367cb2..f2b2f7545ecb 100644 > --- a/arch/x86/kernel/kprobes/core.c > +++ b/arch/x86/kernel/kprobes/core.c > @@ -464,50 +464,26 @@ static void kprobe_emulate_call(struct kprobe *p, struct pt_regs *regs) > } > NOKPROBE_SYMBOL(kprobe_emulate_call); > > -static nokprobe_inline > -void __kprobe_emulate_jmp(struct kprobe *p, struct pt_regs *regs, bool cond) > +static void kprobe_emulate_jmp(struct kprobe *p, struct pt_regs *regs) > { > unsigned long ip = regs->ip - INT3_INSN_SIZE + p->ainsn.size; > > - if (cond) > - ip += p->ainsn.rel32; > + ip += p->ainsn.rel32; > int3_emulate_jmp(regs, ip); > } > - > -static void kprobe_emulate_jmp(struct kprobe *p, struct pt_regs *regs) > -{ > - __kprobe_emulate_jmp(p, regs, true); > -} > NOKPROBE_SYMBOL(kprobe_emulate_jmp); > > -static const unsigned long jcc_mask[6] = { > - [0] = X86_EFLAGS_OF, > - [1] = X86_EFLAGS_CF, > - [2] = X86_EFLAGS_ZF, > - [3] = X86_EFLAGS_CF | X86_EFLAGS_ZF, > - [4] = X86_EFLAGS_SF, > - [5] = X86_EFLAGS_PF, > -}; > - > static void kprobe_emulate_jcc(struct kprobe *p, struct pt_regs *regs) > { > - bool invert = p->ainsn.jcc.type & 1; > - bool match; > - > - if (p->ainsn.jcc.type < 0xc) { > - match = regs->flags & jcc_mask[p->ainsn.jcc.type >> 1]; > - } else { > - match = ((regs->flags & X86_EFLAGS_SF) >> X86_EFLAGS_SF_BIT) ^ > - ((regs->flags & X86_EFLAGS_OF) >> X86_EFLAGS_OF_BIT); > - if (p->ainsn.jcc.type >= 0xe) > - match = match || (regs->flags & X86_EFLAGS_ZF); > - } > - __kprobe_emulate_jmp(p, regs, (match && !invert) || (!match && invert)); > + int3_emulate_jcc(regs, p->ainsn.jcc.type, > + regs->ip - INT3_INSN_SIZE + p->ainsn.size, > + p->ainsn.rel32); > } > NOKPROBE_SYMBOL(kprobe_emulate_jcc); > > static void kprobe_emulate_loop(struct kprobe *p, struct pt_regs *regs) > { > + unsigned long ip = regs->ip - INT3_INSN_SIZE + p->ainsn.size; > bool match; > > if (p->ainsn.loop.type != 3) { /* LOOP* */ > @@ -535,7 +511,9 @@ static void kprobe_emulate_loop(struct kprobe *p, struct pt_regs *regs) > else if (p->ainsn.loop.type == 1) /* LOOPE */ > match = match && (regs->flags & X86_EFLAGS_ZF); > > - __kprobe_emulate_jmp(p, regs, match); > + if (match) > + ip += p->ainsn.rel32; > + int3_emulate_jmp(regs, ip); > } > NOKPROBE_SYMBOL(kprobe_emulate_loop); > > diff --git a/arch/x86/kernel/static_call.c b/arch/x86/kernel/static_call.c > index 2ebc338980bc..deb909f2b62f 100644 > --- a/arch/x86/kernel/static_call.c > +++ b/arch/x86/kernel/static_call.c > @@ -9,6 +9,7 @@ enum insn_type { > NOP = 1, /* site cond-call */ > JMP = 2, /* tramp / site tail-call */ > RET = 3, /* tramp / site cond-tail-call */ > + JCC = 4, > }; > > /* > @@ -25,12 +26,38 @@ static const u8 xor5rax[] = { 0x2e, 0x2e, 0x2e, 0x31, 0xc0 }; > > static const u8 retinsn[] = { RET_INSN_OPCODE, 0xcc, 0xcc, 0xcc, 0xcc }; > > +static u8 __is_Jcc(u8 *insn) /* Jcc.d32 */ > +{ > + u8 ret = 0; > + > + if (insn[0] == 0x0f) { > + u8 tmp = insn[1]; > + if ((tmp & 0xf0) == 0x80) > + ret = tmp; > + } > + > + return ret; > +} > + > +extern void __static_call_return(void); > + > +asm (".global __static_call_return\n\t" > + ".type __static_call_return, @function\n\t" > + ASM_FUNC_ALIGN "\n\t" > + "__static_call_return:\n\t" > + "ret; int3\n\t" > + ".size __static_call_return, . - __static_call_return \n\t"); > + > static void __ref __static_call_transform(void *insn, enum insn_type type, > void *func, bool modinit) > { > const void *emulate = NULL; > int size = CALL_INSN_SIZE; > const void *code; > + u8 op, buf[6]; > + > + if ((type == JMP || type == RET) && (op = __is_Jcc(insn))) > + type = JCC; > > switch (type) { > case CALL: > @@ -57,6 +84,20 @@ static void __ref __static_call_transform(void *insn, enum insn_type type, > else > code = &retinsn; > break; > + > + case JCC: > + if (!func) { > + func = __static_call_return; > + if (cpu_feature_enabled(X86_FEATURE_RETHUNK)) > + func = x86_return_thunk; > + } > + > + buf[0] = 0x0f; > + __text_gen_insn(buf+1, op, insn+1, func, 5); > + code = buf; > + size = 6; > + > + break; > } > > if (memcmp(insn, code, size) == 0) > @@ -79,7 +120,8 @@ static void __static_call_validate(void *insn, bool tail, bool tramp) > > if (tail) { > if (opcode == JMP32_INSN_OPCODE || > - opcode == RET_INSN_OPCODE) > + opcode == RET_INSN_OPCODE || > + __is_Jcc(insn)) > return; > } else { > if (opcode == CALL_INSN_OPCODE ||
-- Masami Hiramatsu (Google) <mhiramat@kernel.org>
|  |