Improve non-"safe" MSR access failure handling

This removes a bunch of assembly and adds some C code instead. It changes the actual printouts on both 32-bit and 64-bit kernels, but they still seem okay.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/include/asm/uaccess.h |  2 +-
 arch/x86/kernel/head_32.S      | 49 +++++-------------------------------------
 arch/x86/kernel/head_64.S      | 45 ++------------------------------------
 arch/x86/mm/extable.c          | 29 ++++++++++++++++++++-----
 4 files changed, 32 insertions(+), 93 deletions(-)

arch/x86/mm/extable.c +9 -3

@@ -99,21 +101,38 @@ int __init early_fixup_exception(struct pt_regs *regs, int trapnr)
 
 	/* Ignore early NMIs. */
 	if (trapnr == X86_TRAP_NMI)
-		return 1;
+		return;
+
+	if (early_recursion_flag > 2)
+		goto halt_loop;
+
+	if (regs->cs != __KERNEL_CS)
+		goto fail;
 
 	e = search_exception_tables(regs->ip);
 	if (!e)
-		return 0;
+		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)
-		return 0;
+		goto fail;
 

This removes a bunch of assembly and adds some C code instead. It changes the actual printouts on both 32-bit and 64-bit kernels, but they still seem okay.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/include/asm/uaccess.h |  2 +-
 arch/x86/kernel/head_32.S      | 49 +++++-------------------------------------
 arch/x86/kernel/head_64.S      | 45 ++------------------------------------
 arch/x86/mm/extable.c          | 29 ++++++++++++++++++++-----
 4 files changed, 32 insertions(+), 93 deletions(-)

arch/x86/mm/extable.c +9 -3

@@ -99,21 +101,38 @@ int __init early_fixup_exception(struct pt_regs *regs, int trapnr)
 
 	/* Ignore early NMIs. */
 	if (trapnr == X86_TRAP_NMI)
-		return 1;
+		return;
+
+	if (early_recursion_flag > 2)
+		goto halt_loop;
+
+	if (regs->cs != __KERNEL_CS)
+		goto fail;
 
 	e = search_exception_tables(regs->ip);
 	if (!e)
-		return 0;
+		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)
-		return 0;
+		goto fail;
 

Quoted 11 lines · Andy Lutomirski

This removes a bunch of assembly and adds some C code instead. It changes the actual printouts on both 32-bit and 64-bit kernels, but they still seem okay.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/include/asm/uaccess.h |  2 +-
 arch/x86/kernel/head_32.S      | 49 +++++-------------------------------------
 arch/x86/kernel/head_64.S      | 45 ++------------------------------------
 arch/x86/mm/extable.c          | 29 ++++++++++++++++++++-----
 4 files changed, 32 insertions(+), 93 deletions(-)

...

Quoted 26 lines

arch/x86/mm/extable.c +9 -3

@@ -99,21 +101,38 @@ int __init early_fixup_exception(struct pt_regs *regs, int trapnr)
 
      /* Ignore early NMIs. */
      if (trapnr == X86_TRAP_NMI)
-             return 1;
+             return;
+
+     if (early_recursion_flag > 2)
+             goto halt_loop;
+
+     if (regs->cs != __KERNEL_CS)
+             goto fail;
 
      e = search_exception_tables(regs->ip);
      if (!e)
-             return 0;
+             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)
-             return 0;
+             goto fail;
 

arch/x86/mm/extable.c int __init early_fixup_exception(struct pt_regs *regs, int trapnr) · line 131

Quoted 10 lines

arch/x86/mm/extable.c +8 -1

      regs->ip = new_ip;
-     return 1;
+     return;
+
+fail:
+     early_printk("PANIC: early exception 0x%02x IP %lx:%lx error %lx cr2 0x%lx\n",
+                  (unsigned)trapnr, (unsigned long)regs->cs, regs->ip,
+                  regs->orig_ax, read_cr2());
+
+     show_regs(regs);

To make this even better, it could be something called early_show_regs() or so and be a simplified version of __show_regs() on both bitness but which calls early_printk().

This way you'll be able to get out stuff to the console as early as possible.

Btw, you don't need to dump rIP, CR2, etc in the PANIC message above since you're going to early_show_regs() anyway.

Given that I this isn't really a regression with my patches (it probably never worked much better on 32-bit and the regs never would have shown at all on 64-bit),

I propose a different approach: make
printk work earlier. Something like:

if (early) {
    early_printk(args);
}

or early_vprintk or whatever.

If the cost of a branch mattered, this could be alternative-patched out later on, but that seems silly. I also bet that a more sensible fallback could be created in which printk would try to use an early console if there's no real console.

Given that I this isn't really a regression with my patches (it probably never worked much better on 32-bit and the regs never would have shown at all on 64-bit),

I propose a different approach: make
printk work earlier. Something like:

if (early) {
    early_printk(args);
}

or early_vprintk or whatever.

If the cost of a branch mattered, this could be alternative-patched out later on, but that seems silly. I also bet that a more sensible fallback could be created in which printk would try to use an early console if there's no real console.

Quoted 3 lines · Andy Lutomirski

Given that I this isn't really a regression with my patches (it probably never worked much better on 32-bit and the regs never would have shown at all on 64-bit),

You're right. That thing calls printk *and* early_printk, WTF:

#ifdef CONFIG_EARLY_PRINTK

        call early_printk
        ...

        call dump_stack

        ...

        call __print_symbol

those last two call printk. Great.

Quoted 7 lines

I propose a different approach: make
printk work earlier. Something like:

if (early) {
    early_printk(args);
}

Quoted 6 lines

or early_vprintk or whatever.

If the cost of a branch mattered, this could be alternative-patched out later on, but that seems silly. I also bet that a more sensible fallback could be created in which printk would try to use an early console if there's no real console.

So how about this:

printk() does

        vprintk_func = this_cpu_read(printk_func);

and that's

DEFINE_PER_CPU(printk_func_t, printk_func) = vprintk_default

I guess we can make that function be early_printk-something and once printk is initialized, we overwrite it with vprintk_default.

Elegant and no need for if branches and alternatives.

Hmmm.

Quoted 3 lines · Andy Lutomirski

Given that I this isn't really a regression with my patches (it probably never worked much better on 32-bit and the regs never would have shown at all on 64-bit),

You're right. That thing calls printk *and* early_printk, WTF:

#ifdef CONFIG_EARLY_PRINTK

        call early_printk
        ...

        call dump_stack

        ...

        call __print_symbol

those last two call printk. Great.

Quoted 7 lines

I propose a different approach: make
printk work earlier. Something like:

if (early) {
    early_printk(args);
}

Quoted 6 lines

or early_vprintk or whatever.

If the cost of a branch mattered, this could be alternative-patched out later on, but that seems silly. I also bet that a more sensible fallback could be created in which printk would try to use an early console if there's no real console.

So how about this:

printk() does

        vprintk_func = this_cpu_read(printk_func);

and that's

DEFINE_PER_CPU(printk_func_t, printk_func) = vprintk_default

I guess we can make that function be early_printk-something and once printk is initialized, we overwrite it with vprintk_default.

Elegant and no need for if branches and alternatives.

Hmmm.

[cc Jan Kara]

Quoted 44 lines · Borislav Petkov

Quoted 3 lines · Andy Lutomirski

Given that I this isn't really a regression with my patches (it probably never worked much better on 32-bit and the regs never would have shown at all on 64-bit),

You're right. That thing calls printk *and* early_printk, WTF:

#ifdef CONFIG_EARLY_PRINTK

        call early_printk
        ...

        call dump_stack

        ...

        call __print_symbol

those last two call printk. Great.

Quoted 7 lines

I propose a different approach: make
printk work earlier. Something like:

if (early) {
    early_printk(args);
}

Quoted 6 lines

or early_vprintk or whatever.

If the cost of a branch mattered, this could be alternative-patched out later on, but that seems silly. I also bet that a more sensible fallback could be created in which printk would try to use an early console if there's no real console.

So how about this:

printk() does

        vprintk_func = this_cpu_read(printk_func);

and that's

DEFINE_PER_CPU(printk_func_t, printk_func) = vprintk_default

Quoted 6 lines

I guess we can make that function be early_printk-something and once printk is initialized, we overwrite it with vprintk_default.

Elegant and no need for if branches and alternatives.

Hmmm.

Jan, IIRC you were looking at printk recently-ish. Any thoughts here?

Sounds like a good idea to me. I've also consulted this with Petr Mladek (added to CC) who is using printk_func per-cpu variable in his printk-from-NMI patches and he also doesn't see a problem with this.

I was just wondering about one thing - this way we add more early printks if I understand your intention right. Are we guaranteed that they happen only from a single CPU? Because currently there is no locking in early_printk() and thus we can end up writing to early console several messages in parallel from different CPUs. Not sure what's going to happen in that case...

Sounds like a good idea to me. I've also consulted this with Petr Mladek (added to CC) who is using printk_func per-cpu variable in his printk-from-NMI patches and he also doesn't see a problem with this.

I was just wondering about one thing - this way we add more early printks if I understand your intention right. Are we guaranteed that they happen only from a single CPU? Because currently there is no locking in early_printk() and thus we can end up writing to early console several messages in parallel from different CPUs. Not sure what's going to happen in that case...

Quoted 47 lines · Andy Lutomirski

[cc Jan Kara]

Quoted 44 lines · Borislav Petkov

Quoted 3 lines · Andy Lutomirski

Given that I this isn't really a regression with my patches (it probably never worked much better on 32-bit and the regs never would have shown at all on 64-bit),

You're right. That thing calls printk *and* early_printk, WTF:

#ifdef CONFIG_EARLY_PRINTK

        call early_printk
        ...

        call dump_stack

        ...

        call __print_symbol

those last two call printk. Great.

Quoted 7 lines

I propose a different approach: make
printk work earlier. Something like:

if (early) {
    early_printk(args);
}

Quoted 6 lines

or early_vprintk or whatever.

If the cost of a branch mattered, this could be alternative-patched out later on, but that seems silly. I also bet that a more sensible fallback could be created in which printk would try to use an early console if there's no real console.

So how about this:

printk() does

        vprintk_func = this_cpu_read(printk_func);

and that's

DEFINE_PER_CPU(printk_func_t, printk_func) = vprintk_default

Quoted 8 lines

Quoted 6 lines

I guess we can make that function be early_printk-something and once printk is initialized, we overwrite it with vprintk_default.

Elegant and no need for if branches and alternatives.

Hmmm.

Jan, IIRC you were looking at printk recently-ish. Any thoughts here?

Sounds like a good idea to me. I've also consulted this with Petr Mladek (added to CC) who is using printk_func per-cpu variable in his printk-from-NMI patches and he also doesn't see a problem with this.

I was just wondering about one thing - this way we add more early printks if I understand your intention right. Are we guaranteed that they happen only from a single CPU? Because currently there is no locking in early_printk() and thus we can end up writing to early console several messages in parallel from different CPUs. Not sure what's going to happen in that case...

Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR

Quoted 47 lines · Andy Lutomirski

[cc Jan Kara]

Quoted 44 lines · Borislav Petkov

Quoted 3 lines · Andy Lutomirski

Given that I this isn't really a regression with my patches (it probably never worked much better on 32-bit and the regs never would have shown at all on 64-bit),

You're right. That thing calls printk *and* early_printk, WTF:

#ifdef CONFIG_EARLY_PRINTK

        call early_printk
        ...

        call dump_stack

        ...

        call __print_symbol

those last two call printk. Great.

Quoted 7 lines

I propose a different approach: make
printk work earlier. Something like:

if (early) {
    early_printk(args);
}

Quoted 6 lines

or early_vprintk or whatever.

If the cost of a branch mattered, this could be alternative-patched out later on, but that seems silly. I also bet that a more sensible fallback could be created in which printk would try to use an early console if there's no real console.

So how about this:

printk() does

        vprintk_func = this_cpu_read(printk_func);

and that's

DEFINE_PER_CPU(printk_func_t, printk_func) = vprintk_default

Quoted 8 lines

Quoted 6 lines

I guess we can make that function be early_printk-something and once printk is initialized, we overwrite it with vprintk_default.

Elegant and no need for if branches and alternatives.

Hmmm.

Jan, IIRC you were looking at printk recently-ish. Any thoughts here?

Sounds like a good idea to me. I've also consulted this with Petr Mladek (added to CC) who is using printk_func per-cpu variable in his printk-from-NMI patches and he also doesn't see a problem with this.

I was just wondering about one thing - this way we add more early printks if I understand your intention right. Are we guaranteed that they happen only from a single CPU? Because currently there is no locking in early_printk() and thus we can end up writing to early console several messages in parallel from different CPUs. Not sure what's going to happen in that case...

Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR

Adding locking would be easy enough, wouldn't it?

But do any platforms really boot a second CPU before switching to real printk? Given that I see all the smpboot stuff in dmesg, I guess real printk happens first. I admit I haven't actually checked.

Adding locking would be easy enough, wouldn't it?

But do any platforms really boot a second CPU before switching to real printk? Given that I see all the smpboot stuff in dmesg, I guess real printk happens first. I admit I haven't actually checked.

Adding locking would be easy enough, wouldn't it?

But do any platforms really boot a second CPU before switching to real printk?

Adding locking would be easy enough, wouldn't it?

But do any platforms really boot a second CPU before switching to real printk?

Quoted 1 line · Andy Lutomirski

Adding locking would be easy enough, wouldn't it?

See patch in this thread..

Quoted 2 lines

But do any platforms really boot a second CPU before switching to real printk?

I _only_ use early_printk() as printk() is a quagmire of fail :-)

Quoted 1 line · Andy Lutomirski

Adding locking would be easy enough, wouldn't it?

See patch in this thread..

Quoted 2 lines

But do any platforms really boot a second CPU before switching to real printk?

I _only_ use early_printk() as printk() is a quagmire of fail :-)

Quoted 10 lines · Peter Zijlstra

Quoted 1 line · Andy Lutomirski

Adding locking would be easy enough, wouldn't it?

See patch in this thread..

Quoted 2 lines

But do any platforms really boot a second CPU before switching to real printk?

I _only_ use early_printk() as printk() is a quagmire of fail :-)

And since I'm the king of minimalistic changes... this below works. However, the problem is that we need to pull up all that early_param parsing in order to enable the early console, i.e. "earlyprintk=ttyS0,115200" cmdline parsing.

And we can do all that after having setup the IDT. So I'd need to do some early dancing with cmdline_find_option_bool(boot_command_line, ... in asm or so. Need to think about it more.

Could you do it by moving just the earlyprintk stuff a la fpu__init_parse_early_param()?

Could you do it by moving just the earlyprintk stuff a la fpu__init_parse_early_param()?

Quoted 10 lines · Peter Zijlstra

Quoted 1 line · Andy Lutomirski

Adding locking would be easy enough, wouldn't it?

See patch in this thread..

Quoted 2 lines

But do any platforms really boot a second CPU before switching to real printk?

I _only_ use early_printk() as printk() is a quagmire of fail :-)

And since I'm the king of minimalistic changes... this below works. However, the problem is that we need to pull up all that early_param parsing in order to enable the early console, i.e. "earlyprintk=ttyS0,115200" cmdline parsing.

