diff mbox series

[RFC,26/34] hurd: Remove __hurd_local_reply_port

Message ID 20230319151017.531737-27-bugaevc@gmail.com
State New
Headers show
Series The rest of the x86_64-gnu port | expand

Commit Message

Sergey Bugaev March 19, 2023, 3:10 p.m. UTC
Now that the signal code no longer accesses it, the only real user of it
was mig-reply.c, so move the logic for managing the port there.

If we're in SHARED and outside of rtld, we know that __LIBC_NO_TLS ()
always evaluates to 0, and a TLS reply port will always be used, not
__hurd_reply_port0. Still, the compiler does not see that
__hurd_reply_port0 is never used due to its address being taken. To deal
with this, explicitly compile out __hurd_reply_port0 when we know we
won't use it.

Also, instead of accessing the port via THREAD_SELF->reply_port, this
uses THREAD_GETMEM and THREAD_SETMEM directly, avoiding possible
miscompilations.

Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---
 hurd/hurd/threadvar.h         |  7 ------
 sysdeps/mach/hurd/mig-reply.c | 43 +++++++++++++++++++++++++++--------
 2 files changed, 33 insertions(+), 17 deletions(-)

Comments

Samuel Thibault April 10, 2023, 10:07 p.m. UTC | #1
Sergey Bugaev, le dim. 19 mars 2023 18:10:09 +0300, a ecrit:
>  /* Called by MiG to deallocate the reply port.  */
>  void
> -__mig_dealloc_reply_port (mach_port_t arg)
> +__mig_dealloc_reply_port (mach_port_t port)
>  {
> -  mach_port_t port = __hurd_local_reply_port;
> -  __hurd_local_reply_port = MACH_PORT_NULL;	/* So the mod_refs RPC won't use it.  */
> +  set_reply_port (MACH_PORT_NULL);	/* So the mod_refs RPC won't use it.  */
>  
>    if (MACH_PORT_VALID (port))
>      __mach_port_mod_refs (__mach_task_self (), port,

I believe we still want to use

 mach_port_t port = get_reply_port();

because the caller might not know whether its port is still valid
or not, e.g. when a signal interrupted the RPC and thus we had to
deallocate the reply port to make sure the server doesn't get confused.
In that case the caller will still have the old reply port name, which
we don't want to reallocate since it might already have been reused for
something else.

Samuel
Samuel Thibault April 10, 2023, 10:35 p.m. UTC | #2
Samuel Thibault, le mar. 11 avril 2023 00:07:43 +0200, a ecrit:
> Sergey Bugaev, le dim. 19 mars 2023 18:10:09 +0300, a ecrit:
> >  /* Called by MiG to deallocate the reply port.  */
> >  void
> > -__mig_dealloc_reply_port (mach_port_t arg)
> > +__mig_dealloc_reply_port (mach_port_t port)
> >  {
> > -  mach_port_t port = __hurd_local_reply_port;
> > -  __hurd_local_reply_port = MACH_PORT_NULL;	/* So the mod_refs RPC won't use it.  */
> > +  set_reply_port (MACH_PORT_NULL);	/* So the mod_refs RPC won't use it.  */
> >  
> >    if (MACH_PORT_VALID (port))
> >      __mach_port_mod_refs (__mach_task_self (), port,
> 
> I believe we still want to use
> 
>  mach_port_t port = get_reply_port();
> 
> because the caller might not know whether its port is still valid
> or not, e.g. when a signal interrupted the RPC and thus we had to
> deallocate the reply port to make sure the server doesn't get confused.
> In that case the caller will still have the old reply port name, which
> we don't want to reallocate since it might already have been reused for

                   deallocate

> something else.
Sergey Bugaev April 11, 2023, 8 a.m. UTC | #3
On Tue, Apr 11, 2023 at 1:07 AM Samuel Thibault <samuel.thibault@gnu.org> wrote:
> I believe we still want to use
>
>  mach_port_t port = get_reply_port();
>
> because the caller might not know whether its port is still valid
> or not, e.g. when a signal interrupted the RPC and thus we had to
> deallocate the reply port to make sure the server doesn't get confused.
> In that case the caller will still have the old reply port name, which
> we don't want to reallocate since it might already have been reused for
> something else.

*Great* point!

(And I should have thought of that..., but hey, this is what code reviews
are for, right?)

Side note, I really really dislike this idea of some code still referencing
port names that are no longer valid / deallocated / reused by someone else.
This is really prone to use-after-frees. Typically we'd solve this by
leaving a dead-name right in place of the port, and having
mig_dealloc_reply_port () dealloc this dead name.

But in this case... we're fairly sure that the code really doesn't do
anything with the name that it has, other than immediately calling
mig_dealloc_reply_port () on it; and there'd have to be a separate code path
for deallocating the dead name since mach_port_mod_refs (recv, -1) won't do
it (mach_port_destroy would handle both, but using that is a terrible idea).

So in order not to overcomplicate this, in this particular case, it should
be fine to just deallocate the stored reply port and not what the user has,
as you're saying. But it definitely needs a comment explaining this. And
maybe an assert (port == arg || port == MACH_PORT_NULL).

Sergey
Samuel Thibault April 11, 2023, 8:18 p.m. UTC | #4
Sergey Bugaev, le mar. 11 avril 2023 11:00:27 +0300, a ecrit:
> Side note, I really really dislike this idea of some code still referencing
> port names that are no longer valid / deallocated / reused by someone else.
> This is really prone to use-after-frees. Typically we'd solve this by
> leaving a dead-name right in place of the port, and having
> mig_dealloc_reply_port () dealloc this dead name.

That could be better indeed. Rather than modifying refs under the hood,
let the code manage them.

> But in this case... we're fairly sure that the code really doesn't do
> anything with the name that it has, other than immediately calling
> mig_dealloc_reply_port () on it; and there'd have to be a separate code path
> for deallocating the dead name since mach_port_mod_refs (recv, -1) won't do
> it (mach_port_destroy would handle both, but using that is a terrible idea).
> 
> So in order not to overcomplicate this, in this particular case, it should
> be fine to just deallocate the stored reply port and not what the user has,
> as you're saying. But it definitely needs a comment explaining this.

Completely agree :)

> And maybe an assert (port == arg || port == MACH_PORT_NULL).

If that does indeed work, yes :)

Samuel
diff mbox series

Patch

diff --git a/hurd/hurd/threadvar.h b/hurd/hurd/threadvar.h
index f5c6a278..c476d988 100644
--- a/hurd/hurd/threadvar.h
+++ b/hurd/hurd/threadvar.h
@@ -29,11 +29,4 @@ 
 extern unsigned long int __hurd_sigthread_stack_base;
 extern unsigned long int __hurd_sigthread_stack_end;
 
-/* Store the MiG reply port reply port until we enable TLS.  */
-extern mach_port_t __hurd_reply_port0;
-
-/* This returns either the TLS reply port variable, or a single-thread variable
-   when TLS is not initialized yet.  */
-#define __hurd_local_reply_port (*(__LIBC_NO_TLS () ? &__hurd_reply_port0 : &THREAD_SELF->reply_port))
-
 #endif	/* hurd/threadvar.h */
diff --git a/sysdeps/mach/hurd/mig-reply.c b/sysdeps/mach/hurd/mig-reply.c
index 33ff260e..d3c5a958 100644
--- a/sysdeps/mach/hurd/mig-reply.c
+++ b/sysdeps/mach/hurd/mig-reply.c
@@ -17,32 +17,55 @@ 
 
 #include <mach.h>
 #include <mach/mig_support.h>
-#include <hurd/threadvar.h>
+#include <tls.h>
 
 /* These functions are called by MiG-generated code.  */
 
+#if !defined (SHARED) || IS_IN (rtld)
 mach_port_t __hurd_reply_port0;
+#endif
+
+static mach_port_t
+get_reply_port (void)
+{
+#if !defined (SHARED) || IS_IN (rtld)
+  if (__LIBC_NO_TLS ())
+    return __hurd_reply_port0;
+#endif
+  return THREAD_GETMEM (THREAD_SELF, reply_port);
+}
+
+static void
+set_reply_port (mach_port_t port)
+{
+#if !defined (SHARED) || IS_IN (rtld)
+  if (__LIBC_NO_TLS ())
+    __hurd_reply_port0 = port;
+  else
+#endif
+    THREAD_SETMEM (THREAD_SELF, reply_port, port);
+}
 
 /* Called by MiG to get a reply port.  */
 mach_port_t
 __mig_get_reply_port (void)
 {
-  if (__hurd_local_reply_port == MACH_PORT_NULL
-      || (&__hurd_local_reply_port != &__hurd_reply_port0
-	  && __hurd_local_reply_port == __hurd_reply_port0))
-    __hurd_local_reply_port = __mach_reply_port ();
-
-  return __hurd_local_reply_port;
+  mach_port_t port = get_reply_port ();
+  if (__glibc_unlikely (port == MACH_PORT_NULL))
+    {
+      port = __mach_reply_port ();
+      set_reply_port (port);
+    }
+  return port;
 }
 weak_alias (__mig_get_reply_port, mig_get_reply_port)
 libc_hidden_def (__mig_get_reply_port)
 
 /* Called by MiG to deallocate the reply port.  */
 void
-__mig_dealloc_reply_port (mach_port_t arg)
+__mig_dealloc_reply_port (mach_port_t port)
 {
-  mach_port_t port = __hurd_local_reply_port;
-  __hurd_local_reply_port = MACH_PORT_NULL;	/* So the mod_refs RPC won't use it.  */
+  set_reply_port (MACH_PORT_NULL);	/* So the mod_refs RPC won't use it.  */
 
   if (MACH_PORT_VALID (port))
     __mach_port_mod_refs (__mach_task_self (), port,