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

[Steven Rostedt]

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?

-- Steve