Re: [PATCH 1/2] hurd: Remove the ecx kludge

[Samuel Thibault]

Applied, thanks!

Sergey Bugaev, le mer. 01 mars 2023 19:23:54 +0300, a ecrit:
> "We don't need it any more"
> 
> The INTR_MSG_TRAP macro in intr-msg.h used to play little trick with
> the stack pointer: it would temporarily save the "real" stack pointer
> into ecx, while setting esp to point to just before the message buffer,
> and then invoke the mach_msg trap. This way, INTR_MSG_TRAP reused the
> on-stack arguments laid out for the containing call of
> _hurd_intr_rpc_mach_msg (), passing them to the mach_msg trap directly.
> 
> This, however, required special support in hurdsig.c and trampoline.c,
> since they now had to recognize when a thread is inside the piece of
> code where esp doesn't point to the real tip of the stack, and handle
> this situation specially.
> 
> Commit 1d20f33ff4fb634310f27493b7b87d0b20f4a0b0 has removed the actual
> temporary change of esp by actually re-pushing mach_msg arguments onto
> the stack, and popping them back at end. It did not, however, deal with
> the rest of "the ecx kludge" code in other files, resulting in potential
> crashes if a signal arrives in the middle of pushing arguments onto the
> stack.
> 
> Fix that by removing "the ecx kludge". Instead, when we want a thread
> to skip the RPC, but cannot make just make it jump to after the trap
> since it's not done adjusting the stack yet, set the SYSRETURN register
> to MACH_SEND_INTERRUPTED (as we do anyway), and rely on the thread
> itself for detecting this case and skipping the RPC.
> 
> This simplifies things somewhat and paves the way for a future x86_64
> port of this code.
> 
> Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
> ---
>  hurd/hurdsig.c                      | 18 +++++++++----
>  sysdeps/mach/hurd/i386/intr-msg.h   | 40 +++++++++++++----------------
>  sysdeps/mach/hurd/i386/trampoline.c | 21 ---------------
>  3 files changed, 31 insertions(+), 48 deletions(-)
> 
> diff --git a/hurd/hurdsig.c b/hurd/hurdsig.c
> index ea79ffb5..5ff0a91f 100644
> --- a/hurd/hurdsig.c
> +++ b/hurd/hurdsig.c
> @@ -415,6 +415,7 @@ _hurdsig_abort_rpcs (struct hurd_sigstate *ss, int signo, 
> int sigthread,
>                    void (*reply) (void))
>  {
>    extern const void _hurd_intr_rpc_msg_about_to;
> +  extern const void _hurd_intr_rpc_msg_setup_done;
>    extern const void _hurd_intr_rpc_msg_in_trap;
>    mach_port_t rcv_port = MACH_PORT_NULL;
>    mach_port_t intr_port;
> @@ -434,11 +435,18 @@ _hurdsig_abort_rpcs (struct hurd_sigstate *ss, int 
> signo, int sigthread,
>        && state->basic.PC < (uintptr_t) &_hurd_intr_rpc_msg_in_trap)
>      {
>        /* The thread is about to do the RPC, but hasn't yet entered
> -      mach_msg.  Mutate the thread's state so it knows not to try
> -      the RPC.  */
> -      INTR_MSG_BACK_OUT (&state->basic);
> -      MACHINE_THREAD_STATE_SET_PC (&state->basic,
> -                                &_hurd_intr_rpc_msg_in_trap);
> +         mach_msg.  Importantly, it may have already checked ss->cancel for
> +         the last time before doing the RPC, so setting that is not enough
> +         to make it not enter mach_msg.  Instead, mutate the thread's state
> +         so it knows not to try the RPC.
> +
> +         If the thread is past _hurd_intr_rpc_msg_setup_done, just make it
> +         jump to after the trap, since we know it's safe to do so.  
> Otherwise,
> +         we know that the thread is yet to check for the 
> MACH_SEND_INTERRUPTED
> +         value we set below, and will skip the trap by itself.  */
> +      if (state->basic.PC >= (uintptr_t) &_hurd_intr_rpc_msg_setup_done)
> +        MACHINE_THREAD_STATE_SET_PC (&state->basic,
> +                                     &_hurd_intr_rpc_msg_in_trap);
>        state->basic.SYSRETURN = MACH_SEND_INTERRUPTED;
>        *state_change = 1;
>      }
> diff --git a/sysdeps/mach/hurd/i386/intr-msg.h 
> b/sysdeps/mach/hurd/i386/intr-msg.h
> index 29cb4620..21088fa8 100644
> --- a/sysdeps/mach/hurd/i386/intr-msg.h
> +++ b/sysdeps/mach/hurd/i386/intr-msg.h
> @@ -25,10 +25,13 @@
>  ({                                                                         \
>    error_t err;                                                               
>       \
>    asm (".globl _hurd_intr_rpc_msg_about_to\n"                                
>       \
> -       ".globl _hurd_intr_rpc_msg_cx_sp\n"                                 \
> -       ".globl _hurd_intr_rpc_msg_do_trap\n"                                 
>       \
> +       ".globl _hurd_intr_rpc_msg_setup_done\n"                              
>       \
>         ".globl _hurd_intr_rpc_msg_in_trap\n"                               \
> -       ".globl _hurd_intr_rpc_msg_sp_restored\n"                           \
> +       /* Clear eax before we do the check for cancel below.  This is to
> +          detect eax being set to non-zero (actually MACH_SEND_INTERRUPTED)
> +          from the outside (namely, _hurdsig_abort_rpcs), which signals us
> +          to skip the trap we were about to enter.  */                       
>       \
> +       "                             xorl %0, %0\n"                        \
>         "_hurd_intr_rpc_msg_about_to:"                                        
>       \
>         /* We need to make a last check of cancel, in case we got interrupted
>            right before _hurd_intr_rpc_msg_about_to.  */                      
>       \
> @@ -36,10 +39,10 @@
>         "                             jz _hurd_intr_rpc_msg_do\n"           \
>         /* We got interrupted, note so and return EINTR.  */                \
>         "                             movl $0, %3\n"                        \
> -       "                             movl %6, %%eax\n"                     \
> +       "                             movl %6, %0\n"                        \
>         "                             jmp _hurd_intr_rpc_msg_sp_restored\n" \
>         "_hurd_intr_rpc_msg_do:"                                              
>       \
> -       /* Ok, push the mach_msg_trap arguments.  */                        \
> +       /* Ok, push the mach_msg_trap arguments and a fake return address.  
> */ \
>         "                             pushl 24(%4)\n"                       \
>         "                             pushl %2\n"                           \
>         "                             pushl 16(%4)\n"                       \
> @@ -48,9 +51,14 @@
>         "                             pushl %1\n"                           \
>         "                             pushl (%4)\n"                         \
>         "                             pushl $0\n"                           \
> -       /* TODO: remove this ecx kludge, we don't need it any more */       \
> -       "                             movl %%esp, %%ecx\n"                  \
> -       "_hurd_intr_rpc_msg_cx_sp:    movl $-25, %%eax\n"                   \
> +       "_hurd_intr_rpc_msg_setup_done:"                                      
>       \
> +       /* From here on, it is safe to make us jump over the syscall.  Now
> +          check if we have been told to skip the syscall while running
> +          the above.  */                                                   \
> +       "                             test %0, %0\n"                        \
> +       "                             jnz _hurd_intr_rpc_msg_in_trap\n"     \
> +       /* Do the actual syscall.  */                                       \
> +       "                             movl $-25, %%eax\n"                   \
>         "_hurd_intr_rpc_msg_do_trap:  lcall $7, $0 # status in %0\n"        \
>         "_hurd_intr_rpc_msg_in_trap:"                                       \
>         /* Ok, clean the arguments and update OPTION and TIMEOUT.  */       \
> @@ -60,22 +68,10 @@
>         "                             popl %2\n"                            \
>         "                             addl $4, %%esp\n"                     \
>         "_hurd_intr_rpc_msg_sp_restored:"                                   \
> -       : "=a" (err), "+r" (option), "+r" (timeout), "=m" (*intr_port_p)      
>       \
> -       : "r" (&msg), "m" (*cancel_p), "i" (EINTR)                          \
> -       : "ecx");                                                           \
> +       : "=&a" (err), "+r" (option), "+r" (timeout), "=m" (*intr_port_p)     
>  \
> +       : "r" (&msg), "m" (*cancel_p), "i" (EINTR));                        \
>    err;                                                                       
>       \
>  })
> -
> -
> -static void inline
> -INTR_MSG_BACK_OUT (struct i386_thread_state *state)
> -{
> -  extern const void _hurd_intr_rpc_msg_cx_sp;
> -  if (state->eip >= (natural_t) &_hurd_intr_rpc_msg_cx_sp)
> -    state->uesp = state->ecx;
> -  else
> -    state->ecx = state->uesp;
> -}
>  
>  #include "hurdfault.h"
>  
> diff --git a/sysdeps/mach/hurd/i386/trampoline.c 
> b/sysdeps/mach/hurd/i386/trampoline.c
> index 42c9d732..8f481e79 100644
> --- a/sysdeps/mach/hurd/i386/trampoline.c
> +++ b/sysdeps/mach/hurd/i386/trampoline.c
> @@ -89,8 +89,6 @@ _hurd_setup_sighandler (struct hurd_sigstate *ss, const 
> struct sigaction *action
>    void trampoline (void);
>    void rpc_wait_trampoline (void);
>    void firewall (void);
> -  extern const void _hurd_intr_rpc_msg_cx_sp;
> -  extern const void _hurd_intr_rpc_msg_sp_restored;
>    void *volatile sigsp;
>    struct sigcontext *scp;
>    struct
> @@ -146,25 +144,6 @@ _hurd_setup_sighandler (struct hurd_sigstate *ss, const 
> struct sigaction *action
>       interrupted RPC frame.  */
>    state->basic.esp = state->basic.uesp;
>  
> -  /* This code has intimate knowledge of the special mach_msg system call
> -     done in intr-msg.c; that code does (see intr-msg.h):
> -                                     movl %esp, %ecx
> -                                     leal ARGS, %esp
> -     _hurd_intr_rpc_msg_cx_sp:       movl $-25, %eax
> -     _hurd_intr_rpc_msg_do_trap:     lcall $7, $0
> -     _hurd_intr_rpc_msg_in_trap:     movl %ecx, %esp
> -     _hurd_intr_rpc_msg_sp_restored:
> -     We must check for the window during which %esp points at the
> -     mach_msg arguments.  The space below until %ecx is used by
> -     the _hurd_intr_rpc_mach_msg frame, and must not be clobbered.  */
> -  if (state->basic.eip >= (int) &_hurd_intr_rpc_msg_cx_sp
> -      && state->basic.eip < (int) &_hurd_intr_rpc_msg_sp_restored)
> -  /* The SP now points at the mach_msg args, but there is more stack
> -     space used below it.  The real SP is saved in %ecx; we must push the
> -     new frame below there (if not on the altstack), and restore that value 
> as
> -     the SP on sigreturn.  */
> -    state->basic.uesp = state->basic.ecx;
> -
>    if ((action->sa_flags & SA_ONSTACK)
>        && !(ss->sigaltstack.ss_flags & (SS_DISABLE|SS_ONSTACK)))
>      {
> -- 
> 2.39.2
> 

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