Re: [RFC PATCH glibc v2 26/34] hurd: Remove __hurd_local_reply_port

[Samuel Thibault]

Applied with fixing the __mig_dealloc_reply_port(NULL) cases, thanks!

Sergey Bugaev, le jeu. 13 avril 2023 14:58:12 +0300, a ecrit:
> 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>
> ---
> Changes since v1:
> * Only deallocate the port in __mig_dealloc_reply_port if it's the same
>   as the stored port, because of signals.
> * Add a comment detailing the reasoning.
> * Add two assertions: that ARG and PORT are exactly the same (unless PORT
>    has been invalidated), and that we succeed deallocating PORT.
> 
> NOTE: Completely untested! Do your own testing! You have been warned!
> 
>  hurd/hurd/threadvar.h         |  7 ----
>  sysdeps/mach/hurd/mig-reply.c | 70 +++++++++++++++++++++++++++++------
>  2 files changed, 58 insertions(+), 19 deletions(-)
> 
> 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..3fdee80e 100644
> --- a/sysdeps/mach/hurd/mig-reply.c
> +++ b/sysdeps/mach/hurd/mig-reply.c
> @@ -17,22 +17,46 @@
>  
>  #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)
> @@ -41,12 +65,34 @@ libc_hidden_def (__mig_get_reply_port)
>  void
>  __mig_dealloc_reply_port (mach_port_t arg)
>  {
> -  mach_port_t port = __hurd_local_reply_port;
> -  __hurd_local_reply_port = MACH_PORT_NULL;  /* So the mod_refs RPC won't 
> use it.  */
> +  error_t err;
> +  mach_port_t port = get_reply_port ();
> +
> +  set_reply_port (MACH_PORT_NULL);   /* So the mod_refs RPC won't use it.  */
> +
> +  /* Normally, ARG should be the same as PORT that we store.  However, if a
> +     signal has interrupted the RPC, the stored PORT has been deallocated and
> +     reset to MACH_PORT_NULL (or possibly MACH_PORT_DEAD).  In this case the
> +     MIG routine still has the old name, which it passes to us here.  We must
> +     not deallocate (or otherwise touch) it, since it may be already 
> allocated
> +     to another port right.  Fortunately MIG itself doesn't do anything with
> +     the reply port on errors either, other than immediately calling this
> +     function.
> +
> +     And so:
> +     1. Assert that things are sane, i.e. and PORT is either invalid or same
> +        as ARG.
> +     2. Only deallocate the name if our stored PORT still names it.  In that
> +        case we're sure the right has not been deallocated / the name reused.
> +    */
> +
> +  if (!MACH_PORT_VALID (port))
> +    return;
> +  assert (port == arg);
>  
> -  if (MACH_PORT_VALID (port))
> -    __mach_port_mod_refs (__mach_task_self (), port,
> -                       MACH_PORT_RIGHT_RECEIVE, -1);
> +  err = __mach_port_mod_refs (__mach_task_self (), port,
> +                              MACH_PORT_RIGHT_RECEIVE, -1);
> +  assert_perror (err);
>  }
>  weak_alias (__mig_dealloc_reply_port, mig_dealloc_reply_port)
>  libc_hidden_def (__mig_dealloc_reply_port)
> -- 
> 2.39.2
> 

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