And we can do all that after having setup the IDT. So I'd need to do some early dancing with cmdline_find_option_bool(boot_command_line, ... in asm or so. Need to think about it more.

[cc Jan Kara]

Quoted 44 lines · Borislav Petkov

Quoted 3 lines · Andy Lutomirski

Given that I this isn't really a regression with my patches (it probably never worked much better on 32-bit and the regs never would have shown at all on 64-bit),

You're right. That thing calls printk *and* early_printk, WTF:

#ifdef CONFIG_EARLY_PRINTK

        call early_printk
        ...

        call dump_stack

        ...

        call __print_symbol

those last two call printk. Great.

Quoted 7 lines

I propose a different approach: make
printk work earlier. Something like:

if (early) {
    early_printk(args);
}

Quoted 6 lines

or early_vprintk or whatever.

If the cost of a branch mattered, this could be alternative-patched out later on, but that seems silly. I also bet that a more sensible fallback could be created in which printk would try to use an early console if there's no real console.

So how about this:

printk() does

        vprintk_func = this_cpu_read(printk_func);

and that's

DEFINE_PER_CPU(printk_func_t, printk_func) = vprintk_default

Quoted 6 lines

I guess we can make that function be early_printk-something and once printk is initialized, we overwrite it with vprintk_default.

Elegant and no need for if branches and alternatives.

Hmmm.

Jan, IIRC you were looking at printk recently-ish. Any thoughts here?

Quoted 11 lines · Andy Lutomirski

This removes a bunch of assembly and adds some C code instead. It changes the actual printouts on both 32-bit and 64-bit kernels, but they still seem okay.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/include/asm/uaccess.h |  2 +-
 arch/x86/kernel/head_32.S      | 49 +++++-------------------------------------
 arch/x86/kernel/head_64.S      | 45 ++------------------------------------
 arch/x86/mm/extable.c          | 29 ++++++++++++++++++++-----
 4 files changed, 32 insertions(+), 93 deletions(-)

...

Quoted 26 lines

arch/x86/mm/extable.c +9 -3

@@ -99,21 +101,38 @@ int __init early_fixup_exception(struct pt_regs *regs, int trapnr)
 
      /* Ignore early NMIs. */
      if (trapnr == X86_TRAP_NMI)
-             return 1;
+             return;
+
+     if (early_recursion_flag > 2)
+             goto halt_loop;
+
+     if (regs->cs != __KERNEL_CS)
+             goto fail;
 
      e = search_exception_tables(regs->ip);
      if (!e)
-             return 0;
+             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)
-             return 0;
+             goto fail;
 

arch/x86/mm/extable.c int __init early_fixup_exception(struct pt_regs *regs, int trapnr) · line 131

Quoted 10 lines

arch/x86/mm/extable.c +8 -1

      regs->ip = new_ip;
-     return 1;
+     return;
+
+fail:
+     early_printk("PANIC: early exception 0x%02x IP %lx:%lx error %lx cr2 0x%lx\n",
+                  (unsigned)trapnr, (unsigned long)regs->cs, regs->ip,
+                  regs->orig_ax, read_cr2());
+
+     show_regs(regs);

To make this even better, it could be something called early_show_regs() or so and be a simplified version of __show_regs() on both bitness but which calls early_printk().

This way you'll be able to get out stuff to the console as early as possible.

Btw, you don't need to dump rIP, CR2, etc in the PANIC message above since you're going to early_show_regs() anyway.

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 <[email protected]>

---
 arch/x86/mm/extable.c | 19 ++-----------------
 1 file changed, 2 insertions(+), 17 deletions(-)

arch/x86/mm/extable.c +0 -4

index 8997022abebc..50dfe438bd91 100644
@@ -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;
 

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 <[email protected]>

---
 arch/x86/mm/extable.c | 19 ++-----------------
 1 file changed, 2 insertions(+), 17 deletions(-)

