On Mon, Jun 15, 2020 at 7:50 AM Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote: > > On Fri, Jun 12, 2020 at 07:50:08PM -0000, tip-bot2 for Andy Lutomirski wrote: > > +DEFINE_IDTENTRY_RAW(exc_invalid_op) > > { > > + bool rcu_exit; > > + > > + /* > > + * Handle BUG/WARN like NMIs instead of like normal idtentries: > > + * if we bugged/warned in a bad RCU context, for example, the last > > + * thing we want is to BUG/WARN again in the idtentry code, ad > > + * infinitum. > > + */ > > + if (!user_mode(regs) && is_valid_bugaddr(regs->ip)) { > > vmlinux.o: warning: objtool: exc_invalid_op()+0x47: call to probe_kernel_read() leaves .noinstr.text section > > > + enum bug_trap_type type; > > + > > + nmi_enter(); > > + instrumentation_begin(); > > + trace_hardirqs_off_finish(); > > + type = report_bug(regs->ip, regs); > > + if (regs->flags & X86_EFLAGS_IF) > > + trace_hardirqs_on_prepare(); > > + instrumentation_end(); > > + nmi_exit(); > > + > > + if (type == BUG_TRAP_TYPE_WARN) { > > + /* Skip the ud2. */ > > + regs->ip += LEN_UD2; > > + return; > > + } > > + > > + /* > > + * Else, if this was a BUG and report_bug returns or if this > > + * was just a normal #UD, we want to continue onward and > > + * crash. > > + */ > > + } > > + > > + rcu_exit = idtentry_enter_cond_rcu(regs); > > + instrumentation_begin(); > > handle_invalid_op(regs); > > + instrumentation_end(); > > + idtentry_exit_cond_rcu(regs, rcu_exit); > > } > > > For now something like so will do, but we need a DEFINE_IDTENTRY_foo() > for the whole: > > if (user_mode()) { > rcu = idtentry_enter_cond_rcu() > foo_user() > idtentry_exit_cond_rcu(rcu); > } else { > nmi_enter(); > foo_kernel() > nmi_exit() > } > > thing, we're repeating that far too often. > > Hmm. IMO you're making two changes here, and this is fiddly enough that it might be worth separating them for bisection purposes. > --- > > diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c > index af75109485c26..a47e74923c4c8 100644 > --- a/arch/x86/kernel/traps.c > +++ b/arch/x86/kernel/traps.c > @@ -218,21 +218,22 @@ static inline void handle_invalid_op(struct pt_regs *regs) > > DEFINE_IDTENTRY_RAW(exc_invalid_op) > { > - bool rcu_exit; > - > /* > * Handle BUG/WARN like NMIs instead of like normal idtentries: > * if we bugged/warned in a bad RCU context, for example, the last > * thing we want is to BUG/WARN again in the idtentry code, ad > * infinitum. > */ > - if (!user_mode(regs) && is_valid_bugaddr(regs->ip)) { > - enum bug_trap_type type; > + if (!user_mode(regs)) { > + enum bug_trap_type type = BUG_TRAP_TYPE_NONE; > > nmi_enter(); > instrumentation_begin(); > trace_hardirqs_off_finish(); > - type = report_bug(regs->ip, regs); > + > + if (is_valid_bugaddr(regs->ip)) > + type = report_bug(regs->ip, regs); > + Sigh, this is indeed necessary. > if (regs->flags & X86_EFLAGS_IF) > trace_hardirqs_on_prepare(); > instrumentation_end(); > @@ -249,13 +250,16 @@ DEFINE_IDTENTRY_RAW(exc_invalid_op) > * was just a normal #UD, we want to continue onward and > * crash. > */ > - } > + handle_invalid_op(regs); But this is really a separate change. This makes handle_invalid_op() be NMI-like even for non-BUG/WARN kernel #UD entries. One might argue that this doesn't matter, and that's probably right, but I think it should be its own change with its own justification. With just my patch, I intentionally call handle_invalid_op() via the normal idtentry_enter_cond_rcu() path. --Andy
![]() |