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

Groups > linux.kernel > #1369948 > unrolled thread

[PATCH v5 4/9] x86/traps: Enable all exception handler callbacks early

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

  [PATCH v5 4/9] x86/traps: Enable all exception handler callbacks early Andy Lutomirski <luto@kernel.org> - 2016-04-02 16:10 +0200
    Re: [PATCH v5 4/9] x86/traps: Enable all exception handler callbacks  early Borislav Petkov <bp@alien8.de> - 2016-04-02 21:00 +0200
      Re: [PATCH v5 4/9] x86/traps: Enable all exception handler callbacks early Andy Lutomirski <luto@amacapital.net> - 2016-04-02 22:20 +0200
        Re: [PATCH v5 4/9] x86/traps: Enable all exception handler callbacks  early Borislav Petkov <bp@alien8.de> - 2016-04-02 23:00 +0200
          Re: [PATCH v5 4/9] x86/traps: Enable all exception handler callbacks  early Borislav Petkov <bp@alien8.de> - 2016-04-03 10:10 +0200
            Re: [PATCH v5 4/9] x86/traps: Enable all exception handler callbacks early Andy Lutomirski <luto@amacapital.net> - 2016-04-03 15:30 +0200
            Re: [PATCH v5 4/9] x86/traps: Enable all exception handler callbacks early Linus Torvalds <torvalds@linux-foundation.org> - 2016-04-03 16:00 +0200
              Re: [PATCH v5 4/9] x86/traps: Enable all exception handler callbacks early Andy Lutomirski <luto@amacapital.net> - 2016-04-03 16:00 +0200
                Re: [PATCH v5 4/9] x86/traps: Enable all exception handler callbacks early Linus Torvalds <torvalds@linux-foundation.org> - 2016-04-03 16:20 +0200
                Re: [PATCH v5 4/9] x86/traps: Enable all exception handler callbacks  early Borislav Petkov <bp@alien8.de> - 2016-04-03 16:20 +0200
                  Re: [PATCH v5 4/9] x86/traps: Enable all exception handler callbacks early Andy Lutomirski <luto@amacapital.net> - 2016-04-04 17:50 +0200
    [tip:x86/asm] x86/traps: Enable all exception handler callbacks  early tip-bot for Andy Lutomirski <tipbot@zytor.com> - 2016-04-13 13:50 +0200

#1369948 — [PATCH v5 4/9] x86/traps: Enable all exception handler callbacks early

Now that early_fixup_exception has pt_regs, we can just call
fixup_exception from it.  This will make fancy exception handlers
work early.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/mm/extable.c | 19 ++-----------------
 1 file changed, 2 insertions(+), 17 deletions(-)

