Date:	Wed, 12 Aug 2015 07:59:44 -0700
From:	Andy Lutomirski <luto@...capital.net>
To:	Frederic Weisbecker <fweisbec@...il.com>
Cc:	Denys Vlasenko <dvlasenk@...hat.com>,
	Rik van Riel <riel@...hat.com>,
	Borislav Petkov <bp@...en8.de>,
	Peter Zijlstra <peterz@...radead.org>,
	Brian Gerst <brgerst@...il.com>,
	Denys Vlasenko <vda.linux@...glemail.com>,
	Kees Cook <keescook@...omium.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Oleg Nesterov <oleg@...hat.com>,
	Andrew Lutomirski <luto@...nel.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Ingo Molnar <mingo@...nel.org>,
	"H. Peter Anvin" <hpa@...or.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-tip-commits@...r.kernel.org" 
	<linux-tip-commits@...r.kernel.org>
Subject: Re: [tip:x86/asm] x86/asm/entry/64: Migrate error and IRQ exit work
 to C and remove old assembly code

On Wed, Aug 12, 2015 at 6:32 AM, Frederic Weisbecker <fweisbec@...il.com> wrote:
> On Tue, Aug 11, 2015 at 04:33:05PM -0700, Andy Lutomirski wrote:
>> On Tue, Aug 11, 2015 at 4:22 PM, Frederic Weisbecker <fweisbec@...il.com> wrote:
>> >
>> > On Tue, Aug 11, 2015 at 03:51:26PM -0700, Andy Lutomirski wrote:
>> >> On Tue, Aug 11, 2015 at 3:38 PM, Frederic Weisbecker <fweisbec@...il.com> wrote:
>> >> >
>> >> > This makes me very nervous as well!
>> >> >
>> >> > It means that instead of using the context tracking save/restore model that we had
>> >> > with exception_enter/exception_exit(), now we rely on the CS register.
>> >> >
>> >> > I don't think we can do that because our "context tracking" is a soft tracking whereas
>> >> > CS is hard tracking and both are not atomically synchronized together.
>> >> >
>> >> > Imagine this situation: we are running in userspace. Context tracking knows it, everything
>> >> > is fine. Now we do a syscall, we enter in kernel entry code but we trigger an exception
>> >> > (DEBUG for example) before we got a chance to call user_exit(), which means that the context
>> >> > tracking code still thinks we are in userspace, so we look at CS from the exception entry code
>> >> > and it says the exception happened in the kernel. Hence we don't call user_exit() before calling
>> >> > the exception handler. There is the bug because the exception handler may use RCU which still
>> >> > thinks we run in userspace.
>> >>
>> >> #DB doesn't go through this patch -- it uses the paranoid entry path
>> >> and ist_enter.  But I see your point.  I think that, if we have a
>> >> problem like this in practice, then we should fix it.
>> >
>> > Whatever hack we do to prevent from exceptions happening in between real kernel entry
>> > to tracked kernel entry is going to be far less robust than relying strictly on soft
>> > context tracking.
>> >
>>
>> Why?
>>
>> Any exception that doesn't leave the context tracking state exactly
>> the way it found it is buggy.  That means that we need to make sure
>> that context tracking itself is safe wrt exceptions and that we need
>> to make sure that any exception that can happen early in entry is
>> itself safe.
>
> Right, and doing it the way we did previously was safe wrt. that.
>
> Can't we have exceptions slow path just like the way we do it in syscalls?
>
> Then the exception slow path would just do:
>
>     if TIF_NOHZ
>        ctx = exception_enter()
>     exception_handler()
>     if TIF_NOHZ
>        exception_exit(ctx)

What's the purpose of TIF_NOHZ right now?  For syscalls, it makes
sense, but is there any case in which TIF_NOHZ is set on one CPU but
not on another CPU?  It might make sense to get the performance back
using static keys instead of TIF_NOHZ.

If we switched back to exception_enter, we'd have to remember the
previous state, and, with a single exception right now, I think that's
unnecessary.

I think there are only three states we can be in at exception entry:
user (and user_mode(regs)), kernel (and kernel_mode(regs)), or
NMI-like.  In the user case, the new code is correct.  In the kernel
case, the new code is also correct.  In the NMI case (if we're nested
in an NMI or similar entry)) then it is and was the responsibility of
the NMI-like entry to call rcu_nmi_enter(), and things that nest
inside that shouldn't touch context tracking (with the possible
exception of calling rcu_nmi_enter() again).

In current -tip, there's a slight hole in this due to syscalls, and I'll fix it.

>
>>
>> The latter is annoying, but the entry code needs to deal with it
>> anyway.  For example, any exception early in NMI is currently really
>> bad.  Non-IST exceptions very early in SYSCALL are fatal.
>> Non-paranoid exceptions outside swapgs are fatal.  Etc.
>
> Sure but that doesn't mean I'm happy with introducing new fragile path
> like those. Especially as we have a way to fix without more overhead.

I think my approach can work with even less overhead: there are fewer
branches due to checking the previous state.

>> > Also as long as there is at least one instruction between entry to the kernel
>> > and context tracking noting it, there is a risk for an exception. Hence entry
>> > code will never be atomic enough to avoid this kind of bugs.
>>
>> By that argument, we're doomed.  Non-IST exceptions outside swapgs are fatal.
>
> Does that concern only error_entry() exceptions?

Yes, but the set of paranoid_entry exceptions is shrinking.  In -tip, there are:

NMI: NMI is special and will call rcu_nmi_enter().  Nothing's changing here.

MCE: Once upon a time, MCE was simply buggy.  As of 4.0 (IIRC) MCE
from kernel mode calls rcu_nmi_enter().

BP: This is going away, I think.  #BP should stop being special by 4.4.

DB: That's the only weird case.  Patches to prevent instruction
breakpoints in entry code are already in -tip.  The only thing left is
kernel watchpoints, and we need to do something about that.

>
>> >
>> > Heh if only we had something like local_exception_save()!
>>
>> What would that mean?
>>
>> Exceptions aren't magic asynchronous things.  They happen only when
>> you do something that can trigger an exception.
>
> Sure but, did you really never wish to have such an API? :-p

:)

-- 
Andy Lutomirski
AMA Capital Management, LLC
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists