Re: PROBLEM: consolidated IDT invalidation causes kexec to reboot
From: Alexandru Chirvasitu
Date: Mon Dec 25 2017 - 16:39:20 EST
Thanks for that!
On Mon, Dec 25, 2017 at 06:40:14AM -0800, Andy Lutomirski wrote:
> On Sat, Dec 23, 2017 at 7:30 PM, Linus Torvalds
> <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> > On Sat, Dec 23, 2017 at 5:44 PM, Alexandru Chirvasitu
> > <achirvasub@xxxxxxxxx> wrote:
> >>
> >> For testing purposes, I've altered machine_kexec_32.c making the
> >> following toy commit. It naively undoes part of e802a51, solely to
> >> confirm that's where it goes awry in my setup.
> >
> > That's really funky.
> >
> > The idt_invalidate() seems to do *exactly* the same thing. It uses
> > "load_idt()" on an IDT with size 0, and the supplied address.
> >
> > Can you disassemble your "set_idt()" code vs the "idt_invalidate()"?
> >
> >> Is this expected behaviour?
> >
> > No. The code literally seems identical. The only difference is
> >
> > (a) where the 0 limit comes from
> >
> > (b) perhaps build flags and whether it is inlined or not due to being
> > in a different file
> >
> > and neither of those should matter, but maybe they do.
> >
> > Which is why I'd like you to actually look at the generated code and
> > see if you can see any difference..
> >
>
> This is presumably the same call-tracing-without-TLS-working problem.
> idt_invalidate() is out-of-line and is compiled with full tracing on,
> and we're calling it from a context without TLS working (it's
> explicitly disabled in load_segments()) in machine_kexec_32.c. The
> right fix is probably to inline idt_invalidate() and to add a comment.
>
This works.. I went back to the troublesome commit e802a51 and
modified it as follows:
--------------------------------------------------------
make idt_invalidate static inline in header file
diff --git a/arch/x86/include/asm/desc.h b/arch/x86/include/asm/desc.h
index 33aff45..87ca363 100644
--- a/arch/x86/include/asm/desc.h
+++ b/arch/x86/include/asm/desc.h
@@ -504,6 +504,17 @@ static inline void load_current_idt(void)
load_idt((const struct desc_ptr *)&idt_descr);
}
-extern void idt_invalidate(void *addr);
+
+/**
+ * idt_invalidate - Invalidate interrupt descriptor table
+ * @addr: The virtual address of the 'invalid' IDT
+ */
+static inline void idt_invalidate(void *addr)
+{
+ struct desc_ptr idt = { .address = (unsigned long) addr, .size = 0 };
+
+ load_idt(&idt);
+}
+
#endif /* _ASM_X86_DESC_H */
diff --git a/arch/x86/kernel/idt.c b/arch/x86/kernel/idt.c
index cd4658c..fec44c6 100644
--- a/arch/x86/kernel/idt.c
+++ b/arch/x86/kernel/idt.c
@@ -25,13 +25,3 @@ const struct desc_ptr debug_idt_descr = {
};
#endif
-/**
- * idt_invalidate - Invalidate interrupt descriptor table
- * @addr: The virtual address of the 'invalid' IDT
- */
-void idt_invalidate(void *addr)
-{
- struct desc_ptr idt = { .address = (unsigned long) addr, .size = 0 };
-
- load_idt(&idt);
-}
--------------------------------------------------------
kexec now works as expected; tested repeatedly, both with direct
execution and crash triggering.
I had to google 'inline function' :)).
> Also, why idt_invalidate(phys_to_virt(0))? That makes rather little
> sense to me.
>
> --Andy