Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1205470 > unrolled thread

Re: [tip:x86/asm] x86/asm/entry/64: Migrate error and IRQ exit work to C and remove old assembly code

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [tip:x86/asm] x86/asm/entry/64: Migrate error and IRQ exit work  to C and remove old assembly code Frederic Weisbecker <fweisbec@gmail.com> - 2015-08-12 00:40 +0200
    Re: [tip:x86/asm] x86/asm/entry/64: Migrate error and IRQ exit work  to C and remove old assembly code Andy Lutomirski <luto@amacapital.net> - 2015-08-12 01:00 +0200
      Re: [tip:x86/asm] x86/asm/entry/64: Migrate error and IRQ exit work  to C and remove old assembly code Frederic Weisbecker <fweisbec@gmail.com> - 2015-08-12 01:30 +0200
        Re: [tip:x86/asm] x86/asm/entry/64: Migrate error and IRQ exit work  to C and remove old assembly code Andy Lutomirski <luto@amacapital.net> - 2015-08-12 01:40 +0200
          Re: [tip:x86/asm] x86/asm/entry/64: Migrate error and IRQ exit work  to C and remove old assembly code Frederic Weisbecker <fweisbec@gmail.com> - 2015-08-12 15:40 +0200
            Re: [tip:x86/asm] x86/asm/entry/64: Migrate error and IRQ exit work  to C and remove old assembly code Andy Lutomirski <luto@amacapital.net> - 2015-08-12 17:10 +0200
              Re: [tip:x86/asm] x86/asm/entry/64: Migrate error and IRQ exit work  to C and remove old assembly code Frederic Weisbecker <fweisbec@gmail.com> - 2015-08-19 00:40 +0200
                Re: [tip:x86/asm] x86/asm/entry/64: Migrate error and IRQ exit work  to C and remove old assembly code Andy Lutomirski <luto@amacapital.net> - 2015-08-19 00:50 +0200
                  Re: [tip:x86/asm] x86/asm/entry/64: Migrate error and IRQ exit work  to C and remove old assembly code Frederic Weisbecker <fweisbec@gmail.com> - 2015-08-19 19:20 +0200
                    Re: [tip:x86/asm] x86/asm/entry/64: Migrate error and IRQ exit work  to C and remove old assembly code Andy Lutomirski <luto@amacapital.net> - 2015-08-19 20:10 +0200

#1205470 — Re: [tip:x86/asm] x86/asm/entry/64: Migrate error and IRQ exit work to C and remove old assembly code

On Tue, Jul 07, 2015 at 03:53:29AM -0700, tip-bot for Andy Lutomirski wrote:
> Commit-ID:  02bc7768fe447ae305e924b931fa629073a4a1b9
> Gitweb:     http://git.kernel.org/tip/02bc7768fe447ae305e924b931fa629073a4a1b9
> Author:     Andy Lutomirski <luto@kernel.org>
> AuthorDate: Fri, 3 Jul 2015 12:44:31 -0700
> Committer:  Ingo Molnar <mingo@kernel.org>
> CommitDate: Tue, 7 Jul 2015 10:59:08 +0200
> 
> x86/asm/entry/64: Migrate error and IRQ exit work to C and remove old assembly code
> 
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> Cc: Andy Lutomirski <luto@amacapital.net>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Brian Gerst <brgerst@gmail.com>
> Cc: Denys Vlasenko <dvlasenk@redhat.com>
> Cc: Denys Vlasenko <vda.linux@googlemail.com>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: H. Peter Anvin <hpa@zytor.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Rik van Riel <riel@redhat.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: paulmck@linux.vnet.ibm.com
> Link: http://lkml.kernel.org/r/60e90901eee611e59e958bfdbbe39969b4f88fe5.1435952415.git.luto@kernel.org
> Signed-off-by: Ingo Molnar <mingo@kernel.org>
> ---
>  arch/x86/entry/entry_64.S        | 64 +++++++++++-----------------------------
>  arch/x86/entry/entry_64_compat.S |  5 ++++
>  2 files changed, 23 insertions(+), 46 deletions(-)
> 
> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
> index 83eb63d..168ee26 100644
> --- a/arch/x86/entry/entry_64.S
> +++ b/arch/x86/entry/entry_64.S
> @@ -1088,7 +1055,12 @@ ENTRY(error_entry)
>  	SWAPGS
>  
>  .Lerror_entry_from_usermode_after_swapgs:
> +#ifdef CONFIG_CONTEXT_TRACKING
> +	call enter_from_user_mode
> +#endif

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.