arch/x86/mm/extable.c +0 -4

index 8997022abebc..50dfe438bd91 100644
@@ -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;
 

Quoted 28 lines · Andy Lutomirski

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 <[email protected]>

---
 arch/x86/mm/extable.c | 19 ++-----------------
 1 file changed, 2 insertions(+), 17 deletions(-)

arch/x86/mm/extable.c +0 -4

index 8997022abebc..50dfe438bd91 100644
@@ -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;
 

arch/x86/mm/extable.c void __init early_fixup_exception(struct pt_regs *regs, int trapnr) · line 121

Quoted 10 lines

arch/x86/mm/extable.c +0 -10

-     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) {

...

(quoted diff) +0 -4

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

arch/x86/mm/extable.c void __init early_fixup_exception(struct pt_regs *regs, int trapnr) · line 109

Quoted 5 lines

arch/x86/mm/extable.c +2 -3

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

Quoted 6 lines · Andy Lutomirski

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?

Quoted 3 lines

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.

Quoted 6 lines · Andy Lutomirski

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?

Quoted 3 lines

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.

Quoted 7 lines · Borislav Petkov

Quoted 6 lines · Andy Lutomirski

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?

Quoted 8 lines

Quoted 3 lines

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?

Quoted 7 lines · Borislav Petkov

Quoted 6 lines · Andy Lutomirski

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?

Quoted 8 lines

Quoted 3 lines

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

Quoted 5 lines · Borislav Petkov

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.

Quoted 11 lines

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.

Quoted 12 lines · Andy Lutomirski

Quoted 11 lines

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.

Quoted 3 lines

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.

Quoted 12 lines · Andy Lutomirski

Quoted 11 lines

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.

Quoted 3 lines

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.

Quoted 11 lines

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.

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.

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.

Quoted 5 lines · Borislav Petkov

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.

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.

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.

Quoted 28 lines · Andy Lutomirski

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 <[email protected]>

---
 arch/x86/mm/extable.c | 19 ++-----------------
 1 file changed, 2 insertions(+), 17 deletions(-)

arch/x86/mm/extable.c +0 -4

index 8997022abebc..50dfe438bd91 100644
@@ -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;
 

arch/x86/mm/extable.c void __init early_fixup_exception(struct pt_regs *regs, int trapnr) · line 121

Quoted 10 lines

arch/x86/mm/extable.c +0 -10

-     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) {

...

(quoted diff) +0 -4

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

arch/x86/mm/extable.c void __init early_fixup_exception(struct pt_regs *regs, int trapnr) · line 109

Quoted 5 lines

arch/x86/mm/extable.c +2 -3

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

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

These hooks match the _safe variants, so name them accordingly. This will make room for unsafe PV hooks.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/include/asm/paravirt.h       | 33 +++++++++++++++++----------------
 arch/x86/include/asm/paravirt_types.h |  8 ++++----
 arch/x86/kernel/paravirt.c            |  4 ++--
 arch/x86/xen/enlighten.c              |  4 ++--
 4 files changed, 25 insertions(+), 24 deletions(-)

arch/x86/include/asm/paravirt_types.h +0 -0

index 77db5616a473..5a06cccd36f0 100644

arch/x86/include/asm/paravirt_types.h +1 -0

+	u64 (*read_msr_safe)(unsigned int msr, int *err)Please reformat this

comment properly, while you're at it:

arch/x86/include/asm/paravirt_types.h +1 -0

+	int (*write_msr_safe)(unsigned int msr, unsigned low, unsigned high);
 
 	u64 (*read_pmc)(int counter);

Quoted 10 lines · Andy Lutomirski

These hooks match the _safe variants, so name them accordingly. This will make room for unsafe PV hooks.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/include/asm/paravirt.h       | 33 +++++++++++++++++----------------
 arch/x86/include/asm/paravirt_types.h |  8 ++++----
 arch/x86/kernel/paravirt.c            |  4 ++--
 arch/x86/xen/enlighten.c              |  4 ++--
 4 files changed, 25 insertions(+), 24 deletions(-)

...

Quoted 5 lines

arch/x86/include/asm/paravirt_types.h +0 -0

index 77db5616a473..5a06cccd36f0 100644

arch/x86/include/asm/paravirt_types.h struct pv_cpu_ops { · line 159

Quoted 9 lines

arch/x86/include/asm/paravirt_types.h +2 -4

@@ -155,10 +155,10 @@ struct pv_cpu_ops {
      void (*cpuid)(unsigned int *eax, unsigned int *ebx,
                    unsigned int *ecx, unsigned int *edx);
 
-     /* MSR, PMC and TSR operations.
-        err = 0/-EFAULT.  wrmsr returns 0/-EFAULT. */
-     u64 (*read_msr)(unsigned int msr, int *err);
-     int (*write_msr)(unsigned int msr, unsigned low, unsigned high);
+     /* MSR operations.
+        err = 0/-EIO.  wrmsr returns 0/-EIO. */

Please reformat this comment properly, while you're at it:

/*
* A sentence.
* Another sentence.
*/

You already caught that one. It's fixed in "x86/paravirt: Add paravirt_{read,write}_msr".

Congrats on being deterministic :)

You already caught that one. It's fixed in "x86/paravirt: Add paravirt_{read,write}_msr".

Congrats on being deterministic :)

Quoted 10 lines · Andy Lutomirski

These hooks match the _safe variants, so name them accordingly. This will make room for unsafe PV hooks.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/include/asm/paravirt.h       | 33 +++++++++++++++++----------------
 arch/x86/include/asm/paravirt_types.h |  8 ++++----
 arch/x86/kernel/paravirt.c            |  4 ++--
 arch/x86/xen/enlighten.c              |  4 ++--
 4 files changed, 25 insertions(+), 24 deletions(-)

...

Quoted 5 lines

arch/x86/include/asm/paravirt_types.h +0 -0

index 77db5616a473..5a06cccd36f0 100644

arch/x86/include/asm/paravirt_types.h struct pv_cpu_ops { · line 159

Quoted 9 lines

arch/x86/include/asm/paravirt_types.h +2 -4

@@ -155,10 +155,10 @@ struct pv_cpu_ops {
      void (*cpuid)(unsigned int *eax, unsigned int *ebx,
                    unsigned int *ecx, unsigned int *edx);
 
-     /* MSR, PMC and TSR operations.
-        err = 0/-EFAULT.  wrmsr returns 0/-EFAULT. */
-     u64 (*read_msr)(unsigned int msr, int *err);
-     int (*write_msr)(unsigned int msr, unsigned low, unsigned high);
+     /* MSR operations.
+        err = 0/-EIO.  wrmsr returns 0/-EIO. */

Please reformat this comment properly, while you're at it:

/*
* A sentence.
* Another sentence.
*/

These hooks match the _safe variants, so name them accordingly. This will make room for unsafe PV hooks.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/include/asm/paravirt.h       | 33 +++++++++++++++++----------------
 arch/x86/include/asm/paravirt_types.h |  8 ++++----
 arch/x86/kernel/paravirt.c            |  4 ++--
 arch/x86/xen/enlighten.c              |  4 ++--
 4 files changed, 25 insertions(+), 24 deletions(-)

arch/x86/include/asm/paravirt_types.h +0 -0

index 77db5616a473..5a06cccd36f0 100644

arch/x86/include/asm/paravirt_types.h +1 -0

+	u64 (*read_msr_safe)(unsigned int msr, int *err)Please reformat this

comment properly, while you're at it:

arch/x86/include/asm/paravirt_types.h +1 -0

+	int (*write_msr_safe)(unsigned int msr, unsigned low, unsigned high);
 
 	u64 (*read_pmc)(int counter);