| Message ID | 20fc047d926150cb08cb9b9f2923519b07ec1a15.1459605520.git.luto@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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...
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?
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
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
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
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.
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
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.
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",
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(-)