intr-msg / hurdsig looks broken, is my analysis correct?

intr-msg / hurdsig looks broken, is my analysis correct?

[Sergey Bugaev]

Hello yet again!

I'm looking at sysdeps/mach/hurd/i386/intr-msg.h in glibc -- with the
intention of eventually porting it to x86_64, but that's out of the
question for now, trying to understand it first. The three other
relevant source files for it seem to be:
1. hurd/intr-msg.c -- this is the main logic behind interruptible
messaging, intr-msg.h basically defining its arch-dependent part
(namely, INTR_MSG_TRAP). intr-msg.c itself does not seem to do any
low-level register/PC/jumps/thread_state magic, other than what
intr-msg.h does.
2. sysdeps/mach/hurd/i386/trampoline.c -- this has a piece of code
that claims to have "intimate knowledge of the special mach_msg system
call done in intr-msg.c", except that it clearly has a wrong idea
about what INTR_MSG_TRAP does, since the snippet of asm included in
the comment in trampoline.c does not match the relevant code of
INTR_MSG_TRAP.
3. hurd/hurdsig.c is what uses INTR_MSG_BACK_OUT. Here's the relevant
piece of code:

  if (state->basic.PC >= (uintptr_t) &_hurd_intr_rpc_msg_about_to
      && 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);
      state->basic.SYSRETURN = MACH_SEND_INTERRUPTED;
      *state_change = 1;
    }

Or in other words: this forcefully jumps the thread to right after the
syscall, pretending it has returned MACH_SEND_INTERRUPTED, but also
makes any arch-specific changes to the state (INTR_MSG_BACK_OUT).

Now, I have found commit 1d20f33ff4fb634310f27493b7b87d0b20f4a0b0 [0]
("hurd: Fix intr-msg parameter/stack kludge") that changed how
INTR_MSG_TRAP deals with the stack. Before the commit, it saved the
original esp into ecx, then set esp to, uh, something else, performed
the syscall, and reset esp back.

[0]: https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=1d20f33ff4fb634310f27493b7b87d0b20f4a0b0

This is explicitly supported by:
- trampoline.c, which detects that esp is not the "real" esp in
between ._hurd_intr_rpc_msg_cx_sp and ._hurd_intr_rpc_msg_sp_restored,
and seems to deal with that *somehow* by loading the real sp from ecx.
- INTR_MSG_BACK_OUT, which... seems to be a little misguided, even for
the pre-1d20f33ff4fb634310f27493b7b87d0b20f4a0b0 logic. Namely, it
does:

  if (state->eip >= (natural_t) &_hurd_intr_rpc_msg_cx_sp)
    state->uesp = state->ecx;
  else
    state->ecx = state->uesp;

but since hurdsig.c then makes the thread jump to
_hurd_intr_rpc_msg_in_trap, which was doing this:

_hurd_intr_rpc_msg_do_trap: lcall $7, $0
_hurd_intr_rpc_msg_in_trap: movl %ecx, %esp
                            .cfi_def_cfa_register %esp
_hurd_intr_rpc_msg_sp_restored:

...it makes no sense, at least as I'm seeing it, to set uesp = ecx,
since esp is going to be set back to ecx in a moment by the code
anyway. Setting ecx = uesp in the other branch makes much more sense
-- it essentially ensures that if movl %esp, %ecx has not been
executed yet, the movl %ecx, %esp will just no-op and not break
anything.

But that was all pre-1d20f33ff4fb634310f27493b7b87d0b20f4a0b0. Now,
while esp is still saved into ecx (with a comment: "TODO: remove this
ecx kludge, we don't need it any more"), there is code _before movl
%esp, %ecx_ that pushes args onto the stack, and the corresponding
code after the syscall that pops the args back off the stack. movl
%ecx, %esp is no longer present at all.

To me this reads like:
- the "ecx kludge" is indeed useless now, since esp is the same as ecx
during the syscall; but this also means that setting uesp = ecx or
vice versa will not break anything
- if hurdsig.c forcefully jumps the thread to
_hurd_intr_rpc_msg_in_trap before the thread is done pushing stuff
onto the stack, the thread will attempt to pop stuff off the stack
that was never pushed there => kaboom!

Or in other words: it seems to me that right now things would break
spectacularly if a signal arrives at just the right time. The logic
tried to handle this, but it's broken
post-1d20f33ff4fb634310f27493b7b87d0b20f4a0b0.

Does my reasoning make sense? Am I missing anything? Was this a simple
oversight of 1d20f33ff4fb634310f27493b7b87d0b20f4a0b0, or is there
some deeper plan that I'm not seeing to this? Are there any tests for
this at all?

As for fixing this:
- we should drop the ecx kludge indeed, meaning, remove everything
related to ecx from all of these files
- there should be an explicit way for hurdsig.c to tell INTR_MSG_TRAP
to not do the syscall. We could use the SYSRETURN register (eax),
which is to be clobbered anyway -- say, set it to 0 initially, and to
1 if the syscall is to be skipped
- INTR_MSG_TRAP should do this check *after* doing anything with the stack
- hurdsig.c would force-jump to after-the-syscall only if we're past
that check, like this:
if (PC >= _hurd_intr_rpc_msg_setup_done)
  {
    /* The thread has completed all the pre-syscall setup, so we
        can safely jump over the syscall */
    PC = &_hurd_intr_rpc_msg_in_trap;
  }
else
  {
    /* Cannot make the thread jump over the syscall, since the setup
is not yet complete, and
        this would confuse the code we would jump to. Instead, set the
flag that the thread will
        check just before doing the syscall, and skip the syscall itself */
    SYSRETURN = 1
  }
- INTR_MSG_BACK_OUT will become empty; we can still leave it in, or
drop it entirely

The above plan should work for any arch, not just i386. What do you think?

Sergey

Re: intr-msg / hurdsig looks broken, is my analysis correct?

[Samuel Thibault]

Hello,

Sergey Bugaev, le lun. 27 févr. 2023 16:39:19 +0300, a ecrit:
> Does my reasoning make sense? Am I missing anything? Was this a simple
> oversight of 1d20f33ff4fb634310f27493b7b87d0b20f4a0b0,

It most probably was, yes.

> Are there any tests for this at all?

It's very difficult to test since it's all about race conditions, with
the interruption happening exactly at the wrong moment.

> As for fixing this:
> - we should drop the ecx kludge indeed, meaning, remove everything
> related to ecx from all of these files

Actually, we probably need a kludge, precisely because of the pushes.

> - there should be an explicit way for hurdsig.c to tell INTR_MSG_TRAP
> to not do the syscall.

Yes, but as much as you'll try, you'll still have a very tiny window
(1-instruction at least) between the moment you check, and the moment
you run lcall. That's why the whole thing. It's simpler to just reset to
a known state.

We can use ecx for that, by saving esp to ecx before the pushes.
If interruption happens between the start of the pushes and the lcall,
we can easily revert the pushes by copying ecx to esp (i.e. without
having to care exactly how many pushes did get achieved), and directly
branch to _hurd_intr_rpc_msg_sp_restored.

> - INTR_MSG_BACK_OUT will become empty; we can still leave it in, or
> drop it entirely

BTW, I don't thing the else part of it is needed any more.

Samuel

Re: intr-msg / hurdsig looks broken, is my analysis correct?

[Sergey Bugaev]

On Mon, Feb 27, 2023 at 7:39 PM Samuel Thibault <samuel.thibault@gnu.org> wrote:
> > Are there any tests for this at all?
>
> It's very difficult to test since it's all about race conditions, with
> the interruption happening exactly at the wrong moment.

Right, but can't it be made reliable with some creative use of
ptrace/thread_state APIs? Set a breakpoint like gdb would do (by
inserting an int3...) on an instruction, have the thread run up to
there and stop, then send it a singal. Repeat for each instruction, to
make sure everything works no matter what state we catch the thread
in. Granted, this would not be trivial, but sounds doable & useful,
no? Not that I volunteer to implement this, of course no :D

> > As for fixing this:
> > - we should drop the ecx kludge indeed, meaning, remove everything
> > related to ecx from all of these files
>
> Actually, we probably need a kludge, precisely because of the pushes.
>
> > - there should be an explicit way for hurdsig.c to tell INTR_MSG_TRAP
> > to not do the syscall.
>
> Yes, but as much as you'll try, you'll still have a very tiny window
> (1-instruction at least) between the moment you check, and the moment
> you run lcall. That's why the whole thing. It's simpler to just reset to
> a known state.

Yes, I was not suggesting to only rely on an explicit flag, of course
that's racy. What I meant is more like:

_hurd_intr_rpc_msg_about_to:
    push stuff...
    any other potential setup...
/* From here on, it's safe to make us jump to after the syscall */
_hurd_intr_rpc_msg_setup_done:
/* Check for explicitly having been told not to do the syscall */
    test %eax, %eax
    jnz _hurd_intr_rpc_msg_in_trap
    potentially do some other stuff in here...
    movl $-25, %eax
_hurd_intr_rpc_msg_do_trap:
    lcall $7, $0
_hurd_intr_rpc_msg_in_trap:
    pop stuff...

If we are stopped *before* _hurd_intr_rpc_msg_setup_done, hurdsig.c
would know we are still to test %eax, %eax, so it can set eax = 1 and
resume us. If we are stopped *after* _hurd_intr_rpc_msg_setup_done, it
is safe to jump over the syscall, so it sets eip =
_hurd_intr_rpc_msg_in_trap.

> We can use ecx for that, by saving esp to ecx before the pushes.
> If interruption happens between the start of the pushes and the lcall,
> we can easily revert the pushes by copying ecx to esp (i.e. without
> having to care exactly how many pushes did get achieved), and directly
> branch to _hurd_intr_rpc_msg_sp_restored.

But you're right, that sounds like it would work too.

I shall take a shot at implementing it then :)

Sergey

Re: intr-msg / hurdsig looks broken, is my analysis correct?

[Samuel Thibault]

Sergey Bugaev, le lun. 27 févr. 2023 20:03:07 +0300, a ecrit:
> On Mon, Feb 27, 2023 at 7:39 PM Samuel Thibault <samuel.thibault@gnu.org> wrote:
> > > Are there any tests for this at all?
> >
> > It's very difficult to test since it's all about race conditions, with
> > the interruption happening exactly at the wrong moment.
> 
> Right, but can't it be made reliable with some creative use of
> ptrace/thread_state APIs? Set a breakpoint like gdb would do (by
> inserting an int3...) on an instruction, have the thread run up to
> there and stop, then send it a singal. Repeat for each instruction, to
> make sure everything works no matter what state we catch the thread
> in. Granted, this would not be trivial, but sounds doable & useful,
> no? Not that I volunteer to implement this, of course no :D

This looks quite complex and fragile, compared to just mangling the CPU
state...

(not that we haven't ever screwed with the CPU state, but more complex
approaches will be yet more fragile).

> > > As for fixing this:
> > > - we should drop the ecx kludge indeed, meaning, remove everything
> > > related to ecx from all of these files
> >
> > Actually, we probably need a kludge, precisely because of the pushes.
> >
> > > - there should be an explicit way for hurdsig.c to tell INTR_MSG_TRAP
> > > to not do the syscall.
> >
> > Yes, but as much as you'll try, you'll still have a very tiny window
> > (1-instruction at least) between the moment you check, and the moment
> > you run lcall. That's why the whole thing. It's simpler to just reset to
> > a known state.
> 
> Yes, I was not suggesting to only rely on an explicit flag, of course
> that's racy. What I meant is more like:

