Re: [PATCH v2 5/7] x86/mm, tracing: Fix CR2 corruption

[Andy Lutomirski]

On Sat, Jul 6, 2019 at 3:27 PM Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>
> On Sat, 6 Jul 2019 14:41:22 -0700
> Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> > On Fri, Jul 5, 2019 at 6:50 AM Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
> > >
> > > Also; all previous attempts at fixing this have been about pushing the
> > > read_cr2() earlier; notably:
> > >
> > >   0ac09f9f8cd1 ("x86, trace: Fix CR2 corruption when tracing page faults")
> > >   d4078e232267 ("x86, trace: Further robustify CR2 handling vs tracing")
> >
> > I think both of those are because people - again - felt like tracing
> > can validly corrupt CPU state, and then they fix up the symptoms
> > rather than the cause.
> >
> > Which I disagree with.
> >
> > > And I'm thinking that with exception of this patch, the rest are
> > > worthwhile cleanups regardless.
> >
> > I don't have any issues with the patches themselves, I agree that they
> > are probably good on their own.
> >
> > I *do* have issues with the "tracing can change CPU state so we need
> > to deal with it" model, though. I think that mode of thinking is
> > wrong. I don't believe tracing should ever change core CPU state and
> > that be considered ok.
> >
> > > Also; while looking at this, if we do continue with the C wrappers from
> > > the very last patch, we can do horrible things like this on top and move
> > > the read_cr2() back into C code.
> >
> > Again, I don't think that is the problem. I think it's a much more
> > fundamental problem in thinking that core code should be changed
> > because tracing is broken garbage and didn't do things right.
> >
> > I see Eiichi's patch, and it makes me go "that looks better" - simply
> > because it fixes the fundamental issue, rather than working around the
> > symptoms.
> >
>
> We also have to deal with reading vmalloc'd data as that can fault too.
> The perf ring buffer IIUC is vmalloc, so if perf records in one of
> these locations, then the reading of the vmalloc area has a potential
> to fault corrupting the CR2 register as well. Or have we changed
> vmalloc to no longer fault?
>

What context is that happening in?  If the tracepoint-saves-CR2 patch
is in, then it should be fine if it nests inside of tracing, right?
And NMI needs to save and restore CR2 no matter what, since it can
happen before we can possibly save CR2.  For that matter, MCE should
save and restore CR2 as well.

All that being said, I do like the idea of moving all this crud (IRQ
tracing, CR2 handling, and context tracking) into C.  I haven't had
time to review that part of Peter's series yet, but I should get to it
soon.