Messages in this thread | | | From | Andy Lutomirski <> | Date | Thu, 21 Mar 2019 19:52:14 -0700 | Subject | Re: [RFC][PATCH] tracing/x86: Save CR2 before tracing irqsoff on error_entry |
| |
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.
| |