[PATCH 1/5] hurd: Fix restoring reply port in sigreturn

[PATCH 1/5] hurd: Fix restoring reply port in sigreturn

[Sergey Bugaev]

We must not use the user's reply port (scp->sc_reply_port) for any of
our own RPCs, otherwise various things break. So, use MACH_PORT_DEAD as
a reply port when destroying our reply port, and make sure to do this
after _hurd_sigstate_unlock (), which may do a gsync_wake () RPC.

Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---
 sysdeps/mach/hurd/i386/sigreturn.c | 35 ++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/sysdeps/mach/hurd/i386/sigreturn.c b/sysdeps/mach/hurd/i386/sigreturn.c
index 4f196710..a0fc8891 100644
--- a/sysdeps/mach/hurd/i386/sigreturn.c
+++ b/sysdeps/mach/hurd/i386/sigreturn.c
@@ -26,11 +26,29 @@ register int *sp asm ("%esp");
 /* This is run on the thread stack after restoring it, to be able to
    unlock SS off sigstack.  */
 static void
-__sigreturn2 (int *usp)
+__sigreturn2 (int *usp, struct sigcontext *scp)
 {
+  mach_port_t reply_port;
   struct hurd_sigstate *ss = _hurd_self_sigstate ();
   _hurd_sigstate_unlock (ss);
 
+  /* Destroy the MiG reply port used by the signal handler, and restore the
+     reply port in use by the thread when interrupted.
+
+     We cannot use the original reply port for our RPCs that we do here, since
+     we could unexpectedly receive/consume a reply message meant for the user
+     (in particular, msg_sig_post_reply), and also since we would deallocate
+     the port if *our* RPC fails, which we don't want to do since the user
+     still has the old name.  And so, temporarily set MACH_PORT_DEAD as our
+     reply name, and make sure destroying the port is the very last RPC we
+     do.  */
+  reply_port = THREAD_GETMEM (THREAD_SELF, reply_port);
+  THREAD_SETMEM (THREAD_SELF, reply_port, MACH_PORT_DEAD);
+  if (MACH_PORT_VALID (reply_port))
+    (void) __mach_port_mod_refs (__mach_task_self (), reply_port,
+                                 MACH_PORT_RIGHT_RECEIVE, -1);
+  THREAD_SETMEM (THREAD_SELF, reply_port, scp->sc_reply_port);
+
   sp = usp;
 #define A(line) asm volatile (#line)
   /* The members in the sigcontext are arranged in this order
@@ -58,7 +76,6 @@ __sigreturn (struct sigcontext *scp)
 {
   struct hurd_sigstate *ss;
   struct hurd_userlink *link = (void *) &scp[1];
-  mach_port_t reply_port;
 
   if (scp == NULL || (scp->sc_mask & _SIG_CANT_MASK))
     {
@@ -98,13 +115,6 @@ __sigreturn (struct sigcontext *scp)
   if (scp->sc_onstack)
     ss->sigaltstack.ss_flags &= ~SS_ONSTACK;
 
-  /* Destroy the MiG reply port used by the signal handler, and restore the
-     reply port in use by the thread when interrupted.  */
-  reply_port = THREAD_GETMEM (THREAD_SELF, reply_port);
-  THREAD_SETMEM (THREAD_SELF, reply_port, scp->sc_reply_port);
-  if (MACH_PORT_VALID (reply_port))
-    __mach_port_destroy (__mach_task_self (), reply_port);
-
   if (scp->sc_fpused)
     /* Restore the FPU state.  Mach conveniently stores the state
        in the format the i387 `frstor' instruction uses to restore it.  */
@@ -115,15 +125,16 @@ __sigreturn (struct sigcontext *scp)
        copy the registers onto the user's stack, switch there, pop and
        return.  */
 
-    int *usp = (int *) scp->sc_uesp;
+    int usp_arg, *usp = (int *) scp->sc_uesp;
 
     *--usp = scp->sc_eip;
     *--usp = scp->sc_efl;
     memcpy (usp -= 12, &scp->sc_i386_thread_state, 12 * sizeof (int));
+    usp_arg = (int) usp;
 
+    *--usp = (int) scp;
     /* Pass usp to __sigreturn2 so it can unwind itself easily.  */
-    *(usp-1) = (int) usp;
-    --usp;
+    *--usp = usp_arg;
     /* Bogus return address for __sigreturn2 */
     *--usp = 0;
     *--usp = (int) __sigreturn2;
-- 
2.39.2

[PATCH 2/5] hurd: Microoptimize sigreturn

[Sergey Bugaev]

Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---

I'll understand if this patch will be rejected, since this is not a hot path.
But I just couldn't help myself when I saw what this was being compiled to!
Much cleaner now.

If you don't like THREAD_GETMEM (THREAD_SELF, _hurd_sigstate), we could have
_hurd_self_sigstate_fast () or something.

 sysdeps/mach/hurd/i386/sigreturn.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/sysdeps/mach/hurd/i386/sigreturn.c b/sysdeps/mach/hurd/i386/sigreturn.c
index a0fc8891..6ad8a998 100644
--- a/sysdeps/mach/hurd/i386/sigreturn.c
+++ b/sysdeps/mach/hurd/i386/sigreturn.c
@@ -29,7 +29,12 @@ static void
 __sigreturn2 (int *usp, struct sigcontext *scp)
 {
   mach_port_t reply_port;
-  struct hurd_sigstate *ss = _hurd_self_sigstate ();
+  struct hurd_sigstate *ss;
+
+  /* We know the sigstate must be initialized, but the compiler does not.
+     Help it out a little bit by eliding the check that _hurd_self_sigstate
+     makes otherwise.  */
+  ss = THREAD_GETMEM (THREAD_SELF, _hurd_sigstate);
   _hurd_sigstate_unlock (ss);
 
   /* Destroy the MiG reply port used by the signal handler, and restore the
@@ -44,7 +49,7 @@ __sigreturn2 (int *usp, struct sigcontext *scp)
      do.  */
   reply_port = THREAD_GETMEM (THREAD_SELF, reply_port);
   THREAD_SETMEM (THREAD_SELF, reply_port, MACH_PORT_DEAD);
-  if (MACH_PORT_VALID (reply_port))
+  if (__glibc_likely (MACH_PORT_VALID (reply_port)))
     (void) __mach_port_mod_refs (__mach_task_self (), reply_port,
                                  MACH_PORT_RIGHT_RECEIVE, -1);
   THREAD_SETMEM (THREAD_SELF, reply_port, scp->sc_reply_port);
@@ -69,6 +74,7 @@ __sigreturn2 (int *usp, struct sigcontext *scp)
   /* Firewall.  */
   A (hlt);
 #undef A
+  __builtin_unreachable ();
 }
 
 int
@@ -77,13 +83,14 @@ __sigreturn (struct sigcontext *scp)
   struct hurd_sigstate *ss;
   struct hurd_userlink *link = (void *) &scp[1];
 
-  if (scp == NULL || (scp->sc_mask & _SIG_CANT_MASK))
+  if (__glibc_unlikely (scp == NULL || (scp->sc_mask & _SIG_CANT_MASK)))
     {
       errno = EINVAL;
       return -1;
     }
 
-  ss = _hurd_self_sigstate ();
+  /* Same as above, microoptimize _hurd_self_sigstate.  */
+  ss = THREAD_GETMEM (THREAD_SELF, _hurd_sigstate);
   _hurd_sigstate_lock (ss);
 
   /* Remove the link on the `active resources' chain added by
-- 
2.39.2

[PATCH 3/5] hurd: Implement sigreturn for x86_64

[Sergey Bugaev]

Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---
This supersedes the previous version of this patch, and contains the same
improvements the last two patches made to the i386 version.

 sysdeps/mach/hurd/x86_64/sigreturn.c | 166 +++++++++++++++++++++++++++
 1 file changed, 166 insertions(+)
 create mode 100644 sysdeps/mach/hurd/x86_64/sigreturn.c

diff --git a/sysdeps/mach/hurd/x86_64/sigreturn.c b/sysdeps/mach/hurd/x86_64/sigreturn.c
new file mode 100644
index 00000000..cba2eb77
--- /dev/null
+++ b/sysdeps/mach/hurd/x86_64/sigreturn.c
@@ -0,0 +1,166 @@
+/* Copyright (C) 1991-2023 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#include <hurd.h>
+#include <hurd/signal.h>
+#include <hurd/msg.h>
+#include <stdlib.h>
+
+/* This is run on the thread stack after restoring it, to be able to
+   unlock SS off sigstack.  */
+void
+__sigreturn2 (struct hurd_sigstate *ss, uintptr_t *usp,
+              struct sigcontext *scp)
+{
+  mach_port_t reply_port;
+  _hurd_sigstate_unlock (ss);
+
+  /* Destroy the MiG reply port used by the signal handler, and restore the
+     reply port in use by the thread when interrupted.
+
+     We cannot use the original reply port for our RPCs that we do here, since
+     we could unexpectedly receive/consume a reply message meant for the user
+     (in particular, msg_sig_post_reply), and also since we would deallocate
+     the port if *our* RPC fails, which we don't want to do since the user
+     still has the old name.  And so, temporarily set MACH_PORT_DEAD as our
+     reply name, and make sure destroying the port is the very last RPC we
+     do.  */
+  reply_port = THREAD_GETMEM (THREAD_SELF, reply_port);
+  THREAD_SETMEM (THREAD_SELF, reply_port, MACH_PORT_DEAD);
+  if (__glibc_likely (MACH_PORT_VALID (reply_port)))
+    (void) __mach_port_mod_refs (__mach_task_self (), reply_port,
+                                 MACH_PORT_RIGHT_RECEIVE, -1);
+  THREAD_SETMEM (THREAD_SELF, reply_port, scp->sc_reply_port);
+
+  asm volatile (
+                /* Point the stack to the register dump.  */
+                "movq %0, %%rsp\n"
+                /* Pop off the registers.  */
+                "popq %%r8\n"
+                "popq %%r9\n"
+                "popq %%r10\n"
+                "popq %%r11\n"
+                "popq %%r12\n"
+                "popq %%r13\n"
+                "popq %%r14\n"
+                "popq %%r15\n"
+                "popq %%rdi\n"
+                "popq %%rsi\n"
+                "popq %%rbp\n"
+                "popq %%rbx\n"
+                "popq %%rdx\n"
+                "popq %%rcx\n"
+                "popq %%rax\n"
+                "popfq\n"
+                /* Restore %rip and %rsp with a single instruction.  */
+                "retq $128" :
+                : "rm" (usp));
+  __builtin_unreachable ();
+}
+
+int
+__sigreturn (struct sigcontext *scp)
+{
+  struct hurd_sigstate *ss;
+  struct hurd_userlink *link = (void *) &scp[1];
+  mach_port_t reply_port;
+
+  if (__glibc_unlikely (scp == NULL || (scp->sc_mask & _SIG_CANT_MASK)))
+    {
+      errno = EINVAL;
+      return -1;
+    }
+
+  /* We know the sigstate must be initialized, but the compiler does not.
+     Help it out a little bit by eliding the check that _hurd_self_sigstate
+     makes otherwise.  */
+  ss = THREAD_GETMEM (THREAD_SELF, _hurd_sigstate);
+  _hurd_sigstate_lock (ss);
+
+  /* Remove the link on the `active resources' chain added by
+     _hurd_setup_sighandler.  Its purpose was to make sure
+     that we got called; now we have, it is done.  */
+  _hurd_userlink_unlink (link);
+
+  /* Restore the set of blocked signals, and the intr_port slot.  */
+  ss->blocked = scp->sc_mask;
+  ss->intr_port = scp->sc_intr_port;
+
+  /* Check for pending signals that were blocked by the old set.  */
+  if (_hurd_sigstate_pending (ss) & ~ss->blocked)
+    {
+      /* There are pending signals that just became unblocked.  Wake up the
+	 signal thread to deliver them.  But first, squirrel away SCP where
+	 the signal thread will notice it if it runs another handler, and
+	 arrange to have us called over again in the new reality.  */
+      ss->context = scp;
+      _hurd_sigstate_unlock (ss);
+      __msg_sig_post (_hurd_msgport, 0, 0, __mach_task_self ());
+      /* If a pending signal was handled, sig_post never returned.
+	 If it did return, the pending signal didn't run a handler;
+	 proceed as usual.  */
+      _hurd_sigstate_lock (ss);
+      ss->context = NULL;
+    }
+
+  if (scp->sc_onstack)
+    ss->sigaltstack.ss_flags &= ~SS_ONSTACK;
+
+  if (scp->sc_fpused)
+    /* Restore the FPU state.  Mach conveniently stores the state
+       in the format the i387 `frstor' instruction uses to restore it.  */
+    asm volatile ("frstor %0" : : "m" (scp->sc_fpsave));
+
+  {
+    /* There are convenient instructions to pop state off the stack, so we
+       copy the registers onto the user's stack, switch there, pop and
+       return.  */
+
+    uintptr_t *usp = (uintptr_t *) scp->sc_ursp - 128;
+
+    *--usp = scp->sc_rip;
+    *--usp = scp->sc_rfl;
+    *--usp = scp->sc_rax;
+    *--usp = scp->sc_rcx;
+    *--usp = scp->sc_rdx;
+    *--usp = scp->sc_rbx;
+    *--usp = scp->sc_rbp;
+    *--usp = scp->sc_rsi;
+    *--usp = scp->sc_rdi;
+    *--usp = scp->sc_r15;
+    *--usp = scp->sc_r14;
+    *--usp = scp->sc_r13;
+    *--usp = scp->sc_r12;
+    *--usp = scp->sc_r11;
+    *--usp = scp->sc_r10;
+    *--usp = scp->sc_r9;
+    *--usp = scp->sc_r8;
+
+    /* Switch to the user's stack that we have just prepared, and call
+       __sigreturn2.  Clobber "memory" to make sure GCC flushes the stack
+       setup to actual memory.  We align the stack as per the ABI, but pass
+       the original usp to __sigreturn2 as an argument.  */
+    asm volatile ("movq %1, %%rsp\n"
+                  "andq $-16, %%rsp\n"
+                  "call __sigreturn2" :
+                  : "D" (ss), "S" (usp), "d" (scp)
+                  : "memory");
+    __builtin_unreachable ();
+  }
+}
+
+weak_alias (__sigreturn, sigreturn)
-- 
2.39.2

[PATCH 4/5] hurd: Simplify _S_catch_exception_raise

[Sergey Bugaev]

_hurd_thread_sigstate () already handles finding an existing sigstate
before allocating a new one, so just use that. Bonus: this will only
lock the _hurd_siglock once.

Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---
 hurd/catch-exc.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/hurd/catch-exc.c b/hurd/catch-exc.c
index 5ee2233a..d375bf67 100644
--- a/hurd/catch-exc.c
+++ b/hurd/catch-exc.c
@@ -58,13 +58,7 @@ _S_catch_exception_raise (mach_port_t port,
   _hurd_exception2signal (&d, &signo);
 
   /* Find the sigstate structure for the faulting thread.  */
-  __mutex_lock (&_hurd_siglock);
-  for (ss = _hurd_sigstates; ss != NULL; ss = ss->next)
-    if (ss->thread == thread)
-      break;
-  __mutex_unlock (&_hurd_siglock);
-  if (ss == NULL)
-    ss = _hurd_thread_sigstate (thread); /* Allocate a fresh one.  */
+  ss = _hurd_thread_sigstate (thread);
 
   if (__spin_lock_locked (&ss->lock))
     {
-- 
2.39.2

[PATCH 5/5] hurd: Avoid leaking task & thread ports

[Sergey Bugaev]

Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---
 hurd/catch-exc.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hurd/catch-exc.c b/hurd/catch-exc.c
index d375bf67..bec850f9 100644
--- a/hurd/catch-exc.c
+++ b/hurd/catch-exc.c
@@ -35,6 +35,7 @@ _S_catch_exception_raise (mach_port_t port,
 #endif
 			  )
 {
+  error_t err;
   struct hurd_sigstate *ss;
   int signo;
   struct hurd_signal_detail d;
@@ -83,6 +84,11 @@ _S_catch_exception_raise (mach_port_t port,
 			      MACH_PORT_NULL, MACH_MSG_TYPE_PORT_SEND,
 			      0);
 
+  err = __mach_port_deallocate (__mach_task_self (), task);
+  assert_perror (err);
+  err = __mach_port_deallocate (__mach_task_self (), thread);
+  assert_perror (err);
+
   return KERN_SUCCESS;
 }
 
-- 
2.39.2

Re: [PATCH 1/5] hurd: Fix restoring reply port in sigreturn

[Samuel Thibault]

Applied, thanks!

Sergey Bugaev, le ven. 14 avril 2023 22:36:56 +0300, a ecrit:
> We must not use the user's reply port (scp->sc_reply_port) for any of
> our own RPCs, otherwise various things break. So, use MACH_PORT_DEAD as
> a reply port when destroying our reply port, and make sure to do this
> after _hurd_sigstate_unlock (), which may do a gsync_wake () RPC.
> 
> Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
> ---
>  sysdeps/mach/hurd/i386/sigreturn.c | 35 ++++++++++++++++++++----------
>  1 file changed, 23 insertions(+), 12 deletions(-)
> 
> diff --git a/sysdeps/mach/hurd/i386/sigreturn.c b/sysdeps/mach/hurd/i386/sigreturn.c
> index 4f196710..a0fc8891 100644
> --- a/sysdeps/mach/hurd/i386/sigreturn.c
> +++ b/sysdeps/mach/hurd/i386/sigreturn.c
> @@ -26,11 +26,29 @@ register int *sp asm ("%esp");
>  /* This is run on the thread stack after restoring it, to be able to
>     unlock SS off sigstack.  */
>  static void
> -__sigreturn2 (int *usp)
> +__sigreturn2 (int *usp, struct sigcontext *scp)
>  {
> +  mach_port_t reply_port;
>    struct hurd_sigstate *ss = _hurd_self_sigstate ();
>    _hurd_sigstate_unlock (ss);
>  
> +  /* Destroy the MiG reply port used by the signal handler, and restore the
> +     reply port in use by the thread when interrupted.
> +
> +     We cannot use the original reply port for our RPCs that we do here, since
> +     we could unexpectedly receive/consume a reply message meant for the user
> +     (in particular, msg_sig_post_reply), and also since we would deallocate
> +     the port if *our* RPC fails, which we don't want to do since the user
> +     still has the old name.  And so, temporarily set MACH_PORT_DEAD as our
> +     reply name, and make sure destroying the port is the very last RPC we
> +     do.  */
> +  reply_port = THREAD_GETMEM (THREAD_SELF, reply_port);
> +  THREAD_SETMEM (THREAD_SELF, reply_port, MACH_PORT_DEAD);
> +  if (MACH_PORT_VALID (reply_port))
> +    (void) __mach_port_mod_refs (__mach_task_self (), reply_port,
> +                                 MACH_PORT_RIGHT_RECEIVE, -1);
> +  THREAD_SETMEM (THREAD_SELF, reply_port, scp->sc_reply_port);
> +
>    sp = usp;
>  #define A(line) asm volatile (#line)
>    /* The members in the sigcontext are arranged in this order
> @@ -58,7 +76,6 @@ __sigreturn (struct sigcontext *scp)
>  {
>    struct hurd_sigstate *ss;
>    struct hurd_userlink *link = (void *) &scp[1];
> -  mach_port_t reply_port;
>  
>    if (scp == NULL || (scp->sc_mask & _SIG_CANT_MASK))
>      {
> @@ -98,13 +115,6 @@ __sigreturn (struct sigcontext *scp)
>    if (scp->sc_onstack)
>      ss->sigaltstack.ss_flags &= ~SS_ONSTACK;
>  
> -  /* Destroy the MiG reply port used by the signal handler, and restore the
> -     reply port in use by the thread when interrupted.  */
> -  reply_port = THREAD_GETMEM (THREAD_SELF, reply_port);
> -  THREAD_SETMEM (THREAD_SELF, reply_port, scp->sc_reply_port);
> -  if (MACH_PORT_VALID (reply_port))
> -    __mach_port_destroy (__mach_task_self (), reply_port);
> -
>    if (scp->sc_fpused)
>      /* Restore the FPU state.  Mach conveniently stores the state
>         in the format the i387 `frstor' instruction uses to restore it.  */
> @@ -115,15 +125,16 @@ __sigreturn (struct sigcontext *scp)
>         copy the registers onto the user's stack, switch there, pop and
>         return.  */
>  
> -    int *usp = (int *) scp->sc_uesp;
> +    int usp_arg, *usp = (int *) scp->sc_uesp;
>  
>      *--usp = scp->sc_eip;
>      *--usp = scp->sc_efl;
>      memcpy (usp -= 12, &scp->sc_i386_thread_state, 12 * sizeof (int));
> +    usp_arg = (int) usp;
>  
> +    *--usp = (int) scp;
>      /* Pass usp to __sigreturn2 so it can unwind itself easily.  */
> -    *(usp-1) = (int) usp;
> -    --usp;
> +    *--usp = usp_arg;
>      /* Bogus return address for __sigreturn2 */
>      *--usp = 0;
>      *--usp = (int) __sigreturn2;
> -- 
> 2.39.2
> 

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

Re: [PATCH 2/5] hurd: Microoptimize sigreturn

[Samuel Thibault]

Hello,

Sergey Bugaev, le ven. 14 avril 2023 22:36:57 +0300, a ecrit:
> -  struct hurd_sigstate *ss = _hurd_self_sigstate ();
> +  struct hurd_sigstate *ss;
> +
> +  /* We know the sigstate must be initialized,

That doesn't seem to be true, we get segfaults with this patch applied
in these tests:

./htl/tst-kill6.test-result:FAIL: htl/tst-kill6
./htl/tst-pt-tls2.test-result:FAIL: htl/tst-pt-tls2

#0  0x0105a7c2 in sigstate_is_global_rcv (ss=0x0) at hurdsig.c:183
183              && (ss->actions[0].sa_handler == SIG_IGN);
(gdb) bt
#0  0x0105a7c2 in sigstate_is_global_rcv (ss=0x0) at hurdsig.c:183
#1  __GI__hurd_sigstate_lock (ss=0x0) at hurdsig.c:192
#2  0x0108bb17 in __sigreturn (scp=0x2104bec) at ../sysdeps/mach/hurd/i386/sigreturn.c:94
#3  0x010602f6 in trampoline () from ./libc.so.0.3

Samuel

Re: [PATCH 4/5] hurd: Simplify _S_catch_exception_raise

[Samuel Thibault]

Applied, thanks!

Sergey Bugaev, le ven. 14 avril 2023 22:36:59 +0300, a ecrit:
> _hurd_thread_sigstate () already handles finding an existing sigstate
> before allocating a new one, so just use that. Bonus: this will only
> lock the _hurd_siglock once.
> 
> Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
> ---
>  hurd/catch-exc.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/hurd/catch-exc.c b/hurd/catch-exc.c
> index 5ee2233a..d375bf67 100644
> --- a/hurd/catch-exc.c
> +++ b/hurd/catch-exc.c
> @@ -58,13 +58,7 @@ _S_catch_exception_raise (mach_port_t port,
>    _hurd_exception2signal (&d, &signo);
>  
>    /* Find the sigstate structure for the faulting thread.  */
> -  __mutex_lock (&_hurd_siglock);
> -  for (ss = _hurd_sigstates; ss != NULL; ss = ss->next)
> -    if (ss->thread == thread)
> -      break;
> -  __mutex_unlock (&_hurd_siglock);
> -  if (ss == NULL)
> -    ss = _hurd_thread_sigstate (thread); /* Allocate a fresh one.  */
> +  ss = _hurd_thread_sigstate (thread);
>  
>    if (__spin_lock_locked (&ss->lock))
>      {
> -- 
> 2.39.2
> 

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

Re: [PATCH 5/5] hurd: Avoid leaking task & thread ports

[Samuel Thibault]

Applied, thanks!

Sergey Bugaev, le ven. 14 avril 2023 22:37:00 +0300, a ecrit:
> Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
> ---
>  hurd/catch-exc.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/hurd/catch-exc.c b/hurd/catch-exc.c
> index d375bf67..bec850f9 100644
> --- a/hurd/catch-exc.c
> +++ b/hurd/catch-exc.c
> @@ -35,6 +35,7 @@ _S_catch_exception_raise (mach_port_t port,
>  #endif
>  			  )
>  {
> +  error_t err;
>    struct hurd_sigstate *ss;
>    int signo;
>    struct hurd_signal_detail d;
> @@ -83,6 +84,11 @@ _S_catch_exception_raise (mach_port_t port,
>  			      MACH_PORT_NULL, MACH_MSG_TYPE_PORT_SEND,
>  			      0);
>  
> +  err = __mach_port_deallocate (__mach_task_self (), task);
> +  assert_perror (err);
> +  err = __mach_port_deallocate (__mach_task_self (), thread);
> +  assert_perror (err);
> +
>    return KERN_SUCCESS;
>  }
>  
> -- 
> 2.39.2
> 

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

(no subject)

[Sergey Bugaev]

Re: [PATCH 2/5] hurd: Microoptimize sigreturn

[Sergey Bugaev]

(Git managed to eat my email -- it both sent an empty one *and*
truncated my local file (what?); so retyping and resending this.)

On Tue, Apr 18, 2023 at 2:23 AM Samuel Thibault <samuel.thibault@gnu.org> wrote:
> That doesn't seem to be true, we get segfaults with this patch applied
> in these tests:
>
> ./htl/tst-kill6.test-result:FAIL: htl/tst-kill6
> ./htl/tst-pt-tls2.test-result:FAIL: htl/tst-pt-tls2
>
> #0  0x0105a7c2 in sigstate_is_global_rcv (ss=0x0) at hurdsig.c:183
> 183              && (ss->actions[0].sa_handler == SIG_IGN);
> (gdb) bt
> #0  0x0105a7c2 in sigstate_is_global_rcv (ss=0x0) at hurdsig.c:183
> #1  __GI__hurd_sigstate_lock (ss=0x0) at hurdsig.c:192
> #2  0x0108bb17 in __sigreturn (scp=0x2104bec) at ../sysdeps/mach/hurd/i386/sigreturn.c:94
> #3  0x010602f6 in trampoline () from ./libc.so.0.3

Interesting; so while the sigstate for the thread must have been
initialized, it seems it has never been installed into the
tcb->_hurd_sigstate slot, because the thread has never called
_hurd_self_sigstate (). So we have to make the full call to
_hurd_self_sigstate () the first time, to find the sigstate and
install it into the slot.

Please try this:

Sergey

---- >8 ----


From 94fc9499390b4de9fbe10c3014d473ac42878c6a Mon Sep 17 00:00:00 2001
From: Sergey Bugaev <bugaevc@gmail.com>
Date: Fri, 14 Apr 2023 16:32:45 +0300
Subject: [PATCH] hurd: Microoptimize sigreturn

Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---
 sysdeps/mach/hurd/i386/sigreturn.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/sysdeps/mach/hurd/i386/sigreturn.c b/sysdeps/mach/hurd/i386/sigreturn.c
index a0fc8891..1686734d 100644
--- a/sysdeps/mach/hurd/i386/sigreturn.c
+++ b/sysdeps/mach/hurd/i386/sigreturn.c
@@ -29,7 +29,12 @@ static void
 __sigreturn2 (int *usp, struct sigcontext *scp)
 {
   mach_port_t reply_port;
-  struct hurd_sigstate *ss = _hurd_self_sigstate ();
+  struct hurd_sigstate *ss;
+
+  /* We know the sigstate must be initialized by the call below, but the
+     compiler does not.  Help it out a little bit by eliding the check that
+     _hurd_self_sigstate makes otherwise.  */
+  ss = THREAD_GETMEM (THREAD_SELF, _hurd_sigstate);
   _hurd_sigstate_unlock (ss);
 
   /* Destroy the MiG reply port used by the signal handler, and restore the
@@ -44,7 +49,7 @@ __sigreturn2 (int *usp, struct sigcontext *scp)
      do.  */
   reply_port = THREAD_GETMEM (THREAD_SELF, reply_port);
   THREAD_SETMEM (THREAD_SELF, reply_port, MACH_PORT_DEAD);
-  if (MACH_PORT_VALID (reply_port))
+  if (__glibc_likely (MACH_PORT_VALID (reply_port)))
     (void) __mach_port_mod_refs (__mach_task_self (), reply_port,
                                  MACH_PORT_RIGHT_RECEIVE, -1);
   THREAD_SETMEM (THREAD_SELF, reply_port, scp->sc_reply_port);
@@ -69,6 +74,7 @@ __sigreturn2 (int *usp, struct sigcontext *scp)
   /* Firewall.  */
   A (hlt);
 #undef A
+  __builtin_unreachable ();
 }
 
 int
@@ -77,7 +83,7 @@ __sigreturn (struct sigcontext *scp)
   struct hurd_sigstate *ss;
   struct hurd_userlink *link = (void *) &scp[1];
 
-  if (scp == NULL || (scp->sc_mask & _SIG_CANT_MASK))
+  if (__glibc_unlikely (scp == NULL || (scp->sc_mask & _SIG_CANT_MASK)))
     {
       errno = EINVAL;
       return -1;
-- 
2.39.2

Re: [PATCH 2/5] hurd: Microoptimize sigreturn

[Samuel Thibault]

Sergey Bugaev, le mar. 18 avril 2023 13:21:50 +0300, a ecrit:
> (Git managed to eat my email -- it both sent an empty one *and*
> truncated my local file (what?); so retyping and resending this.)
> 
> On Tue, Apr 18, 2023 at 2:23 AM Samuel Thibault <samuel.thibault@gnu.org> wrote:
> > That doesn't seem to be true, we get segfaults with this patch applied
> > in these tests:
> >
> > ./htl/tst-kill6.test-result:FAIL: htl/tst-kill6
> > ./htl/tst-pt-tls2.test-result:FAIL: htl/tst-pt-tls2
> >
> > #0  0x0105a7c2 in sigstate_is_global_rcv (ss=0x0) at hurdsig.c:183
> > 183              && (ss->actions[0].sa_handler == SIG_IGN);
> > (gdb) bt
> > #0  0x0105a7c2 in sigstate_is_global_rcv (ss=0x0) at hurdsig.c:183
> > #1  __GI__hurd_sigstate_lock (ss=0x0) at hurdsig.c:192
> > #2  0x0108bb17 in __sigreturn (scp=0x2104bec) at ../sysdeps/mach/hurd/i386/sigreturn.c:94
> > #3  0x010602f6 in trampoline () from ./libc.so.0.3
> 
> Interesting; so while the sigstate for the thread must have been
> initialized, it seems it has never been installed into the
> tcb->_hurd_sigstate slot, because the thread has never called
> _hurd_self_sigstate (). So we have to make the full call to
> _hurd_self_sigstate () the first time, to find the sigstate and
> install it into the slot.
> 
> Please try this:

That one works fine, applied, thanks!


> Sergey
> 
> ---- >8 ----
> 
> 
> From 94fc9499390b4de9fbe10c3014d473ac42878c6a Mon Sep 17 00:00:00 2001
> From: Sergey Bugaev <bugaevc@gmail.com>
> Date: Fri, 14 Apr 2023 16:32:45 +0300
> Subject: [PATCH] hurd: Microoptimize sigreturn
> 
> Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
> ---
>  sysdeps/mach/hurd/i386/sigreturn.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/sysdeps/mach/hurd/i386/sigreturn.c b/sysdeps/mach/hurd/i386/sigreturn.c
> index a0fc8891..1686734d 100644
> --- a/sysdeps/mach/hurd/i386/sigreturn.c
> +++ b/sysdeps/mach/hurd/i386/sigreturn.c
> @@ -29,7 +29,12 @@ static void
>  __sigreturn2 (int *usp, struct sigcontext *scp)
>  {
>    mach_port_t reply_port;
> -  struct hurd_sigstate *ss = _hurd_self_sigstate ();
> +  struct hurd_sigstate *ss;
> +
> +  /* We know the sigstate must be initialized by the call below, but the
> +     compiler does not.  Help it out a little bit by eliding the check that
> +     _hurd_self_sigstate makes otherwise.  */
> +  ss = THREAD_GETMEM (THREAD_SELF, _hurd_sigstate);
>    _hurd_sigstate_unlock (ss);
>  
>    /* Destroy the MiG reply port used by the signal handler, and restore the
> @@ -44,7 +49,7 @@ __sigreturn2 (int *usp, struct sigcontext *scp)
>       do.  */
>    reply_port = THREAD_GETMEM (THREAD_SELF, reply_port);
>    THREAD_SETMEM (THREAD_SELF, reply_port, MACH_PORT_DEAD);
> -  if (MACH_PORT_VALID (reply_port))
> +  if (__glibc_likely (MACH_PORT_VALID (reply_port)))
>      (void) __mach_port_mod_refs (__mach_task_self (), reply_port,
>                                   MACH_PORT_RIGHT_RECEIVE, -1);
>    THREAD_SETMEM (THREAD_SELF, reply_port, scp->sc_reply_port);
> @@ -69,6 +74,7 @@ __sigreturn2 (int *usp, struct sigcontext *scp)
>    /* Firewall.  */
>    A (hlt);
>  #undef A
> +  __builtin_unreachable ();
>  }
>  
>  int
> @@ -77,7 +83,7 @@ __sigreturn (struct sigcontext *scp)
>    struct hurd_sigstate *ss;
>    struct hurd_userlink *link = (void *) &scp[1];
>  
> -  if (scp == NULL || (scp->sc_mask & _SIG_CANT_MASK))
> +  if (__glibc_unlikely (scp == NULL || (scp->sc_mask & _SIG_CANT_MASK)))
>      {
>        errno = EINVAL;
>        return -1;
> -- 
> 2.39.2
> 

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