Re: [PATCH v2] x86_64: install emergency handler for double fault

[Samuel Thibault]

Applied, thanks!

Luca Dariz, le sam. 29 juil. 2023 19:45:14 +0200, a ecrit:
> * i386/i386/idt.c: add selector for the interrupt-specific stack
> * i386/i386/ktss.c: configure ist1 to use a dedicated stack
> * i386/i386/trap.c: add double fault handler, which just prints the
>   state and panics. There is not much else to do in this case but it's
>   useful for troubleshooting
> * x86_64/idt_inittab.S: allow to specify an interrupt stack for custom
>   handlers
> * x86_64/locore.S: add double fault handler
> ---
>  i386/i386/idt.c      | 12 +++++++++++-
>  i386/i386/ktss.c     |  4 +++-
>  i386/i386/trap.c     |  6 ++++++
>  x86_64/idt_inittab.S | 25 +++++++++++++------------
>  x86_64/locore.S      | 15 +++++++++++++++
>  5 files changed, 48 insertions(+), 14 deletions(-)
> 
> diff --git a/i386/i386/idt.c b/i386/i386/idt.c
> index cdfb9a88..caa44d71 100644
> --- a/i386/i386/idt.c
> +++ b/i386/i386/idt.c
> @@ -34,6 +34,10 @@ struct idt_init_entry
>       unsigned long entrypoint;
>       unsigned short vector;
>       unsigned short type;
> +#ifdef __x86_64__
> +     unsigned short ist;
> +     unsigned short pad_0;
> +#endif
>  };
>  extern struct idt_init_entry idt_inittab[];
>  
> @@ -49,7 +53,13 @@ idt_fill(struct real_gate *myidt)
>       /* Initialize the exception vectors from the idt_inittab.  */
>       while (iie->entrypoint)
>       {
> -             fill_idt_gate(myidt, iie->vector, iie->entrypoint, KERNEL_CS, 
> iie->type, 0);
> +             fill_idt_gate(myidt, iie->vector, iie->entrypoint, KERNEL_CS, 
> iie->type,
> +#ifdef __x86_64__
> +                           iie->ist
> +#else
> +                           0
> +#endif
> +                 );
>               iie++;
>       }
>  
> diff --git a/i386/i386/ktss.c b/i386/i386/ktss.c
> index 1d880167..34cb6df2 100644
> --- a/i386/i386/ktss.c
> +++ b/i386/i386/ktss.c
> @@ -43,9 +43,10 @@ struct task_tss ktss;
>  void
>  ktss_fill(struct task_tss *myktss, struct real_descriptor *mygdt)
>  {
> -     /* XXX temporary exception stack */
> +     /* XXX temporary exception stacks */
>       /* FIXME: make it per-processor */
>       static int exception_stack[1024];
> +     static int double_fault_stack[1024];
>  
>  #ifdef       MACH_RING1
>       /* Xen won't allow us to do any I/O by default anyway, just register
> @@ -61,6 +62,7 @@ ktss_fill(struct task_tss *myktss, struct real_descriptor 
> *mygdt)
>       /* Initialize the master TSS.  */
>  #ifdef __x86_64__
>       myktss->tss.rsp0 = (unsigned long)(exception_stack+1024);
> +     myktss->tss.ist1 = (unsigned long)(double_fault_stack+1024);
>  #else /* ! __x86_64__ */
>       myktss->tss.ss0 = KERNEL_DS;
>       myktss->tss.esp0 = (unsigned long)(exception_stack+1024);
> diff --git a/i386/i386/trap.c b/i386/i386/trap.c
> index f7bd8e38..b3689c9a 100644
> --- a/i386/i386/trap.c
> +++ b/i386/i386/trap.c
> @@ -666,3 +666,9 @@ db_debug_all_traps (boolean_t enable)
>  }
>  
>  #endif       /* MACH_KDB */
> +
> +void handle_double_fault(struct i386_saved_state *regs)
> +{
> +  dump_ss(regs);
> +  panic("DOUBLE FAULT! This is critical\n");
> +}
> diff --git a/x86_64/idt_inittab.S b/x86_64/idt_inittab.S
> index f021b56d..fc1df0c7 100644
> --- a/x86_64/idt_inittab.S
> +++ b/x86_64/idt_inittab.S
> @@ -50,12 +50,13 @@ ENTRY(idt_inittab)
>       .quad   entry           ;\
>       .text
>  #else        /* MACH_PV_DESCRIPTORS */
> -#define      IDT_ENTRY(n,entry,type) \
> +#define      IDT_ENTRY(n,entry,type,ist) \
>       .data   2               ;\
>       .quad   entry           ;\
>       .word   n               ;\
>       .word   type            ;\
> -     .long   0       /*pad*/ ;\
> +     .word   ist             ;\
> +     .word   0       /*pad*/ ;\
>       .text
>  #endif       /* MACH_PV_DESCRIPTORS */
>  
> @@ -63,7 +64,7 @@ ENTRY(idt_inittab)
>   * No error code.  Clear error code and push trap number.
>   */
>  #define      EXCEPTION(n,name) \
> -     IDT_ENTRY(n,EXT(name),ACC_PL_K|ACC_TRAP_GATE);\
> +     IDT_ENTRY(n,EXT(name),ACC_PL_K|ACC_TRAP_GATE, 0);\
>  ENTRY(name)                          ;\
>       INT_FIX                         ;\
>       pushq   $(0)                    ;\
> @@ -74,7 +75,7 @@ ENTRY(name)                         ;\
>   * User-accessible exception.  Otherwise, same as above.
>   */
>  #define      EXCEP_USR(n,name) \
> -     IDT_ENTRY(n,EXT(name),ACC_PL_U|ACC_TRAP_GATE);\
> +     IDT_ENTRY(n,EXT(name),ACC_PL_U|ACC_TRAP_GATE, 0);\
>  ENTRY(name)                          ;\
>       INT_FIX                         ;\
>       pushq   $(0)                    ;\
> @@ -85,7 +86,7 @@ ENTRY(name)                         ;\
>   * Error code has been pushed.  Just push trap number.
>   */
>  #define      EXCEP_ERR(n,name) \
> -     IDT_ENTRY(n,EXT(name),ACC_PL_K|ACC_INTR_GATE);\
> +     IDT_ENTRY(n,EXT(name),ACC_PL_K|ACC_INTR_GATE, 0);\
>  ENTRY(name)                          ;\
>       INT_FIX                         ;\
>       pushq   $(n)                    ;\
> @@ -95,25 +96,25 @@ ENTRY(name)                               ;\
>   * Special interrupt code: dispatches to a unique entrypoint,
>   * not defined automatically here.
>   */
> -#define      EXCEP_SPC(n,name)  \
> -     IDT_ENTRY(n,EXT(name),ACC_PL_K|ACC_TRAP_GATE)
> +#define      EXCEP_SPC(n,name, ist)  \
> +     IDT_ENTRY(n,EXT(name),ACC_PL_K|ACC_TRAP_GATE, ist)
>  
>  
>  EXCEPTION(0x00,t_zero_div)
> -EXCEP_SPC(0x01,t_debug)
> +EXCEP_SPC(0x01,t_debug, 0)
>  /* skip NMI interrupt - let more specific code figure that out.  */
>  EXCEP_USR(0x03,t_int3)
>  EXCEP_USR(0x04,t_into)
>  EXCEP_USR(0x05,t_bounds)
>  EXCEPTION(0x06,t_invop)
>  EXCEPTION(0x07,t_nofpu)
> -EXCEPTION(0x08,a_dbl_fault)
> +EXCEP_SPC(0x08,t_dbl_fault, 1)
>  EXCEPTION(0x09,a_fpu_over)
>  EXCEPTION(0x0a,a_inv_tss)
> -EXCEP_SPC(0x0b,t_segnp)
> +EXCEP_SPC(0x0b,t_segnp, 0)
>  EXCEP_ERR(0x0c,t_stack_fault)
> -EXCEP_SPC(0x0d,t_gen_prot)
> -EXCEP_SPC(0x0e,t_page_fault)
> +EXCEP_SPC(0x0d,t_gen_prot, 0)
> +EXCEP_SPC(0x0e,t_page_fault, 0)
>  #ifdef MACH_PV_DESCRIPTORS
>  EXCEP_ERR(0x0f,t_trap_0f)
>  #else
> diff --git a/x86_64/locore.S b/x86_64/locore.S
> index 2938e430..16b0dde5 100644
> --- a/x86_64/locore.S
> +++ b/x86_64/locore.S
> @@ -345,6 +345,21 @@ ENTRY(start_timer)
>   *
>   */
>  
> +/* Try to save/show some information when a double fault happens
> + * We can't recover to a working state, so if we have a debugger wait for it,
> + * otherwise reset */
> +ENTRY(t_dbl_fault)
> +     INT_FIX
> +     cli     /* disable interrupts that might corrupt the state*/
> +     pusha
> +     movq    %cr2,%rax
> +     movq    %rax,R_CR2-R_R15(%rsp)  /* CR2 might contain the faulting 
> address */
> +     subq    $48,%rsp        // FIXME remove when segments are cleaned up
> +     movq    %rsp,%rdi               /* pass the saved state */
> +     call    handle_double_fault
> +     jmp     cpu_shutdown    /* reset */
> +END(t_dbl_fault)
> +
>  /*
>   * General protection or segment-not-present fault.
>   * Check for a GP/NP fault in the kernel_return
> -- 
> 2.39.2
> 
> 

-- 
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.