(add movl $0, %eax here)

> _hurd_intr_rpc_msg_about_to:
>     push stuff...
>     any other potential setup...
> /* From here on, it's safe to make us jump to after the syscall */
> _hurd_intr_rpc_msg_setup_done:
> /* Check for explicitly having been told not to do the syscall */
>     test %eax, %eax
>     jnz _hurd_intr_rpc_msg_in_trap
>     potentially do some other stuff in here...
>     movl $-25, %eax
> _hurd_intr_rpc_msg_do_trap:
>     lcall $7, $0
> _hurd_intr_rpc_msg_in_trap:
>     pop stuff...
> 
> If we are stopped *before* _hurd_intr_rpc_msg_setup_done, hurdsig.c
> would know we are still to test %eax, %eax, so it can set eax = 1 and
> resume us. If we are stopped *after* _hurd_intr_rpc_msg_setup_done, it
> is safe to jump over the syscall, so it sets eip =
> _hurd_intr_rpc_msg_in_trap.

That looks alright to me, indeed, and less kludgy :)

> I shall take a shot at implementing it then :)

Thanks!

Samuel

Re: intr-msg / hurdsig looks broken, is my analysis correct?

[Sergey Bugaev]

So I implemented this, and it seems to work — as in, signals arrive
and handlers run. It's extremely unlikely that I managed to hit the
interesting code path where eip is inside that piece of code, so who
knows whether that works or not — especially given this has been
apparently broken in glibc for years and nobody noticed.

But I'm not sending the patch; that's because I've got a much better idea now:

let's not do any of this at all! :)

Really, why would it matter whether eip is after
_hurd_intr_rpc_msg_about_to or not? What if it's 1 instruction before
that? We skip the call, pretending it was interrupted, but does it
really matter that we do that — can't we just admit that the signal
arrived before the call, after all that's exactly how it would look if
we catch the thread before it enters _hurd_intr_rpc_msg_about_to?

The answer, I think, is that the ecx kludge was the reason this code
in hurdsig.c and trampoline.c exists in the first place, since they
need to be aware and restore esp appropriately. It's not related to
skipping the message trap at all, it's only really about handling the
fact that the stack pointer is incorrect. Any other code that would
temporarily set esp to below the actual tip of the stack — no RPCing
needs to be involved — would need similar special-casing in these two
files.

And so since 1d20f33ff4fb634310f27493b7b87d0b20f4a0b0 has dropped the
root cause of the kludge, _all of this code_ can be dropped too.

I will un-implement that and see whether that really works; in the
meantime — does this make sense?

Sergey

[PATCH 1/2] hurd: Fully remove the ecx kludge

[Sergey Bugaev]

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

This was made more confusing because of a comment in hurdsig.c, which
seemed to indicate that if a thread is about to enter the mach_msg trap,
the trap has to be skipped while setting the return code as if the
thread has already entered the trap and the operation has been
interrupted. In fact, skipping the trap was not a goal in and of itself,
but rather a way to get the thread back into a consistent state with
respect to what the value of esp is and what the code expects it to be.
So it is fine to remove this, since the value of esp is no longer
inconsistent.

Fix the issue by removing all traces of "the ecx kludge", which also
simplifies things nicely and paves the way for a future x86_64 port of
this code.

Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---
 hurd/hurdsig.c                      | 23 +++++----------------
 sysdeps/mach/hurd/i386/intr-msg.h   | 31 ++++++-----------------------
 sysdeps/mach/hurd/i386/trampoline.c | 21 -------------------
 3 files changed, 11 insertions(+), 64 deletions(-)

diff --git a/hurd/hurdsig.c b/hurd/hurdsig.c
index ea79ffb5..3e759ae5 100644
--- a/hurd/hurdsig.c
+++ b/hurd/hurdsig.c
@@ -414,7 +414,6 @@ _hurdsig_abort_rpcs (struct hurd_sigstate *ss, int signo, int sigthread,
 		     struct machine_thread_all_state *state, int *state_change,
 		     void (*reply) (void))
 {
-  extern const void _hurd_intr_rpc_msg_about_to;
   extern const void _hurd_intr_rpc_msg_in_trap;
   mach_port_t rcv_port = MACH_PORT_NULL;
   mach_port_t intr_port;
@@ -430,23 +429,11 @@ _hurdsig_abort_rpcs (struct hurd_sigstate *ss, int signo, int sigthread,
      receive completes immediately or aborts.  */
   abort_thread (ss, state, reply);
 
-  if (state->basic.PC >= (uintptr_t) &_hurd_intr_rpc_msg_about_to
-      && 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);
-      state->basic.SYSRETURN = MACH_SEND_INTERRUPTED;
-      *state_change = 1;
-    }
-  else if (state->basic.PC == (uintptr_t) &_hurd_intr_rpc_msg_in_trap
-	   /* The thread was blocked in the system call.  After thread_abort,
-	      the return value register indicates what state the RPC was in
-	      when interrupted.  */
-	   && state->basic.SYSRETURN == MACH_RCV_INTERRUPTED)
+  if (state->basic.PC == (uintptr_t) &_hurd_intr_rpc_msg_in_trap
+      /* The thread was blocked in the system call.  After thread_abort, the
+         return value register indicates what state the RPC was in when
+         interrupted.  */
+      && state->basic.SYSRETURN == MACH_RCV_INTERRUPTED)
       {
 	/* The RPC request message was sent and the thread was waiting for
 	   the reply message; now the message receive has been aborted, so
diff --git a/sysdeps/mach/hurd/i386/intr-msg.h b/sysdeps/mach/hurd/i386/intr-msg.h
index 29cb4620..953e4553 100644
--- a/sysdeps/mach/hurd/i386/intr-msg.h
+++ b/sysdeps/mach/hurd/i386/intr-msg.h
@@ -24,12 +24,7 @@
 #define INTR_MSG_TRAP(msg, option, send_size, rcv_size, rcv_name, timeout, notify, cancel_p, intr_port_p) \
 ({									      \
   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_in_trap\n"				      \
-       ".globl _hurd_intr_rpc_msg_sp_restored\n"			      \
-       "_hurd_intr_rpc_msg_about_to:"					      \
+  asm (".globl _hurd_intr_rpc_msg_in_trap\n"				      \
        /* We need to make a last check of cancel, in case we got interrupted
           right before _hurd_intr_rpc_msg_about_to.  */			      \
        "				cmpl $0, %5\n"			      \
@@ -37,7 +32,7 @@
        /* We got interrupted, note so and return EINTR.  */		      \
        "				movl $0, %3\n"			      \
        "				movl %6, %%eax\n"		      \
-       "				jmp _hurd_intr_rpc_msg_sp_restored\n" \
+       "				jmp _hurd_intr_rpc_msg_out\n"	      \
        "_hurd_intr_rpc_msg_do:"						      \
        /* Ok, push the mach_msg_trap arguments.  */			      \
        "				pushl 24(%4)\n"			      \
@@ -48,10 +43,8 @@
        "				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_do_trap:	lcall $7, $0 # status in %0\n"	      \
+       "				movl $-25, %%eax\n"		      \
+       "				lcall $7, $0 # status in %0\n"	      \
        "_hurd_intr_rpc_msg_in_trap:"					      \
        /* Ok, clean the arguments and update OPTION and TIMEOUT.  */	      \
        "				addl $8, %%esp\n"		      \
@@ -59,23 +52,11 @@
        "				addl $12, %%esp\n"		      \
        "				popl %2\n"			      \
        "				addl $4, %%esp\n"		      \
-       "_hurd_intr_rpc_msg_sp_restored:"				      \
+       "_hurd_intr_rpc_msg_out:"					      \
        : "=a" (err), "+r" (option), "+r" (timeout), "=m" (*intr_port_p)	      \
-       : "r" (&msg), "m" (*cancel_p), "i" (EINTR)			      \
-       : "ecx");							      \
+       : "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;
-}
 \f
 #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

[PATCH 2/2] hurd: Fix some broken indentation

[Sergey Bugaev]

Also, fix a couple of typos. No functional change.

Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---
 hurd/hurdsig.c | 93 +++++++++++++++++++++++++-------------------------
 1 file changed, 47 insertions(+), 46 deletions(-)

diff --git a/hurd/hurdsig.c b/hurd/hurdsig.c
index 3e759ae5..1c85b29f 100644
--- a/hurd/hurdsig.c
+++ b/hurd/hurdsig.c
@@ -202,7 +202,7 @@ _hurd_sigstate_unlock (struct hurd_sigstate *ss)
 }
 libc_hidden_def (_hurd_sigstate_set_global_rcv)
 
-/* Retreive a thread's full set of pending signals, including the global
+/* Retrieve a thread's full set of pending signals, including the global
    ones if appropriate.  SS must be locked.  */
 sigset_t
 _hurd_sigstate_pending (const struct hurd_sigstate *ss)
@@ -233,7 +233,7 @@ sigstate_clear_pending (struct hurd_sigstate *ss, int signo)
 libc_hidden_def (_hurd_sigstate_lock)
 libc_hidden_def (_hurd_sigstate_unlock)
 