In early context tracking days we have relied on CS. But I changed that because of such
issue. The only reliable source for soft context tracking is the soft context tracking itself.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1205479

On Tue, Aug 11, 2015 at 3:38 PM, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> On Tue, Jul 07, 2015 at 03:53:29AM -0700, tip-bot for Andy Lutomirski wrote:
>> Commit-ID:  02bc7768fe447ae305e924b931fa629073a4a1b9
>> Gitweb:     http://git.kernel.org/tip/02bc7768fe447ae305e924b931fa629073a4a1b9
>> Author:     Andy Lutomirski <luto@kernel.org>
>> AuthorDate: Fri, 3 Jul 2015 12:44:31 -0700
>> Committer:  Ingo Molnar <mingo@kernel.org>
>> CommitDate: Tue, 7 Jul 2015 10:59:08 +0200
>>
>> x86/asm/entry/64: Migrate error and IRQ exit work to C and remove old assembly code
>>
>> Signed-off-by: Andy Lutomirski <luto@kernel.org>
>> Cc: Andy Lutomirski <luto@amacapital.net>
>> Cc: Borislav Petkov <bp@alien8.de>
>> Cc: Brian Gerst <brgerst@gmail.com>
>> Cc: Denys Vlasenko <dvlasenk@redhat.com>
>> Cc: Denys Vlasenko <vda.linux@googlemail.com>
>> Cc: Frederic Weisbecker <fweisbec@gmail.com>
>> Cc: H. Peter Anvin <hpa@zytor.com>
>> Cc: Kees Cook <keescook@chromium.org>
>> Cc: Linus Torvalds <torvalds@linux-foundation.org>
>> Cc: Oleg Nesterov <oleg@redhat.com>
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> Cc: Rik van Riel <riel@redhat.com>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: paulmck@linux.vnet.ibm.com
>> Link: http://lkml.kernel.org/r/60e90901eee611e59e958bfdbbe39969b4f88fe5.1435952415.git.luto@kernel.org
>> Signed-off-by: Ingo Molnar <mingo@kernel.org>
>> ---
>>  arch/x86/entry/entry_64.S        | 64 +++++++++++-----------------------------
>>  arch/x86/entry/entry_64_compat.S |  5 ++++
>>  2 files changed, 23 insertions(+), 46 deletions(-)
>>
>> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
>> index 83eb63d..168ee26 100644
>> --- a/arch/x86/entry/entry_64.S
>> +++ b/arch/x86/entry/entry_64.S
>> @@ -1088,7 +1055,12 @@ ENTRY(error_entry)
>>       SWAPGS
>>
>>  .Lerror_entry_from_usermode_after_swapgs:
>> +#ifdef CONFIG_CONTEXT_TRACKING
>> +     call enter_from_user_mode
>> +#endif
>
> 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.

But the old code had the same issue.  If we got an exception (the most
likely one is probably a vmalloc fault) during user_exit and we then
hit exception_enter, the result would probably be bad.

>
> In early context tracking days we have relied on CS. But I changed that because of such
> issue. The only reliable source for soft context tracking is the soft context tracking itself.

I don't see why the soft state is more reliable.  The only bad case is
where the entry itself (HW entry up to user_exit) is not atomic
enough, but that path should be at least as atomic as user_exit itself
is.

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

[toc] | [prev] | [next] | [standalone]

#1205500

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@gmail.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.

The resulting bugs are rare and very hard to reproduce and diagnose.

> 
> But the old code had the same issue.  If we got an exception (the most
> likely one is probably a vmalloc fault) during user_exit and we then
> hit exception_enter, the result would probably be bad.

We have a recursion protection in context tracking that should protect against
exceptions triggering in the middle of half-set states.

> 
> >
> > In early context tracking days we have relied on CS. But I changed that because of such
> > issue. The only reliable source for soft context tracking is the soft context tracking itself.
> 
> I don't see why the soft state is more reliable.  The only bad case is
> where the entry itself (HW entry up to user_exit) is not atomic
> enough, but that path should be at least as atomic as user_exit itself
> is.

Note it's not only about entry code up to user_exit() but also about
user_enter() up to iret.

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.

Heh if only we had something like local_exception_save()!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1205503

On Tue, Aug 11, 2015 at 4:22 PM, Frederic Weisbecker <fweisbec@gmail.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@gmail.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.

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.

> The resulting bugs are rare and very hard to reproduce and diagnose.

