* [PATCH 1/5] hurd: Fix restoring reply port in sigreturn
@ 2023-04-14 19:36 Sergey Bugaev
2023-04-14 19:36 ` [PATCH 2/5] hurd: Microoptimize sigreturn Sergey Bugaev
` (4 more replies)
0 siblings, 5 replies; 12+ messages in thread
From: Sergey Bugaev @ 2023-04-14 19:36 UTC (permalink / raw)
To: libc-alpha, bug-hurd; +Cc: Samuel Thibault, 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/5] hurd: Microoptimize sigreturn
2023-04-14 19:36 [PATCH 1/5] hurd: Fix restoring reply port in sigreturn Sergey Bugaev
@ 2023-04-14 19:36 ` Sergey Bugaev
2023-04-17 23:23 ` Samuel Thibault
2023-04-14 19:36 ` [PATCH 3/5] hurd: Implement sigreturn for x86_64 Sergey Bugaev
` (3 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Sergey Bugaev @ 2023-04-14 19:36 UTC (permalink / raw)
To: libc-alpha, bug-hurd; +Cc: Samuel Thibault, 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/5] hurd: Implement sigreturn for x86_64
2023-04-14 19:36 [PATCH 1/5] hurd: Fix restoring reply port in sigreturn Sergey Bugaev
2023-04-14 19:36 ` [PATCH 2/5] hurd: Microoptimize sigreturn Sergey Bugaev
@ 2023-04-14 19:36 ` Sergey Bugaev
2023-04-14 19:36 ` [PATCH 4/5] hurd: Simplify _S_catch_exception_raise Sergey Bugaev
` (2 subsequent siblings)
4 siblings, 0 replies; 12+ messages in thread
From: Sergey Bugaev @ 2023-04-14 19:36 UTC (permalink / raw)
To: libc-alpha, bug-hurd; +Cc: Samuel Thibault, 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 4/5] hurd: Simplify _S_catch_exception_raise
2023-04-14 19:36 [PATCH 1/5] hurd: Fix restoring reply port in sigreturn Sergey Bugaev
2023-04-14 19:36 ` [PATCH 2/5] hurd: Microoptimize sigreturn Sergey Bugaev
2023-04-14 19:36 ` [PATCH 3/5] hurd: Implement sigreturn for x86_64 Sergey Bugaev
@ 2023-04-14 19:36 ` Sergey Bugaev
2023-04-17 23:24 ` Samuel Thibault
2023-04-14 19:37 ` [PATCH 5/5] hurd: Avoid leaking task & thread ports Sergey Bugaev
2023-04-17 19:00 ` [PATCH 1/5] hurd: Fix restoring reply port in sigreturn Samuel Thibault
4 siblings, 1 reply; 12+ messages in thread
From: Sergey Bugaev @ 2023-04-14 19:36 UTC (permalink / raw)
To: libc-alpha, bug-hurd; +Cc: Samuel Thibault, 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 5/5] hurd: Avoid leaking task & thread ports
2023-04-14 19:36 [PATCH 1/5] hurd: Fix restoring reply port in sigreturn Sergey Bugaev
` (2 preceding siblings ...)
2023-04-14 19:36 ` [PATCH 4/5] hurd: Simplify _S_catch_exception_raise Sergey Bugaev
@ 2023-04-14 19:37 ` Sergey Bugaev
2023-04-17 23:24 ` Samuel Thibault
2023-04-17 19:00 ` [PATCH 1/5] hurd: Fix restoring reply port in sigreturn Samuel Thibault
4 siblings, 1 reply; 12+ messages in thread
From: Sergey Bugaev @ 2023-04-14 19:37 UTC (permalink / raw)
To: libc-alpha, bug-hurd; +Cc: Samuel Thibault, 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/5] hurd: Fix restoring reply port in sigreturn
2023-04-14 19:36 [PATCH 1/5] hurd: Fix restoring reply port in sigreturn Sergey Bugaev
` (3 preceding siblings ...)
2023-04-14 19:37 ` [PATCH 5/5] hurd: Avoid leaking task & thread ports Sergey Bugaev
@ 2023-04-17 19:00 ` Samuel Thibault
4 siblings, 0 replies; 12+ messages in thread
From: Samuel Thibault @ 2023-04-17 19:00 UTC (permalink / raw)
To: Sergey Bugaev; +Cc: libc-alpha, bug-hurd
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/5] hurd: Microoptimize sigreturn
2023-04-14 19:36 ` [PATCH 2/5] hurd: Microoptimize sigreturn Sergey Bugaev
@ 2023-04-17 23:23 ` Samuel Thibault
2023-04-18 9:34 ` Sergey Bugaev
2023-04-18 10:21 ` [PATCH 2/5] hurd: Microoptimize sigreturn Sergey Bugaev
0 siblings, 2 replies; 12+ messages in thread
From: Samuel Thibault @ 2023-04-17 23:23 UTC (permalink / raw)
To: Sergey Bugaev; +Cc: libc-alpha, bug-hurd
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 4/5] hurd: Simplify _S_catch_exception_raise
2023-04-14 19:36 ` [PATCH 4/5] hurd: Simplify _S_catch_exception_raise Sergey Bugaev
@ 2023-04-17 23:24 ` Samuel Thibault
0 siblings, 0 replies; 12+ messages in thread
From: Samuel Thibault @ 2023-04-17 23:24 UTC (permalink / raw)
To: Sergey Bugaev; +Cc: libc-alpha, bug-hurd
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 5/5] hurd: Avoid leaking task & thread ports
2023-04-14 19:37 ` [PATCH 5/5] hurd: Avoid leaking task & thread ports Sergey Bugaev
@ 2023-04-17 23:24 ` Samuel Thibault
0 siblings, 0 replies; 12+ messages in thread
From: Samuel Thibault @ 2023-04-17 23:24 UTC (permalink / raw)
To: Sergey Bugaev; +Cc: libc-alpha, bug-hurd
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* (no subject)
2023-04-17 23:23 ` Samuel Thibault
@ 2023-04-18 9:34 ` Sergey Bugaev
2023-04-18 10:21 ` [PATCH 2/5] hurd: Microoptimize sigreturn Sergey Bugaev
1 sibling, 0 replies; 12+ messages in thread
From: Sergey Bugaev @ 2023-04-18 9:34 UTC (permalink / raw)
To: Samuel Thibault; +Cc: libc-alpha, bug-hurd
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/5] hurd: Microoptimize sigreturn
2023-04-17 23:23 ` Samuel Thibault
2023-04-18 9:34 ` Sergey Bugaev
@ 2023-04-18 10:21 ` Sergey Bugaev
2023-04-18 14:21 ` Samuel Thibault
1 sibling, 1 reply; 12+ messages in thread
From: Sergey Bugaev @ 2023-04-18 10:21 UTC (permalink / raw)
To: Samuel Thibault; +Cc: libc-alpha, bug-hurd, 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/5] hurd: Microoptimize sigreturn
2023-04-18 10:21 ` [PATCH 2/5] hurd: Microoptimize sigreturn Sergey Bugaev
@ 2023-04-18 14:21 ` Samuel Thibault
0 siblings, 0 replies; 12+ messages in thread
From: Samuel Thibault @ 2023-04-18 14:21 UTC (permalink / raw)
To: Sergey Bugaev; +Cc: libc-alpha, bug-hurd
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2023-04-18 14:21 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-04-14 19:36 [PATCH 1/5] hurd: Fix restoring reply port in sigreturn Sergey Bugaev
2023-04-14 19:36 ` [PATCH 2/5] hurd: Microoptimize sigreturn Sergey Bugaev
2023-04-17 23:23 ` Samuel Thibault
2023-04-18 9:34 ` Sergey Bugaev
2023-04-18 10:21 ` [PATCH 2/5] hurd: Microoptimize sigreturn Sergey Bugaev
2023-04-18 14:21 ` Samuel Thibault
2023-04-14 19:36 ` [PATCH 3/5] hurd: Implement sigreturn for x86_64 Sergey Bugaev
2023-04-14 19:36 ` [PATCH 4/5] hurd: Simplify _S_catch_exception_raise Sergey Bugaev
2023-04-17 23:24 ` Samuel Thibault
2023-04-14 19:37 ` [PATCH 5/5] hurd: Avoid leaking task & thread ports Sergey Bugaev
2023-04-17 23:24 ` Samuel Thibault
2023-04-17 19:00 ` [PATCH 1/5] hurd: Fix restoring reply port in sigreturn Samuel Thibault
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).