-/* Retreive a thread's action vector.  SS must be locked.  */
+/* Retrieve a thread's action vector.  SS must be locked.  */
 struct sigaction *
 _hurd_sigstate_actions (struct hurd_sigstate *ss)
 {
@@ -434,50 +434,51 @@ _hurdsig_abort_rpcs (struct hurd_sigstate *ss, int signo, int sigthread,
          return value register indicates what state the RPC was in when
          interrupted.  */
       && state->basic.SYSRETURN == MACH_RCV_INTERRUPTED)
-      {
-	/* The RPC request message was sent and the thread was waiting for
-	   the reply message; now the message receive has been aborted, so
-	   the mach_msg call will return MACH_RCV_INTERRUPTED.  We must tell
-	   the server to interrupt the pending operation.  The thread must
-	   wait for the reply message before running the signal handler (to
-	   guarantee that the operation has finished being interrupted), so
-	   our nonzero return tells the trampoline code to finish the message
-	   receive operation before running the handler.  */
-
-	mach_port_t *reply = interrupted_reply_port_location (ss->thread,
-							      state,
-							      sigthread);
-	error_t err = __interrupt_operation (intr_port, _hurdsig_interrupt_timeout);
-
-	if (err)
-	  {
-	    if (reply)
-	      {
-		/* The interrupt didn't work.
-		   Destroy the receive right the thread is blocked on.  */
-		__mach_port_destroy (__mach_task_self (), *reply);
-		*reply = MACH_PORT_NULL;
-	      }
-
-	    /* The system call return value register now contains
-	       MACH_RCV_INTERRUPTED; when mach_msg resumes, it will retry the
-	       call.  Since we have just destroyed the receive right, the
-	       retry will fail with MACH_RCV_INVALID_NAME.  Instead, just
-	       change the return value here to EINTR so mach_msg will not
-	       retry and the EINTR error code will propagate up.  */
-	    state->basic.SYSRETURN = EINTR;
-	    *state_change = 1;
-	  }
-	else if (reply)
-	  rcv_port = *reply;
-
-	/* All threads whose RPCs were interrupted by the interrupt_operation
-	   call above will retry their RPCs unless we clear SS->intr_port.
-	   So we clear it for the thread taking a signal when SA_RESTART is
-	   clear, so that its call returns EINTR.  */
-	if (! signo || !(_hurd_sigstate_actions (ss) [signo].sa_flags & SA_RESTART))
-	  ss->intr_port = MACH_PORT_NULL;
-      }
+    {
+      /* The RPC request message was sent and the thread was waiting for the
+         reply message; now the message receive has been aborted, so the
+         mach_msg call will return MACH_RCV_INTERRUPTED.  We must tell the
+         server to interrupt the pending operation.  The thread must wait for
+         the reply message before running the signal handler (to guarantee that
+         the operation has finished being interrupted), so our nonzero return
+         tells the trampoline code to finish the message receive operation
+         before running the handler.  */
+
+      mach_port_t *reply = interrupted_reply_port_location (ss->thread,
+                                                            state,
+                                                            sigthread);
+      error_t err = __interrupt_operation (intr_port,
+                                           _hurdsig_interrupt_timeout);
+
+      if (err)
+        {
+          if (reply)
+            {
+              /* The interrupt didn't work.
+                 Destroy the receive right the thread is blocked on.  */
+              __mach_port_destroy (__mach_task_self (), *reply);
+              *reply = MACH_PORT_NULL;
+            }
+
+          /* The system call return value register now contains
+             MACH_RCV_INTERRUPTED; when mach_msg resumes, it will retry the
+             call.  Since we have just destroyed the receive right, the retry
+             will fail with MACH_RCV_INVALID_NAME.  Instead, just change the
+             return value here to EINTR so mach_msg will not retry and the
+             EINTR error code will propagate up.  */
+          state->basic.SYSRETURN = EINTR;
+          *state_change = 1;
+	}
+      else if (reply)
+        rcv_port = *reply;
+
+      /* All threads whose RPCs were interrupted by the interrupt_operation
+         call above will retry their RPCs unless we clear SS->intr_port.  So we
+         clear it for the thread taking a signal when SA_RESTART is clear, so
+         that its call returns EINTR.  */
+      if (! signo || !(_hurd_sigstate_actions (ss) [signo].sa_flags & SA_RESTART))
+        ss->intr_port = MACH_PORT_NULL;
+    }
 
   return rcv_port;
 }
-- 
2.39.2

Re: intr-msg / hurdsig looks broken, is my analysis correct?

[Samuel Thibault]

Sergey Bugaev, le mar. 28 févr. 2023 18:53:05 +0300, a ecrit:
> Really, why would it matter whether eip is after
> _hurd_intr_rpc_msg_about_to or not? What if it's 1 instruction before
> that? We skip the call, pretending it was interrupted, but does it
> really matter that we do that — can't we just admit that the signal
> arrived before the call, after all that's exactly how it would look if
> we catch the thread before it enters _hurd_intr_rpc_msg_about_to?

Yes, but we still need to know whether it was "about to" make an RPC,
to determine whether to interrupt it, or let the code flow until an
cancellation point.

> Any other code that would temporarily set esp to below the actual tip
> of the stack — no RPCing needs to be involved — would need similar
> special-casing in these two files.

That's not supposed to happen. Really it should have never be handled.

But properly supporting cancellation is a reason for the kludge.

> And so since 1d20f33ff4fb634310f27493b7b87d0b20f4a0b0 has dropped the
> root cause of the kludge, _all of this code_ can be dropped too.
> 
> I will un-implement that and see whether that really works; in the
> meantime — does this make sense?

You'll most probably not easily meet the cases that matter: a
cancelation or interruption that is triggered right at the wrong time,
i.e. between the C check for cancellation etc., and the lcall.

Samuel

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

[Samuel Thibault]