That's why I stuck assertions all over the place.  I know of exactly
one case that will trip the assertion, and it's a false positive and I
plan on fixing it soon.

>
>>
>> But the old code had the same issue.  If we got an exception (the most
>> likely one is probably a vmalloc fault) during user_exit and we then
>> hit exception_enter, the result would probably be bad.
>
> We have a recursion protection in context tracking that should protect against
> exceptions triggering in the middle of half-set states.

I sure hope so.  It would be nice to mark it with with nokprobes, etc
if needed, too.

>
>>
>> >
>> > In early context tracking days we have relied on CS. But I changed that because of such
>> > issue. The only reliable source for soft context tracking is the soft context tracking itself.
>>
>> I don't see why the soft state is more reliable.  The only bad case is
>> where the entry itself (HW entry up to user_exit) is not atomic
>> enough, but that path should be at least as atomic as user_exit itself
>> is.
>
> Note it's not only about entry code up to user_exit() but also about
> user_enter() up to iret.
>

We already need to block interrupts there, and the code for exit back
to userspace is very clean in -tip.

> 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.

>
> 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.

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

[toc] | [prev] | [next] | [standalone]

#1206097

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@gmail.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@gmail.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)

Right now we are calling unconditionally the context tracking code, which is
not good.

> 
> 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.

> 
> > The resulting bugs are rare and very hard to reproduce and diagnose.
> 
> That's why I stuck assertions all over the place.  I know of exactly
> one case that will trip the assertion, and it's a false positive and I
> plan on fixing it soon.
> 
> >
> >>
> >> But the old code had the same issue.  If we got an exception (the most
> >> likely one is probably a vmalloc fault) during user_exit and we then
> >> hit exception_enter, the result would probably be bad.
> >
> > We have a recursion protection in context tracking that should protect against
> > exceptions triggering in the middle of half-set states.
> 
> I sure hope so.  It would be nice to mark it with with nokprobes, etc
> if needed, too.

Sure.

> > 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?

> >
> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1206201

On Wed, Aug 12, 2015 at 6:32 AM, Frederic Weisbecker <fweisbec@gmail.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@gmail.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@gmail.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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1209524

On Wed, Aug 12, 2015 at 07:59:44AM -0700, Andy Lutomirski wrote:
> On Wed, Aug 12, 2015 at 6:32 AM, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > 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.

Sure if we can manage to do that. The nice thing about TIF flags is that
they are a single check that is always there.

> 
> 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.

But we can have user && (!user_mode(regs)) if exception happens on exception
entry code.

> 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.

There must be a check for context tracking enabled anyway. So why can't
we just just do in exception entry code:

       if (exception_slow_path()) {
           exception_enter()
           exception_handler()
           exception_exit()
       } else {
           normal stuff
       }

Especially if we can manage to implement static keys in ASM, this will sum up to
a single one.

> >> 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.

So now we can't set a breakpoint on syscall entry anymore?

I'm still nervous with all that.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1209531

On Tue, Aug 18, 2015 at 3:34 PM, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> On Wed, Aug 12, 2015 at 07:59:44AM -0700, Andy Lutomirski wrote:
>> On Wed, Aug 12, 2015 at 6:32 AM, Frederic Weisbecker <fweisbec@gmail.com> wrote:
>> > 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.
>
> Sure if we can manage to do that. The nice thing about TIF flags is that
> they are a single check that is always there.
>

True, although my patch loses that benefit for the fast compat entries
due to the syscall arg fault stuff (what a mess!).

>>
>> 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.
>
> But we can have user && (!user_mode(regs)) if exception happens on exception
> entry code.

I sure hope not, unless it nests inside an NMI-like thing.  It's
conceivable that this might happen due to perf NMIs causing a failed
MSR read or similar.  We might need to relax the assertions to check
that we're either in kernel or NMI context.  If so, that's
straightforward.  Meanwhile no one has reported this happening.

>
>> 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.
>
> There must be a check for context tracking enabled anyway. So why can't
> we just just do in exception entry code:
>
>        if (exception_slow_path()) {
>            exception_enter()
>            exception_handler()
>            exception_exit()
>        } else {
>            normal stuff
>        }
>
> Especially if we can manage to implement static keys in ASM, this will sum up to
> a single one.

There isn't really an exception slow path.  There's already a branch
for user vs kernel (in the CPL sense), and with my patches, there's no
additional branch for previous context tracking state.

>
>> >> 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.
>
> So now we can't set a breakpoint on syscall entry anymore?
>
> I'm still nervous with all that.

