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

[Andy Lutomirski]

> On Jul 6, 2019, at 6:08 PM, Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> 
> On Sat, Jul 6, 2019 at 3:41 PM Linus Torvalds
> <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>> 
>>> On Sat, Jul 6, 2019 at 3:27 PM Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>>> 
>>> We also have to deal with reading vmalloc'd data as that can fault too.
>> 
>> Ahh, that may be a better reason for PeterZ's patches and reading cr2
>> very early from asm code than the stack trace case.
> 
> Hmm. Another alternative might be to simply just make our vmalloc page
> fault handling more robust.
> 
> Right now, if we take a vmalloc page fault in an inconvenient spot, it
> is fatal because it corrupts the cr2 in the outer context.
> 
> However, it doesn't *need* to be fatal. Who cares if the outer context
> cr2 gets corrupted? We probably *shouldn't* care - it's an odd and
> unusual case, and the outer context could just handle the wrong
> vmalloc-address cr2 fine (it's going to be a no-op, since the inner
> page fault will have handled it already), return, and then re-fault.
> 
> The only reason it's fatal right now is that we care much too deeply about
> 
> (a) the error code
> (b) the pt_regs state
> 
> when we handle vmalloc faults.
> 
> So one option is that we simply handle the vmalloc faults _without_
> caring about the error code and the pt_regs state, because even if the
> error code or the pt_regs implies that the fault comes from user
> space, the cr2 value might be due to a vmalloc fault from the inner
> kernel page fault that corrupted cr2.
> 
> Right now vmalloc faults are already special and need to be handled
> without holding any locks etc. We'd just make them even more special,
> and say that we might have a vmalloc area fault from any context.
> 
> IOW, somethinig like the attached patch would make nesting vmalloc
> faults harmless. Sure, we'll do the "vmalloc fault" twice, and return
> and re-do the original page fault, but this is a very unusual case, so
> from a performance angle we don't really care.

Eww. I would like to be able to rely on fault into being correct.  Also, your patch wonât do so well if the fault is from user mode, I think.


> 
> But I guess the "read cr2 early" is fine too..
> 
>               Linus
> <patch.diff>