Sergey Bugaev, le mar. 28 févr. 2023 22:44:08 +0300, a ecrit:
> @@ -430,23 +429,11 @@ _hurdsig_abort_rpcs (struct hurd_sigstate *ss, int signo, int sigthread,
>       receive completes immediately or aborts.  */
>    abort_thread (ss, state, reply);
>  
> -  if (state->basic.PC >= (uintptr_t) &_hurd_intr_rpc_msg_about_to
> -      && 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);
> -      state->basic.SYSRETURN = MACH_SEND_INTERRUPTED;
> -      *state_change = 1;
> -    }
> -  else if (state->basic.PC == (uintptr_t) &_hurd_intr_rpc_msg_in_trap
> -	   /* The thread was blocked in the system call.  After thread_abort,
> -	      the return value register indicates what state the RPC was in
> -	      when interrupted.  */
> -	   && state->basic.SYSRETURN == MACH_RCV_INTERRUPTED)
> +  if (state->basic.PC == (uintptr_t) &_hurd_intr_rpc_msg_in_trap
> +      /* The thread was blocked in the system call.  After thread_abort, the
> +         return value register indicates what state the RPC was in when
> +         interrupted.  */
> +      && state->basic.SYSRETURN == MACH_RCV_INTERRUPTED)
>        {
>  	/* The RPC request message was sent and the thread was waiting for
>  	   the reply message; now the message receive has been aborted, so

No, that's not enough, it's racy: if cancellation/interrupt
happens between the last C check and actually running lcall, the
cancellation/interrupt will be lost.

We do need to properly check for the eip case between the last check for
these, and lcall.

Samuel

Re: intr-msg / hurdsig looks broken, is my analysis correct?

[Sergey Bugaev]

On Wed, Mar 1, 2023 at 12:09 AM Samuel Thibault <samuel.thibault@gnu.org> wrote:
>
> Sergey Bugaev, le mar. 28 févr. 2023 18:53:05 +0300, a ecrit:
> > Really, why would it matter whether eip is after
> > _hurd_intr_rpc_msg_about_to or not? What if it's 1 instruction before
> > that? We skip the call, pretending it was interrupted, but does it
> > really matter that we do that — can't we just admit that the signal
> > arrived before the call, after all that's exactly how it would look if
> > we catch the thread before it enters _hurd_intr_rpc_msg_about_to?
>
> Yes, but we still need to know whether it was "about to" make an RPC,
> to determine whether to interrupt it, or let the code flow until an
> cancellation point.

Ah, right, cancellation! That explains it. Thank you for correcting me.

I was only thinking of delivering a signal, in which case, yes, it
should be fine if we just say that the signal arrived before the RPC
and did not interrupt it. But in case of cancellation (as in --
hurd_thread_cancel) we indeed need to not enter the RPC (nor *any*
interruptible RPCs?) once we're cancelled.

So then indeed we need _hurdsig_abort_rpcs to handle the case where
the eip is between the very last check for cancellation and the trap,
and make it skip over the trap. Back to my original plan then, luckily
I have the code saved in a git stash.

* eip < _hurd_intr_rpc_msg_about_to => thread is still to check
ss->cancel, so just set ss->cancel = 1;
* _hurd_intr_rpc_msg_about_to <= eip < _hurd_intr_rpc_msg_setup_done
=> thread is still to check eax, so set eax = MACH_SEND_INTERRUPTED;
* _hurd_intr_rpc_msg_setup_done <= eip => the thread will not check
anything, but we can make it skip over the trap safely, so do that.

Sergey

Re: intr-msg / hurdsig looks broken, is my analysis correct?

[Samuel Thibault]

Sergey Bugaev, le mer. 01 mars 2023 12:21:17 +0300, a ecrit:
> * eip < _hurd_intr_rpc_msg_about_to => thread is still to check
> ss->cancel, so just set ss->cancel = 1;
> * _hurd_intr_rpc_msg_about_to <= eip < _hurd_intr_rpc_msg_setup_done
> => thread is still to check eax, so set eax = MACH_SEND_INTERRUPTED;
> * _hurd_intr_rpc_msg_setup_done <= eip => the thread will not check
> anything, but we can make it skip over the trap safely, so do that.

Yup!

Samuel

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

[Sergey Bugaev]

"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;
-}
 \f
 #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

[PATCH 2/2] hurd: Fix some broken indentation

[Sergey Bugaev]

Also, fix a couple of typos. No functional change.

Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---
 hurd/hurdsig.c | 101 +++++++++++++++++++++++++------------------------
 1 file changed, 51 insertions(+), 50 deletions(-)

diff --git a/hurd/hurdsig.c b/hurd/hurdsig.c
index 5ff0a91f..85bd46b5 100644
--- a/hurd/hurdsig.c
+++ b/hurd/hurdsig.c
@@ -202,7 +202,7 @@ _hurd_sigstate_unlock (struct hurd_sigstate *ss)
 }
 libc_hidden_def (_hurd_sigstate_set_global_rcv)
 
-/* Retreive a thread's full set of pending signals, including the global
+/* Retrieve a thread's full set of pending signals, including the global
    ones if appropriate.  SS must be locked.  */
 sigset_t
 _hurd_sigstate_pending (const struct hurd_sigstate *ss)
@@ -233,7 +233,7 @@ sigstate_clear_pending (struct hurd_sigstate *ss, int signo)
 libc_hidden_def (_hurd_sigstate_lock)
 libc_hidden_def (_hurd_sigstate_unlock)
 
-/* Retreive a thread's action vector.  SS must be locked.  */
+/* Retrieve a thread's action vector.  SS must be locked.  */
 struct sigaction *
 _hurd_sigstate_actions (struct hurd_sigstate *ss)
 {
@@ -451,54 +451,55 @@ _hurdsig_abort_rpcs (struct hurd_sigstate *ss, int signo, int sigthread,
       *state_change = 1;
     }
   else if (state->basic.PC == (uintptr_t) &_hurd_intr_rpc_msg_in_trap
-	   /* The thread was blocked in the system call.  After thread_abort,
-	      the return value register indicates what state the RPC was in
-	      when interrupted.  */
-	   && state->basic.SYSRETURN == MACH_RCV_INTERRUPTED)
-      {
-	/* The RPC request message was sent and the thread was waiting for
-	   the reply message; now the message receive has been aborted, so
-	   the mach_msg call will return MACH_RCV_INTERRUPTED.  We must tell
-	   the server to interrupt the pending operation.  The thread must
-	   wait for the reply message before running the signal handler (to
-	   guarantee that the operation has finished being interrupted), so
-	   our nonzero return tells the trampoline code to finish the message
-	   receive operation before running the handler.  */
-
-	mach_port_t *reply = interrupted_reply_port_location (ss->thread,
-							      state,
-							      sigthread);
-	error_t err = __interrupt_operation (intr_port, _hurdsig_interrupt_timeout);
-
-	if (err)
-	  {
-	    if (reply)
-	      {
-		/* The interrupt didn't work.
-		   Destroy the receive right the thread is blocked on.  */
-		__mach_port_destroy (__mach_task_self (), *reply);
-		*reply = MACH_PORT_NULL;
-	      }
-
-	    /* The system call return value register now contains
-	       MACH_RCV_INTERRUPTED; when mach_msg resumes, it will retry the
-	       call.  Since we have just destroyed the receive right, the
-	       retry will fail with MACH_RCV_INVALID_NAME.  Instead, just
-	       change the return value here to EINTR so mach_msg will not
-	       retry and the EINTR error code will propagate up.  */
-	    state->basic.SYSRETURN = EINTR;
-	    *state_change = 1;
-	  }
-	else if (reply)
-	  rcv_port = *reply;
-
-	/* All threads whose RPCs were interrupted by the interrupt_operation
-	   call above will retry their RPCs unless we clear SS->intr_port.
-	   So we clear it for the thread taking a signal when SA_RESTART is
-	   clear, so that its call returns EINTR.  */
-	if (! signo || !(_hurd_sigstate_actions (ss) [signo].sa_flags & SA_RESTART))
-	  ss->intr_port = MACH_PORT_NULL;
-      }
+           /* The thread was blocked in the system call.  After thread_abort,
+              the return value register indicates what state the RPC was in
+              when interrupted.  */
+           && state->basic.SYSRETURN == MACH_RCV_INTERRUPTED)
+    {
+      /* The RPC request message was sent and the thread was waiting for the
+         reply message; now the message receive has been aborted, so the
+         mach_msg call will return MACH_RCV_INTERRUPTED.  We must tell the
+         server to interrupt the pending operation.  The thread must wait for
+         the reply message before running the signal handler (to guarantee that
+         the operation has finished being interrupted), so our nonzero return
+         tells the trampoline code to finish the message receive operation
+         before running the handler.  */
+
+      mach_port_t *reply = interrupted_reply_port_location (ss->thread,
+                                                            state,
+                                                            sigthread);
+      error_t err = __interrupt_operation (intr_port,
+                                           _hurdsig_interrupt_timeout);
+
+      if (err)
+        {
+          if (reply)
+            {
+              /* The interrupt didn't work.
+                 Destroy the receive right the thread is blocked on.  */
+              __mach_port_destroy (__mach_task_self (), *reply);
+              *reply = MACH_PORT_NULL;
+            }
+
+          /* The system call return value register now contains
+             MACH_RCV_INTERRUPTED; when mach_msg resumes, it will retry the
+             call.  Since we have just destroyed the receive right, the retry
+             will fail with MACH_RCV_INVALID_NAME.  Instead, just change the
+             return value here to EINTR so mach_msg will not retry and the
+             EINTR error code will propagate up.  */
+          state->basic.SYSRETURN = EINTR;
+          *state_change = 1;
+	}
+      else if (reply)
+        rcv_port = *reply;
+
+      /* All threads whose RPCs were interrupted by the interrupt_operation
+         call above will retry their RPCs unless we clear SS->intr_port.  So we
+         clear it for the thread taking a signal when SA_RESTART is clear, so
+         that its call returns EINTR.  */
+      if (! signo || !(_hurd_sigstate_actions (ss) [signo].sa_flags & SA_RESTART))
+        ss->intr_port = MACH_PORT_NULL;
+    }
 
   return rcv_port;
 }