We haven't done anything that would make breakpoints on syscall entry
less safe than they were, but we now disallow the breakpoints.  In the
future, we might take advantage of that change.

-- 
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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1209959

On Tue, Aug 18, 2015 at 03:40:20PM -0700, Andy Lutomirski wrote:
> On Tue, Aug 18, 2015 at 3:34 PM, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> >> 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.
> >
> > But we can have user && (!user_mode(regs)) if exception happens on exception
> > entry code.
> 
> I sure hope not, unless it nests inside an NMI-like thing.  It's
> conceivable that this might happen due to perf NMIs causing a failed
> MSR read or similar.  We might need to relax the assertions to check
> that we're either in kernel or NMI context.  If so, that's
> straightforward.  Meanwhile no one has reported this happening.

But we can still have #DB on entry code right? We blocked breakpoints on entry
code (I still don't get why and it looks to me like an overkill) but we still
have watchpoints.

> 
> >
> >> 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.
> >
> > There must be a check for context tracking enabled anyway. So why can't
> > we just just do in exception entry code:
> >
> >        if (exception_slow_path()) {
> >            exception_enter()
> >            exception_handler()
> >            exception_exit()
> >        } else {
> >            normal stuff
> >        }
> >
> > Especially if we can manage to implement static keys in ASM, this will sum up to
> > a single one.
> 
> There isn't really an exception slow path.  There's already a branch
> for user vs kernel (in the CPL sense), and with my patches, there's no
> additional branch for previous context tracking state.

But an exception slow path based on static key would the most lightweight
thing for context tracking off-case (which is 99.9999% of usecases) and we
would keep it robust (ie: no need to enumerate all the fragile non-possibility
for an exception in entry code to get it safe).

> > So now we can't set a breakpoint on syscall entry anymore?
> >
> > I'm still nervous with all that.
> 
> We haven't done anything that would make breakpoints on syscall entry
> less safe than they were, but we now disallow the breakpoints.  In the
> future, we might take advantage of that change.

I still don't get the reason of that.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1209973

On Wed, Aug 19, 2015 at 10:18 AM, Frederic Weisbecker
<fweisbec@gmail.com> wrote:
> On Tue, Aug 18, 2015 at 03:40:20PM -0700, Andy Lutomirski wrote:
>>
>> I sure hope not, unless it nests inside an NMI-like thing.  It's
>> conceivable that this might happen due to perf NMIs causing a failed
>> MSR read or similar.  We might need to relax the assertions to check
>> that we're either in kernel or NMI context.  If so, that's
>> straightforward.  Meanwhile no one has reported this happening.
>
> But we can still have #DB on entry code right? We blocked breakpoints on entry
> code (I still don't get why and it looks to me like an overkill) but we still
> have watchpoints.

The actual reason is buried in the many threads about NMIs.
Basically, we want to start using RET to return from exceptions to
contexts with IF=0, but we can't do that if we need RF to work
correctly, and we need RF to work correctly if we allow breakpoints in
entry asm (otherwise we risk random infinite loops).  So we're
disallowing breakpoints in entry asm.

> But an exception slow path based on static key would the most lightweight
> thing for context tracking off-case (which is 99.9999% of usecases) and we
> would keep it robust (ie: no need to enumerate all the fragile non-possibility
> for an exception in entry code to get it safe).
>

IRQs work more or less like this in -tip (restructured, but this gets the gist):

if (user_mode(regs)) {
  swapgs;
  enter_from_user_mode;
  do_IRQ;
  prepare_exit_to_usermode;
  swapgs;
  iret;
} else {
  do_IRQ;
  check for preemption;
  iret;
}

In 4.2 and before, the enter_from_user_mode wasn't there, and instead
of calling prepare_exit_to_usermode in a known context
(CONTEXT_KERNEL), we went through the maze of retint_user in an
unknown context.  That meant that we needed things like SCHEDULE_USER
(which had a bug at some point), do_notify_resume (probably had tons
of bugs), etc, and somehow we still needed to end up in CONTEXT_USER
at the end.

I think the new state of affairs is much nicer.  It means that we
finally actually know what state we're in throughout the entry asm.
The only real downsides that I can see are:

1. There's an unnecessary pair of branches due to rcu_irq_enter and
rcu_irq_exit when an IRQ hits user mode.

2. If user_exit is indeed much more expensive than rcu_irq_enter, then
we pay that cost.

If you have suggestions for how to make this faster without making it
uglier, please let me know. :)

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web