Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20200225223321.231477305@linutronix.de> <20200225224145.444611199@linutronix.de>
 <20200226080538.GO18400@hirez.programming.kicks-ass.net> <20200226092018.GR18400@hirez.programming.kicks-ass.net>
In-Reply-To: <20200226092018.GR18400@hirez.programming.kicks-ass.net>
From:   Andy Lutomirski <luto@amacapital.net>
Date:   Wed, 26 Feb 2020 07:11:39 -0800
Message-ID: <CALCETrXYbmrVvYQzBDp8YP+-UyF3KPDgcK__HuNmpdsMBJYDVA@mail.gmail.com>
Subject: Re: [patch 13/16] x86/entry: Move irqflags and context tracking to C
 for simple idtentries
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     Thomas Gleixner <tglx@linutronix.de>,
        LKML <linux-kernel@vger.kernel.org>, X86 ML <x86@kernel.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Brian Gerst <brgerst@gmail.com>,
        Juergen Gross <jgross@suse.com>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Arnd Bergmann <arnd@arndb.de>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Feb 26, 2020 at 1:20 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Wed, Feb 26, 2020 at 09:05:38AM +0100, Peter Zijlstra wrote:
> > On Tue, Feb 25, 2020 at 11:33:34PM +0100, Thomas Gleixner wrote:
> >
> > > --- a/arch/x86/include/asm/idtentry.h
> > > +++ b/arch/x86/include/asm/idtentry.h
> > > @@ -7,14 +7,31 @@
> > >
> > >  #ifndef __ASSEMBLY__
> > >
> > > +#ifdef CONFIG_CONTEXT_TRACKING
> > > +static __always_inline void enter_from_user_context(void)
> > > +{
> > > +   CT_WARN_ON(ct_state() != CONTEXT_USER);
> > > +   user_exit_irqoff();
> > > +}
> > > +#else
> > > +static __always_inline void enter_from_user_context(void) { }
> > > +#endif
> > > +
> > >  /**
> > >   * idtentry_enter - Handle state tracking on idtentry
> > >   * @regs:  Pointer to pt_regs of interrupted context
> > >   *
> > > - * Place holder for now.
> > > + * Invokes:
> > > + *  - The hardirq tracer to keep the state consistent as low level ASM
> > > + *    entry disabled interrupts.
> > > + *
> > > + *  - Context tracking if the exception hit user mode
> > >   */
> > >  static __always_inline void idtentry_enter(struct pt_regs *regs)
> > >  {
> > > +   trace_hardirqs_off();
> > > +   if (user_mode(regs))
> > > +           enter_from_user_context();
> > >  }
> >
> > So:
> >
> >       asm_exc_int3
> >         exc_int3
> >           idtentry_enter()
> >             enter_from_user_context
> >               if (context_tracking_enabled())
> >
> >           poke_int3_handler();
> >
> > Is, AFAICT, completely buggered.
> >
> > You can't have a static_branch before the poke_int3_handler that deals
> > with text_poke.
>
> Forgot to say that this isn't new with this patch, just noticed it when
> chasing after tracepoints.
>
> After my patch series we can actually fix this by moving the
> poke_int3_handler() before idtentry_enter().

In some sense, this is a weakness of the magic macro approach.  Some
of the entries just want to have code that runs before all the entry
fixups.  This is an example of it.  So are the cr2 reads.  It can all
be made to work, but it's a bit gross.