-- 
2.39.2

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

[Luca]

Hi,

Il 01/03/23 17:23, Sergey Bugaev ha scritto:
> "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.

I still have to fully understand the existing code, so this might be 
something completely wrong... but if interrupting an rpc is a complex 
thing to do reliably in user space, why not add some kernel support? 
Also, how do you test this code?

For example, we could add a thread_interrupt_rpc() where the kernel 
could just check the state of the suspended thread:
* if the thread has a continuation set it means it still has to finish 
the send/recv operation, so we replace the continuation, discard the 
previous syscall state, and we just return an RPC_INTERRUPTED_BY_USER 
code or MACH_SEND/RCV_INTERRUPTED.
* if the thread finished the syscall, but it was de-scheduled before 
returning to userspace (by AST) it will just return normally, as the 
syscall was executed completely.

There could also be a new option to mach_msg(), so we can have 
uninterruptible mach_msg() if needed, or reuse MACH_SEND/RCV_INTERRUPT.

Does it make sense?

Also, if I understand correctly, in case the thread finished the syscall 
and successfully received a message, but didn't return to userspace yet, 
isn't there the risk of losing the message, with the current approach?


Luca

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

[Samuel Thibault]

Luca, le mer. 01 mars 2023 19:01:02 +0100, a ecrit:
> I still have to fully understand the existing code, so this might be
> something completely wrong... but if interrupting an rpc is a complex thing
> to do reliably in user space, why not add some kernel support? Also, how do
> you test this code?

Mach already provides support for interruption. But it cannot deal
with the user code itself. One way could be to add a pointer to the
mach_msg call for the kernel to check the cancel flag itself, but that
becomes relatively dirty, compared to just dealing with the matter in
userland, which on the other hand allows userland to introduce whatever
interruption/cancelation/signal/etc. notions at will.

> Also, if I understand correctly, in case the thread finished the syscall and
> successfully received a message, but didn't return to userspace yet, isn't
> there the risk of losing the message, with the current approach?

IIRC that part is not interruptible, so either we get a message, or we
get interrupted, but not both.

Samuel

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;
> -}
>  \f
>  #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.

Re: [PATCH 2/2] hurd: Fix some broken indentation

[Samuel Thibault]

Applied, thanks!

