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: <20190320221534.165ab87b@oasis.local.home> <20190321083317.GL6058@hirez.programming.kicks-ass.net>
 <20190321090241.GL6521@hirez.programming.kicks-ass.net> <20190321104517.GM6521@hirez.programming.kicks-ass.net>
 <20190321093242.4a948198@gandalf.local.home> <20190321172203.GS5996@hirez.programming.kicks-ass.net>
 <CALCETrXqmjJt7azNoEtxs2eBkDd-vjUAYvrjNh_RD30DAvksBw@mail.gmail.com>
 <20190321141020.641e313f@gandalf.local.home> <CALCETrV1YRo8C6=VtFfrSDFAjQBs=Z6+uw-5hbus4frqAbUqGQ@mail.gmail.com>
 <20190321205011.GE2490@worktop.programming.kicks-ass.net>
In-Reply-To: <20190321205011.GE2490@worktop.programming.kicks-ass.net>
From:   Andy Lutomirski <luto@kernel.org>
Date:   Thu, 21 Mar 2019 19:52:14 -0700
Message-ID: <CALCETrUXYypfeZoVBQ4VUFnUNt9un71C=QwTKLOobqXbpZuE+w@mail.gmail.com>
Subject: Re: [RFC][PATCH] tracing/x86: Save CR2 before tracing irqsoff on error_entry
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     Andy Lutomirski <luto@kernel.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Juergen Gross <jgross@suse.com>,
        LKML <linux-kernel@vger.kernel.org>,
        "H. Peter Anvin" <hpa@zytor.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@kernel.org>, Borislav Petkov <bp@alien8.de>,
        Joel Fernandes <joel@joelfernandes.org>,
        He Zhe <zhe.he@windriver.com>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Clark Williams <williams@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Mar 21, 2019 at 1:50 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Thu, Mar 21, 2019 at 11:27:00AM -0700, Andy Lutomirski wrote:
> > Well, here's pass zero at this.  Untested, because it obviously
> > doesn't work.  Here are just a few things that are almost certainly
> > wrong with it
>
> Aah, you're proposing to simply not do TRACE_IRQS_OFF and
> CALL_enter_from_user_mode and let \do_sym deal with it all.
>
> Yes, that looks like it could almost work; esp. if you start by only
> doing this for the idtentry stuff.
>
> >  - The IRQ tracing needs to be re-added.
> >
> >  - Some real semantics need to be defined for precisely what code is
> > responsible for tracing.
>
> So we get passed \do_sym, how about we do:
>
>         call    __\do_sym
>
> And then use some CPP magic to generate the those functions such that we
> have a consistent part of C glue between our asm and our 'real' C
> handler.
>
> This glue can then do the tracing in a consistent manner.
>
> #define IDT_HANDLER(do_sym)                                             \
> asmlinkage __visible notrace void __do_sym(struct pt_regs *regs)        \
> {                                                                       \
>         trace_hardirqs_off();                                           \
>         if (user_mode(regs))                                            \
>                 enter_from_user_mode();                                 \
>         do_sym(regs);                                                   \
> }
>
> Except more complicated I'm afraid, we need to handle more args etc..

Seems reasonable.  We should drop the asmlinkage, and if do_sym is
static, the code should be pretty good.

>
> >  - We need some asm-callable assertions to check the following
> > conditions as appropriate:
> >
> >  (a) that IRQ flags are currently traced as off.
>
> What do you need this for? When returning from do_sym ?
>
> >  (b) that IRQ flags are currently traced to match the IRET frame.
>
> idem. Can't we have our C glue do that?

The idea was to sanity check the C glue.  Maybe this isn't such a big deal.

>
> >  (c) that our context tracking is currently in good shape.  I'm not
> > 100% sure how to define this.
>
> So looking at this more; I used the %ebx games employed by
> paranoid_entry to convey the state, but I didn't have to do that, the
> actual condition seems to be:
>
>         regs->cs & 3
>
> aka. user_mode(regs). In this case our C glue would need to do the
> context tracking user exit.
>
> In fact, I can change my patch to use that and reduce the ebx ugly.
>
> >  - We need to do some serious don't-instrument-me stuff to all the C
> > entries, since we're now in an awful context when calling them.
>
> Yah, but that's not new. do_page_fault(), sync_regs() at the very least
> have this, so we can easily have our C glue have this too.

True.