Re: [RFC][PATCH] tracing/x86: Save CR2 before tracing irqsoff on error_entry

From: Peter Zijlstra <peterz@infradead.org>
To: Andy Lutomirski <luto@kernel.org>
Cc: 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>
Subject: Re: [RFC][PATCH] tracing/x86: Save CR2 before tracing irqsoff on error_entry
Date: Thu, 21 Mar 2019 21:50:11 +0100	[thread overview]
Message-ID: <20190321205011.GE2490@worktop.programming.kicks-ass.net> (raw)
In-Reply-To: <CALCETrV1YRo8C6=VtFfrSDFAjQBs=Z6+uw-5hbus4frqAbUqGQ@mail.gmail.com>

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

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

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