diff --git a/arch/x86/mm/extable.c b/arch/x86/mm/extable.c
index 8997022abebc..50dfe438bd91 100644
--- a/arch/x86/mm/extable.c
+++ b/arch/x86/mm/extable.c
@@ -95,10 +95,6 @@ extern unsigned int early_recursion_flag;
 /* Restricted version used during very early boot */
 void __init early_fixup_exception(struct pt_regs *regs, int trapnr)
 {
-	const struct exception_table_entry *e;
-	unsigned long new_ip;
-	ex_handler_t handler;
-
 	/* Ignore early NMIs. */
 	if (trapnr == X86_TRAP_NMI)
 		return;
@@ -109,19 +105,8 @@ void __init early_fixup_exception(struct pt_regs *regs, int trapnr)
 	if (regs->cs != __KERNEL_CS)
 		goto fail;
 
-	e = search_exception_tables(regs->ip);
-	if (!e)
-		goto fail;
-
-	new_ip  = ex_fixup_addr(e);
-	handler = ex_fixup_handler(e);
-
-	/* special handling not supported during early boot */
-	if (handler != ex_handler_default)
-		goto fail;
-
-	regs->ip = new_ip;
-	return;
+	if (fixup_exception(regs, trapnr))
+		return;
 
 fail:
 	early_printk("PANIC: early exception 0x%02x IP %lx:%lx error %lx cr2 0x%lx\n",
-- 
2.5.5

[toc] | [next] | [standalone]

#1370033 — Re: [PATCH v5 4/9] x86/traps: Enable all exception handler callbacks early

On Sat, Apr 02, 2016 at 07:01:35AM -0700, Andy Lutomirski wrote:
> Now that early_fixup_exception has pt_regs, we can just call
> fixup_exception from it.  This will make fancy exception handlers
> work early.
> 
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  arch/x86/mm/extable.c | 19 ++-----------------
>  1 file changed, 2 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/x86/mm/extable.c b/arch/x86/mm/extable.c
> index 8997022abebc..50dfe438bd91 100644
> --- a/arch/x86/mm/extable.c
> +++ b/arch/x86/mm/extable.c
> @@ -95,10 +95,6 @@ extern unsigned int early_recursion_flag;
>  /* Restricted version used during very early boot */
>  void __init early_fixup_exception(struct pt_regs *regs, int trapnr)
>  {
> -	const struct exception_table_entry *e;
> -	unsigned long new_ip;
> -	ex_handler_t handler;
> -
>  	/* Ignore early NMIs. */
>  	if (trapnr == X86_TRAP_NMI)
>  		return;
> @@ -109,19 +105,8 @@ void __init early_fixup_exception(struct pt_regs *regs, int trapnr)
>  	if (regs->cs != __KERNEL_CS)
>  		goto fail;
>  
> -	e = search_exception_tables(regs->ip);
> -	if (!e)
> -		goto fail;
> -
> -	new_ip  = ex_fixup_addr(e);
> -	handler = ex_fixup_handler(e);
> -
> -	/* special handling not supported during early boot */
> -	if (handler != ex_handler_default)
> -		goto fail;

Hold on, what happened to the uaccess handling not being supported
during early boot?

So before Tony changed it, the original code had:


 /* Restricted version used during very early boot */
 int __init early_fixup_exception(unsigned long *ip)
 {

...
-               if (fixup->fixup - fixup->insn >= 0x7ffffff0 - 4) {
-                       /* uaccess handling not supported during early boot */
-                       return 0;
-               }

I'm guessing that wasn't supported early, probably because some stuff
wasn't initialized yet. Our normal, late fixup is by doing:

        current_thread_info()->uaccess_err = 1;

and I'm assuming we can't do that early. current_thread_info is probably
not setup yet...

> -
> -	regs->ip = new_ip;
> -	return;
> +	if (fixup_exception(regs, trapnr))
> +		return;

So why can we do it now, all of a sudden?

/me is scratching head.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

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

#1370042

On Sat, Apr 2, 2016 at 11:52 AM, Borislav Petkov <bp@alien8.de> wrote:
> On Sat, Apr 02, 2016 at 07:01:35AM -0700, Andy Lutomirski wrote:
>> Now that early_fixup_exception has pt_regs, we can just call
>> fixup_exception from it.  This will make fancy exception handlers
>> work early.
>>
>> Signed-off-by: Andy Lutomirski <luto@kernel.org>
>> ---
>>  arch/x86/mm/extable.c | 19 ++-----------------
>>  1 file changed, 2 insertions(+), 17 deletions(-)
>>
>> diff --git a/arch/x86/mm/extable.c b/arch/x86/mm/extable.c
>> index 8997022abebc..50dfe438bd91 100644
>> --- a/arch/x86/mm/extable.c
>> +++ b/arch/x86/mm/extable.c
>> @@ -95,10 +95,6 @@ extern unsigned int early_recursion_flag;
>>  /* Restricted version used during very early boot */
>>  void __init early_fixup_exception(struct pt_regs *regs, int trapnr)
>>  {
>> -     const struct exception_table_entry *e;
>> -     unsigned long new_ip;
>> -     ex_handler_t handler;
>> -
>>       /* Ignore early NMIs. */
>>       if (trapnr == X86_TRAP_NMI)
>>               return;
>> @@ -109,19 +105,8 @@ void __init early_fixup_exception(struct pt_regs *regs, int trapnr)
>>       if (regs->cs != __KERNEL_CS)
>>               goto fail;
>>
>> -     e = search_exception_tables(regs->ip);
>> -     if (!e)
>> -             goto fail;
>> -
>> -     new_ip  = ex_fixup_addr(e);
>> -     handler = ex_fixup_handler(e);
>> -
>> -     /* special handling not supported during early boot */
>> -     if (handler != ex_handler_default)
>> -             goto fail;
>
> Hold on, what happened to the uaccess handling not being supported
> during early boot?
>
> So before Tony changed it, the original code had:
>
>
>  /* Restricted version used during very early boot */
>  int __init early_fixup_exception(unsigned long *ip)
>  {
>
> ...
> -               if (fixup->fixup - fixup->insn >= 0x7ffffff0 - 4) {
> -                       /* uaccess handling not supported during early boot */
> -                       return 0;
> -               }
>
> I'm guessing that wasn't supported early, probably because some stuff
> wasn't initialized yet. Our normal, late fixup is by doing:
>
>         current_thread_info()->uaccess_err = 1;
>
> and I'm assuming we can't do that early. current_thread_info is probably
> not setup yet...
>
>> -
>> -     regs->ip = new_ip;
>> -     return;
>> +     if (fixup_exception(regs, trapnr))
>> +             return;
>
> So why can we do it now, all of a sudden?

I have no idea why it was explicitly unsupported, but I'm guessing it
was just to avoid duplicating the code.  Early "ext" uaccess failures
are certainly not going to work, but I don't think this is a problem
-- there's no userspace before trap_init runs, so how exactly is an
"ext" uaccess going to happen in the first place?

In any event, if it did happen in older kernels, it would have
immediately panicked due to that code.  At least with my code it just
might manage to EFAULT correctly.

--Andy

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

#1370051 — Re: [PATCH v5 4/9] x86/traps: Enable all exception handler callbacks early

On Sat, Apr 02, 2016 at 01:16:07PM -0700, Andy Lutomirski wrote:
> I have no idea why it was explicitly unsupported, but I'm guessing it
> was just to avoid duplicating the code.  Early "ext" uaccess failures
> are certainly not going to work, but I don't think this is a problem
> -- there's no userspace before trap_init runs, so how exactly is an
> "ext" uaccess going to happen in the first place?
> 
> In any event, if it did happen in older kernels, it would have
> immediately panicked due to that code.  At least with my code it just
> might manage to EFAULT correctly.

Yeah, I was wondering what that early thing meant.

Linus or tip guys probably remember what this whole deal with early
uaccess was about. I'll try to do some git archeology tomorrow.

In any event, it would be a good idea IMO, to hold that situation down
in a comment somewhere. Once we've figured it out...

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

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

#1370127 — Re: [PATCH v5 4/9] x86/traps: Enable all exception handler callbacks early

On Sat, Apr 02, 2016 at 10:52:48PM +0200, Borislav Petkov wrote:
> On Sat, Apr 02, 2016 at 01:16:07PM -0700, Andy Lutomirski wrote:
> > I have no idea why it was explicitly unsupported, but I'm guessing it
> > was just to avoid duplicating the code.  Early "ext" uaccess failures
> > are certainly not going to work, but I don't think this is a problem
> > -- there's no userspace before trap_init runs, so how exactly is an
> > "ext" uaccess going to happen in the first place?
> > 
> > In any event, if it did happen in older kernels, it would have
> > immediately panicked due to that code.  At least with my code it just
> > might manage to EFAULT correctly.
> 
> Yeah, I was wondering what that early thing meant.
> 
> Linus or tip guys probably remember what this whole deal with early
> uaccess was about. I'll try to do some git archeology tomorrow.

Yep, just as I suspected:

6a1ea279c210 ("x86, extable: Add early_fixup_exception()")

Apparently, thread_info might not have been setup yet. I'm guessing the
intention behind this no-uaccess-fixup-early is to not even attempt any
fixup due to stuff *probably* not initialized yet and so the safer thing
would be to panic instead.

I'm wondering whether making it try to EFAULT correctly is the right
thing to do... We're certainly more conservative if we panic and not
allow some silently failed attempt at recovery which looks successful,
to continue.

hpa, thoughts?

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

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

#1370219

On Sun, Apr 3, 2016 at 1:07 AM, Borislav Petkov <bp@alien8.de> wrote:
> On Sat, Apr 02, 2016 at 10:52:48PM +0200, Borislav Petkov wrote:
>> On Sat, Apr 02, 2016 at 01:16:07PM -0700, Andy Lutomirski wrote:
>> > I have no idea why it was explicitly unsupported, but I'm guessing it
>> > was just to avoid duplicating the code.  Early "ext" uaccess failures
>> > are certainly not going to work, but I don't think this is a problem
>> > -- there's no userspace before trap_init runs, so how exactly is an
>> > "ext" uaccess going to happen in the first place?
>> >
>> > In any event, if it did happen in older kernels, it would have
>> > immediately panicked due to that code.  At least with my code it just
>> > might manage to EFAULT correctly.
>>
>> Yeah, I was wondering what that early thing meant.
>>
>> Linus or tip guys probably remember what this whole deal with early
>> uaccess was about. I'll try to do some git archeology tomorrow.
>
> Yep, just as I suspected:
>
> 6a1ea279c210 ("x86, extable: Add early_fixup_exception()")
>
> Apparently, thread_info might not have been setup yet. I'm guessing the
> intention behind this no-uaccess-fixup-early is to not even attempt any
> fixup due to stuff *probably* not initialized yet and so the safer thing
> would be to panic instead.
>
> I'm wondering whether making it try to EFAULT correctly is the right
> thing to do... We're certainly more conservative if we panic and not
> allow some silently failed attempt at recovery which looks successful,
> to continue.
>
> hpa, thoughts?

I don't think this matters much.  There aren't many users of this
mechanism in the tree:

./arch/x86/kernel/signal.c:    get_user_try {
./arch/x86/kernel/signal.c:    put_user_try {
./arch/x86/kernel/signal.c:    put_user_try {
./arch/x86/kernel/signal.c:    put_user_try {
./arch/x86/kernel/signal.c:    put_user_try {
./arch/x86/kernel/signal_compat.c:    put_user_try {
./arch/x86/kernel/signal_compat.c:    get_user_try {
./arch/x86/kernel/vm86_32.c:    put_user_try {
./arch/x86/kernel/vm86_32.c:    get_user_try {
./arch/x86/ia32/ia32_signal.c:    get_user_try {
./arch/x86/ia32/ia32_signal.c:    put_user_try {
./arch/x86/ia32/ia32_signal.c:    put_user_try {
./arch/x86/ia32/ia32_signal.c:    put_user_try {
./arch/x86/include/asm/uaccess.h: * {get|put}_user_try and catch
./arch/x86/include/asm/uaccess.h: * get_user_try {
./arch/x86/include/asm/uaccess.h:#define get_user_try        uaccess_try
./arch/x86/include/asm/uaccess.h:#define put_user_try        uaccess_try

I don't see how we could get to that code in the first place without
current_thread_info() working.

If we can ever convince gcc to do jump labels properly for uaccess, it
would probably be better to just delete all that code.

--Andy

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

#1370221

On Sun, Apr 3, 2016 at 3:07 AM, Borislav Petkov <bp@alien8.de> wrote:
>
> I'm wondering whether making it try to EFAULT correctly is the right
> thing to do... We're certainly more conservative if we panic and not
> allow some silently failed attempt at recovery which looks successful,
> to continue.

No, please don't fail at early boot.

Early boot is just about the *worst* situation to try to debug odd
failures, exactly since things like printk may not be reliable, and
things won't get logged etc.

So particularly during early boot we should try as hard as possible
not to crash - even if it means not being able to log about a problem.
At least that way you have a hopefully working machine and can *maybe*
debug things.

            Linus

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

#1370222

On Sun, Apr 3, 2016 at 6:51 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Sun, Apr 3, 2016 at 3:07 AM, Borislav Petkov <bp@alien8.de> wrote:
>>
>> I'm wondering whether making it try to EFAULT correctly is the right
>> thing to do... We're certainly more conservative if we panic and not
>> allow some silently failed attempt at recovery which looks successful,
>> to continue.
>
> No, please don't fail at early boot.
>
> Early boot is just about the *worst* situation to try to debug odd
> failures, exactly since things like printk may not be reliable, and
> things won't get logged etc.
>
> So particularly during early boot we should try as hard as possible
> not to crash - even if it means not being able to log about a problem.
> At least that way you have a hopefully working machine and can *maybe*
> debug things.
>

In this regard, at least, my patch is the right approach.  Calling the
handler, whatever it is, is less likely to panic than refusing to call
it.

--Andy

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

#1370229

On Sun, Apr 3, 2016 at 8:55 AM, Andy Lutomirski <luto@amacapital.net> wrote:
>
> In this regard, at least, my patch is the right approach.  Calling the
> handler, whatever it is, is less likely to panic than refusing to call
> it.

Agreed, the patch series looks like a good thing in general.

            Linus

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

#1370230 — Re: [PATCH v5 4/9] x86/traps: Enable all exception handler callbacks early

On Sun, Apr 03, 2016 at 06:55:00AM -0700, Andy Lutomirski wrote:
> > No, please don't fail at early boot.
> >
> > Early boot is just about the *worst* situation to try to debug odd
> > failures, exactly since things like printk may not be reliable, and
> > things won't get logged etc.
> >
> > So particularly during early boot we should try as hard as possible
> > not to crash - even if it means not being able to log about a problem.
> > At least that way you have a hopefully working machine and can *maybe*
> > debug things.
> >
> 
> In this regard, at least, my patch is the right approach.  Calling the
> handler, whatever it is, is less likely to panic than refusing to call
> it.

Ok, good.

But can we pretty please document this whole situation, i.e., the
fact that we're trying really really hard not to fail early boot for
debuggability reasons - either in the commit message or better in the
code, for future reference. I think this is an important aspect to hold
down.

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

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

#1370714

On Sun, Apr 3, 2016 at 7:10 AM, Borislav Petkov <bp@alien8.de> wrote:
> On Sun, Apr 03, 2016 at 06:55:00AM -0700, Andy Lutomirski wrote:
>> > No, please don't fail at early boot.
>> >
>> > Early boot is just about the *worst* situation to try to debug odd
>> > failures, exactly since things like printk may not be reliable, and
>> > things won't get logged etc.
>> >
>> > So particularly during early boot we should try as hard as possible
>> > not to crash - even if it means not being able to log about a problem.
>> > At least that way you have a hopefully working machine and can *maybe*
>> > debug things.
>> >
>>
>> In this regard, at least, my patch is the right approach.  Calling the
>> handler, whatever it is, is less likely to panic than refusing to call
>> it.
>
> Ok, good.
>
> But can we pretty please document this whole situation, i.e., the
> fact that we're trying really really hard not to fail early boot for
> debuggability reasons - either in the commit message or better in the
> code, for future reference. I think this is an important aspect to hold
> down.

I emailed out a followup patch to add a comment.

--Andy

>
> Thanks.
>
> --
> Regards/Gruss,
>     Boris.
>
> ECO tip #101: Trim your mails when you reply.



-- 
Andy Lutomirski
AMA Capital Management, LLC

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

#1377833 — [tip:x86/asm] x86/traps: Enable all exception handler callbacks early

Commit-ID:  ae7ef45e12354a1e2f6013b46df0c9f5bbb6ffbe
Gitweb:     http://git.kernel.org/tip/ae7ef45e12354a1e2f6013b46df0c9f5bbb6ffbe
Author:     Andy Lutomirski <luto@kernel.org>
AuthorDate: Sat, 2 Apr 2016 07:01:35 -0700
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 13 Apr 2016 11:37:45 +0200

x86/traps: Enable all exception handler callbacks early

Now that early_fixup_exception() has pt_regs, we can just call
fixup_exception() from it.  This will make fancy exception handlers
work early.

Tested-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Arjan van de Ven <arjan@linux.intel.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: KVM list <kvm@vger.kernel.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: xen-devel <Xen-devel@lists.xen.org>
Link: http://lkml.kernel.org/r/20fc047d926150cb08cb9b9f2923519b07ec1a15.1459605520.git.luto@kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/mm/extable.c | 19 ++-----------------
 1 file changed, 2 insertions(+), 17 deletions(-)

diff --git a/arch/x86/mm/extable.c b/arch/x86/mm/extable.c
index da442f3..061a237 100644
--- a/arch/x86/mm/extable.c
+++ b/arch/x86/mm/extable.c
@@ -88,10 +88,6 @@ extern unsigned int early_recursion_flag;
 /* Restricted version used during very early boot */
 void __init early_fixup_exception(struct pt_regs *regs, int trapnr)
 {
-	const struct exception_table_entry *e;
-	unsigned long new_ip;
-	ex_handler_t handler;
-
 	/* Ignore early NMIs. */
 	if (trapnr == X86_TRAP_NMI)
 		return;
@@ -102,19 +98,8 @@ void __init early_fixup_exception(struct pt_regs *regs, int trapnr)
 	if (regs->cs != __KERNEL_CS)
 		goto fail;
 
-	e = search_exception_tables(regs->ip);
-	if (!e)
-		goto fail;
-
-	new_ip  = ex_fixup_addr(e);
-	handler = ex_fixup_handler(e);
-
-	/* special handling not supported during early boot */
-	if (handler != ex_handler_default)
-		goto fail;
-
-	regs->ip = new_ip;
-	return;
+	if (fixup_exception(regs, trapnr))
+		return;
 
 fail:
 	early_printk("PANIC: early exception 0x%02x IP %lx:%lx error %lx cr2 0x%lx\n",

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web