Re: [tip: x86/entry] x86/entry: Treat BUG/WARN as NMI-like entries

[Peter Zijlstra]

On Mon, Jun 15, 2020 at 10:06:20AM -0700, Andy Lutomirski wrote:
> On Mon, Jun 15, 2020 at 7:50 AM Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:

> Hmm.  IMO you're making two changes here, and this is fiddly enough
> that it might be worth separating them for bisection purposes.

Sure, can do.

> > ---
> >
> > 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.

All !user exceptions really should be NMI-like. If you want to go
overboard, I suppose you can look at IF and have them behave interrupt
like when set, but why make things complicated.

Anyway, let me to smaller and proper patches for this.