Sergey Bugaev, le mer. 01 mars 2023 19:23:55 +0300, a ecrit:
> Also, fix a couple of typos. No functional change.
> 
> Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
> ---
>  hurd/hurdsig.c | 101 +++++++++++++++++++++++++------------------------
>  1 file changed, 51 insertions(+), 50 deletions(-)
> 
> diff --git a/hurd/hurdsig.c b/hurd/hurdsig.c
> index 5ff0a91f..85bd46b5 100644
> --- a/hurd/hurdsig.c
> +++ b/hurd/hurdsig.c
> @@ -202,7 +202,7 @@ _hurd_sigstate_unlock (struct hurd_sigstate *ss)
>  }
>  libc_hidden_def (_hurd_sigstate_set_global_rcv)
>  
> -/* Retreive a thread's full set of pending signals, including the global
> +/* Retrieve a thread's full set of pending signals, including the global
>     ones if appropriate.  SS must be locked.  */
>  sigset_t
>  _hurd_sigstate_pending (const struct hurd_sigstate *ss)
> @@ -233,7 +233,7 @@ sigstate_clear_pending (struct hurd_sigstate *ss, int signo)
>  libc_hidden_def (_hurd_sigstate_lock)
>  libc_hidden_def (_hurd_sigstate_unlock)
>  
> -/* Retreive a thread's action vector.  SS must be locked.  */
> +/* Retrieve a thread's action vector.  SS must be locked.  */
>  struct sigaction *
>  _hurd_sigstate_actions (struct hurd_sigstate *ss)
>  {
> @@ -451,54 +451,55 @@ _hurdsig_abort_rpcs (struct hurd_sigstate *ss, int signo, int sigthread,
>        *state_change = 1;
>      }
>    else if (state->basic.PC == (uintptr_t) &_hurd_intr_rpc_msg_in_trap
> -	   /* The thread was blocked in the system call.  After thread_abort,
> -	      the return value register indicates what state the RPC was in
> -	      when interrupted.  */
> -	   && state->basic.SYSRETURN == MACH_RCV_INTERRUPTED)
> -      {
> -	/* The RPC request message was sent and the thread was waiting for
> -	   the reply message; now the message receive has been aborted, so
> -	   the mach_msg call will return MACH_RCV_INTERRUPTED.  We must tell
> -	   the server to interrupt the pending operation.  The thread must
> -	   wait for the reply message before running the signal handler (to
> -	   guarantee that the operation has finished being interrupted), so
> -	   our nonzero return tells the trampoline code to finish the message
> -	   receive operation before running the handler.  */
> -
> -	mach_port_t *reply = interrupted_reply_port_location (ss->thread,
> -							      state,
> -							      sigthread);
> -	error_t err = __interrupt_operation (intr_port, _hurdsig_interrupt_timeout);
> -
> -	if (err)
> -	  {
> -	    if (reply)
> -	      {
> -		/* The interrupt didn't work.
> -		   Destroy the receive right the thread is blocked on.  */
> -		__mach_port_destroy (__mach_task_self (), *reply);
> -		*reply = MACH_PORT_NULL;
> -	      }
> -
> -	    /* The system call return value register now contains
> -	       MACH_RCV_INTERRUPTED; when mach_msg resumes, it will retry the
> -	       call.  Since we have just destroyed the receive right, the
> -	       retry will fail with MACH_RCV_INVALID_NAME.  Instead, just
> -	       change the return value here to EINTR so mach_msg will not
> -	       retry and the EINTR error code will propagate up.  */
> -	    state->basic.SYSRETURN = EINTR;
> -	    *state_change = 1;
> -	  }
> -	else if (reply)
> -	  rcv_port = *reply;
> -
> -	/* All threads whose RPCs were interrupted by the interrupt_operation
> -	   call above will retry their RPCs unless we clear SS->intr_port.
> -	   So we clear it for the thread taking a signal when SA_RESTART is
> -	   clear, so that its call returns EINTR.  */
> -	if (! signo || !(_hurd_sigstate_actions (ss) [signo].sa_flags & SA_RESTART))
> -	  ss->intr_port = MACH_PORT_NULL;
> -      }
> +           /* The thread was blocked in the system call.  After thread_abort,
> +              the return value register indicates what state the RPC was in
> +              when interrupted.  */
> +           && state->basic.SYSRETURN == MACH_RCV_INTERRUPTED)
> +    {
> +      /* The RPC request message was sent and the thread was waiting for the
> +         reply message; now the message receive has been aborted, so the
> +         mach_msg call will return MACH_RCV_INTERRUPTED.  We must tell the
> +         server to interrupt the pending operation.  The thread must wait for
> +         the reply message before running the signal handler (to guarantee that
> +         the operation has finished being interrupted), so our nonzero return
> +         tells the trampoline code to finish the message receive operation
> +         before running the handler.  */
> +
> +      mach_port_t *reply = interrupted_reply_port_location (ss->thread,
> +                                                            state,
> +                                                            sigthread);
> +      error_t err = __interrupt_operation (intr_port,
> +                                           _hurdsig_interrupt_timeout);
> +
> +      if (err)
> +        {
> +          if (reply)
> +            {
> +              /* The interrupt didn't work.
> +                 Destroy the receive right the thread is blocked on.  */
> +              __mach_port_destroy (__mach_task_self (), *reply);
> +              *reply = MACH_PORT_NULL;
> +            }
> +
> +          /* The system call return value register now contains
> +             MACH_RCV_INTERRUPTED; when mach_msg resumes, it will retry the
> +             call.  Since we have just destroyed the receive right, the retry
> +             will fail with MACH_RCV_INVALID_NAME.  Instead, just change the
> +             return value here to EINTR so mach_msg will not retry and the
> +             EINTR error code will propagate up.  */
> +          state->basic.SYSRETURN = EINTR;
> +          *state_change = 1;
> +	}
> +      else if (reply)
> +        rcv_port = *reply;
> +
> +      /* All threads whose RPCs were interrupted by the interrupt_operation
> +         call above will retry their RPCs unless we clear SS->intr_port.  So we
> +         clear it for the thread taking a signal when SA_RESTART is clear, so
> +         that its call returns EINTR.  */
> +      if (! signo || !(_hurd_sigstate_actions (ss) [signo].sa_flags & SA_RESTART))
> +        ss->intr_port = MACH_PORT_NULL;
> +    }
>  
>    return rcv_port;
>  }
> -- 
> 2.39.2
> 

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