[PATCH 0/1] Let's improve/fix ccty handling

[PATCH 0/1] Let's improve/fix ccty handling

[Sergey Bugaev]

Hello!

Here are some assorted thoughts on cttys (controlling terminals).

Running rpctrace on any simple program, I see this:

<snip>
  13<--33(pid1354)->term_getctty () = 0    4<--39(pid1354)
task16(pid1354)->mach_port_deallocate (pn{ 10}) = 0
  13<--33(pid1354)->term_open_ctty (0 0) = 0x40000016 (Invalid argument)
  13<--33(pid1354)->term_getctty () = 0    4<--39(pid1354)
task16(pid1354)->mach_port_deallocate (pn{ 10}) = 0
  13<--33(pid1354)->term_open_ctty (0 0) = 0x40000016 (Invalid argument)
  13<--33(pid1354)->term_getctty () = 0    4<--39(pid1354)
task16(pid1354)->mach_port_deallocate (pn{ 10}) = 0
  13<--33(pid1354)->term_open_ctty (0 0) = 0x40000016 (Invalid argument)
<snip>

This happens inside init_dtable () -> _hurd_port2fd (). We can notice
that:

1. stdin, stdout, stderr all refer to the same port (/dev/ttyp1 in my
   case), and yet glibc tries to set up ctty handling for each fd
   separately, making way too many RPCs. This happens each time a
   process starts up => not cool!
2. ctty setup actually fails with EINVAL! -- even though this is a
   valid tty.

I'm attaching a patch to fix #1, so now we get this:

<snip>
  13<--33(pid1255)->term_getctty () = 0    4<--39(pid1255)
task16(pid1255)->mach_port_deallocate (pn{ 10}) = 0
  13<--33(pid1255)->term_open_ctty (0 0) = 0x40000016 (Invalid argument)
<snip>

Much better, but still, what's that error? Well, this is what
S_term_open_ctty () checks:

  if (pid <= 0 || pgrp <= 0)
    {
      return EINVAL;
    }

so it makes sense that it errors out if we pass pid = 0, pgrp = 0. But
why are we passing that? Looking at the code, it does

__term_open_ctty (dport, _hurd_pid, _hurd_pgrp, &ctty);

so clearly at this point our _hurd_pid / _hurd_pgrp are not initialized
yet, and indeed the proc_getpids_request / proc_getpgrp_request RPCs
happen much later in the trace.

I think commit 1ccbb9258eed0f667edf459a28ba23a805549b36
"hurd: Notify the proc server later during initialization" was the one
that has broken things here (I guess ctty working is not a part of the
testsuite? :). The commit makes sense, in that we want to start doing
signals late; but apparently we need to fetch our pid/pgrp early. This
itself would be fine, but I'm not sure how to implement it. Fetching
pid/pgrp is done in init_pids (in hurdpid.c), which is attached to the
_hurd_proc_subinit hook (and is the only thing attached there).

Do I understand it right that this hooks system is libc-internal? --
or can user programs add their own functions to the hooks? (Looks like
it works through the linker-defined start/end section symbols, which
wouldn't work across DSOs, which is why I think it must be
libc-internal.) If it is internal to libc, we're not bounded by
compatibility concerns, and have more liberty in shuffling things
around.

_hurd_proc_subinit is defined as "Hook for things which should be
initialized as soon as the proc server is available." If that is
accurate, surely we don't need to have started signals and told the
proc server our argv location for it to be "available"? Can we just
run this hook sooner, namely in _hurd_init, once we set
_hurd_portarray but before we do RUN_RELHOOK (_hurd_subinit, ())?
_hurd_subinit is the hook init_dtable is attached to (and again,
there's nothing else on it).

Note: it's important to avoid calling init_pids multiple times during
startup, because each call does two RPCs, and we don't want any extra
ones!


Now for another thing: glibc also makes these term_getctty RPCs when
they are clearly pointless, such as when loading the locale archive:

11<--36(pid1403)->dir_lookup ("usr/lib/locale/locale-archive" 4194305 0) = 0 1 ""    48<--43(pid1403)
48<--43(pid1403)->term_getctty () = 0xfffffed1 ((ipc/mig) bad request message ID)

We should be probably using O_IGNORE_CTTY (which makes it not do that)
in more places where we expect to open a regular file, not a terminal.
But how do we do that, considering O_IGNORE_CTTY is Hurd-specific? We
could, for instance, do

#ifndef O_IGNORE_CTTY
#define O_IGNORE_CTTY 0
#endif

in each .c file where we'd like to use it. Or maybe there's some
internal version of fcntl.h for the Linux port where we could just

#define O_IGNORE_CTTY 0

without exposing it to user code?

Sergey

[PATCH 1/1] hurd: Avoid extra ctty RPCs in init_dtable ()

[Sergey Bugaev]

It is common to have (some of) stdin, stdout and stderr point to the
very same port. We were making the ctty RPCs that _hurd_port2fd () does
for each one of them separately:

1. term_getctty ()
2. mach_port_deallocate ()
3. term_open_ctty ()

Instead, let's detect this case and duplicate the ctty port we already
have. This means we do 1 RPC instead of 3 (and create a single protid
on the server side) if the file is our ctty, and no RPCs instead of 1
if it's not. A clear win!

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

diff --git a/hurd/dtable.c b/hurd/dtable.c
index 5dd04131..41f4d7af 100644
--- a/hurd/dtable.c
+++ b/hurd/dtable.c
@@ -60,18 +60,50 @@ init_dtable (void)
 	_hurd_dtable[i] = NULL;
       else
 	{
+	  int copy;
 	  /* Allocate a new file descriptor structure.  */
 	  struct hurd_fd *new = malloc (sizeof (struct hurd_fd));
 	  if (new == NULL)
 	    __libc_fatal ("hurd: Can't allocate initial file descriptors\n");
 
-	  /* Initialize the port cells.  */
-	  _hurd_port_init (&new->port, MACH_PORT_NULL);
-	  _hurd_port_init (&new->ctty, MACH_PORT_NULL);
-
-	  /* Install the port in the descriptor.
-	     This sets up all the ctty magic.  */
-	  _hurd_port2fd (new, _hurd_init_dtable[i], 0);
+	  /* See if this file descriptor is the same as a previous one we have
+	     already installed.  In this case, we can just copy over the same
+	     ctty port without making any more RPCs.  We only check the the
+	     immediately preceding fd and fd 0 -- this should be enough to
+	     handle the common cases while not requiring quadratic
+	     complexity.  */
+	  if (i > 0 && _hurd_init_dtable[i] == _hurd_init_dtable[i - 1])
+	    copy = i - 1;
+	  else if (i > 0 && _hurd_init_dtable[i] == _hurd_init_dtable[0])
+	    copy = 0;
+	  else
+	    copy = -1;
+
+	  if (copy < 0)
+	    {
+	      /* Initialize the port cells.  */
+	      _hurd_port_init (&new->port, MACH_PORT_NULL);
+	      _hurd_port_init (&new->ctty, MACH_PORT_NULL);
+
+	      /* Install the port in the descriptor.
+	         This sets up all the ctty magic.  */
+	      _hurd_port2fd (new, _hurd_init_dtable[i], 0);
+	    }
+	  else
+	    {
+	      /* Copy over ctty from the already set up file descriptor that
+	         contains the same port.  We can access the contents of the
+	         cell without any locking since no one could have seen it
+	         yet.  */
+	      mach_port_t ctty = _hurd_dtable[copy]->ctty.port;
+
+	      if (MACH_PORT_VALID (ctty))
+	        __mach_port_mod_refs (__mach_task_self (), ctty,
+	                              MACH_PORT_RIGHT_SEND, +1);
+
+	      _hurd_port_init (&new->port, _hurd_init_dtable[i]);
+	      _hurd_port_init (&new->ctty, ctty);
+	    }
 
 	  _hurd_dtable[i] = new;
 	}
-- 
2.39.2

Re: [PATCH 0/1] Let's improve/fix ccty handling

[Samuel Thibault]

Sergey Bugaev, le sam. 15 avril 2023 19:17:17 +0300, a ecrit:
> Do I understand it right that this hooks system is libc-internal? --
> or can user programs add their own functions to the hooks?

I don't think it was meant to be used by user programs.

Samuel

[PATCH] hurd: Run init_pids () before init_dtable ()

[Sergey Bugaev]

Much as the comment says, things on _hurd_subinit assume that _hurd_pid
is already initialized by the time _hurd_subinit is run, so
_hurd_proc_subinit has to run before it. Specifically, init_dtable ()
calls _hurd_port2fd (), which uses _hurd_pid and _hurd_pgrp to set up
ctty handling. With _hurd_subinit running before _hurd_proc_subinit,
ctty setup was broken:

  13<--33(pid1255)->term_getctty () = 0    4<--39(pid1255)
task16(pid1255)->mach_port_deallocate (pn{ 10}) = 0
  13<--33(pid1255)->term_open_ctty (0 0) = 0x40000016 (Invalid argument)

Fix this by running the _hurd_proc_subinit hook in the correct place --
just after _hurd_portarray is set up (so the proc server port is
available in its usual place) and just before running _hurd_subinit.

Fixes 1ccbb9258eed0f667edf459a28ba23a805549b36
("hurd: Notify the proc server later during initialization").

Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---
 hurd/hurdinit.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/hurd/hurdinit.c b/hurd/hurdinit.c
index f88c5666..d0c7a831 100644
--- a/hurd/hurdinit.c
+++ b/hurd/hurdinit.c
@@ -54,6 +54,10 @@ _hurd_ports_use (int which, error_t (*operate) (mach_port_t))
 
 DEFINE_HOOK (_hurd_subinit, (void));
 
+/* Hook for things which should be initialized as soon as the proc
+   server is available.  */
+DEFINE_HOOK (_hurd_proc_subinit, (void));
+
 __typeof (_hurd_proc_init) _hurd_new_proc_init;	/* below */
 
 /* Initialize the library data structures from the
@@ -105,6 +109,11 @@ _hurd_init (int flags, char **argv,
       */
     }
 
+  /* Call other things which want to do some initialization.  These are not
+     on the _hurd_subinit hook because things there assume that things done
+     here, like _hurd_pid, are already initialized.  */
+  RUN_RELHOOK (_hurd_proc_subinit, ());
+
   /* Call other things which want to do some initialization.  These are not
      on the __libc_subinit hook because things there like to be able to
      assume the availability of the POSIX.1 services we provide.  */
@@ -148,10 +157,6 @@ libc_hidden_def (_hurd_libc_proc_init)
    sure the arguments are never visible with `ps'.  */
 int _hide_arguments, _hide_environment;
 
-/* Hook for things which should be initialized as soon as the proc
-   server is available.  */
-DEFINE_HOOK (_hurd_proc_subinit, (void));
-
 /* Do startup handshaking with the proc server just installed in _hurd_ports.
    Call _hurdsig_init to set up signal processing.  */
 
@@ -187,11 +192,6 @@ _hurd_new_proc_init (char **argv,
   /* Initialize proc server-assisted fault recovery for the signal thread.  */
   _hurdsig_fault_init ();
 
-  /* Call other things which want to do some initialization.  These are not
-     on the _hurd_subinit hook because things there assume that things done
-     here, like _hurd_pid, are already initialized.  */
-  RUN_RELHOOK (_hurd_proc_subinit, ());
-
   /* XXX This code should probably be removed entirely at some point.  This
      conditional should make it reasonably usable with old gdb's for a
      while.  Eventually it probably makes most sense for the exec server to
-- 
2.39.2

Re: [PATCH 1/1] hurd: Avoid extra ctty RPCs in init_dtable ()

[Samuel Thibault]

Applied, thanks!

Sergey Bugaev, le sam. 15 avril 2023 19:17:18 +0300, a ecrit:
> It is common to have (some of) stdin, stdout and stderr point to the
> very same port. We were making the ctty RPCs that _hurd_port2fd () does
> for each one of them separately:
> 
> 1. term_getctty ()
> 2. mach_port_deallocate ()
> 3. term_open_ctty ()
> 
> Instead, let's detect this case and duplicate the ctty port we already
> have. This means we do 1 RPC instead of 3 (and create a single protid
> on the server side) if the file is our ctty, and no RPCs instead of 1
> if it's not. A clear win!
> 
> Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
> ---
>  hurd/dtable.c | 46 +++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 39 insertions(+), 7 deletions(-)
> 
> diff --git a/hurd/dtable.c b/hurd/dtable.c
> index 5dd04131..41f4d7af 100644
> --- a/hurd/dtable.c
> +++ b/hurd/dtable.c
> @@ -60,18 +60,50 @@ init_dtable (void)
>  	_hurd_dtable[i] = NULL;
>        else
>  	{
> +	  int copy;
>  	  /* Allocate a new file descriptor structure.  */
>  	  struct hurd_fd *new = malloc (sizeof (struct hurd_fd));
>  	  if (new == NULL)
>  	    __libc_fatal ("hurd: Can't allocate initial file descriptors\n");
>  
> -	  /* Initialize the port cells.  */
> -	  _hurd_port_init (&new->port, MACH_PORT_NULL);
> -	  _hurd_port_init (&new->ctty, MACH_PORT_NULL);
> -
> -	  /* Install the port in the descriptor.
> -	     This sets up all the ctty magic.  */
> -	  _hurd_port2fd (new, _hurd_init_dtable[i], 0);
> +	  /* See if this file descriptor is the same as a previous one we have
> +	     already installed.  In this case, we can just copy over the same
> +	     ctty port without making any more RPCs.  We only check the the
> +	     immediately preceding fd and fd 0 -- this should be enough to
> +	     handle the common cases while not requiring quadratic
> +	     complexity.  */
> +	  if (i > 0 && _hurd_init_dtable[i] == _hurd_init_dtable[i - 1])
> +	    copy = i - 1;
> +	  else if (i > 0 && _hurd_init_dtable[i] == _hurd_init_dtable[0])
> +	    copy = 0;
> +	  else
> +	    copy = -1;
> +
> +	  if (copy < 0)
> +	    {
> +	      /* Initialize the port cells.  */
> +	      _hurd_port_init (&new->port, MACH_PORT_NULL);
> +	      _hurd_port_init (&new->ctty, MACH_PORT_NULL);
> +
> +	      /* Install the port in the descriptor.
> +	         This sets up all the ctty magic.  */
> +	      _hurd_port2fd (new, _hurd_init_dtable[i], 0);
> +	    }
> +	  else
> +	    {
> +	      /* Copy over ctty from the already set up file descriptor that
> +	         contains the same port.  We can access the contents of the
> +	         cell without any locking since no one could have seen it
> +	         yet.  */
> +	      mach_port_t ctty = _hurd_dtable[copy]->ctty.port;
> +
> +	      if (MACH_PORT_VALID (ctty))
> +	        __mach_port_mod_refs (__mach_task_self (), ctty,
> +	                              MACH_PORT_RIGHT_SEND, +1);
> +
> +	      _hurd_port_init (&new->port, _hurd_init_dtable[i]);
> +	      _hurd_port_init (&new->ctty, ctty);
> +	    }
>  
>  	  _hurd_dtable[i] = new;
>  	}
> -- 
> 2.39.2
> 

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

Re: [PATCH 0/1] Let's improve/fix ccty handling

[Samuel Thibault]

Sergey Bugaev, le sam. 15 avril 2023 19:17:17 +0300, a ecrit:
> We should be probably using O_IGNORE_CTTY (which makes it not do that)
> in more places where we expect to open a regular file, not a terminal.
> But how do we do that, considering O_IGNORE_CTTY is Hurd-specific? We
> could, for instance, do
> 
> #ifndef O_IGNORE_CTTY
> #define O_IGNORE_CTTY 0
> #endif
> 
> in each .c file where we'd like to use it. Or maybe there's some
> internal version of fcntl.h for the Linux port where we could just
> 
> #define O_IGNORE_CTTY 0
> 
> without exposing it to user code?

You can add it to include/fcntl.h

Samuel

Re: [PATCH] hurd: Run init_pids () before init_dtable ()

[Samuel Thibault]

Applied, thanks!

Sergey Bugaev, le sam. 15 avril 2023 22:08:56 +0300, a ecrit:
> Much as the comment says, things on _hurd_subinit assume that _hurd_pid
> is already initialized by the time _hurd_subinit is run, so
> _hurd_proc_subinit has to run before it. Specifically, init_dtable ()
> calls _hurd_port2fd (), which uses _hurd_pid and _hurd_pgrp to set up
> ctty handling. With _hurd_subinit running before _hurd_proc_subinit,
> ctty setup was broken:
> 
>   13<--33(pid1255)->term_getctty () = 0    4<--39(pid1255)
> task16(pid1255)->mach_port_deallocate (pn{ 10}) = 0
>   13<--33(pid1255)->term_open_ctty (0 0) = 0x40000016 (Invalid argument)
> 
> Fix this by running the _hurd_proc_subinit hook in the correct place --
> just after _hurd_portarray is set up (so the proc server port is
> available in its usual place) and just before running _hurd_subinit.
> 
> Fixes 1ccbb9258eed0f667edf459a28ba23a805549b36
> ("hurd: Notify the proc server later during initialization").
> 
> Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
> ---
>  hurd/hurdinit.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/hurd/hurdinit.c b/hurd/hurdinit.c
> index f88c5666..d0c7a831 100644
> --- a/hurd/hurdinit.c
> +++ b/hurd/hurdinit.c
> @@ -54,6 +54,10 @@ _hurd_ports_use (int which, error_t (*operate) (mach_port_t))
>  
>  DEFINE_HOOK (_hurd_subinit, (void));
>  
> +/* Hook for things which should be initialized as soon as the proc
> +   server is available.  */
> +DEFINE_HOOK (_hurd_proc_subinit, (void));
> +
>  __typeof (_hurd_proc_init) _hurd_new_proc_init;	/* below */
>  
>  /* Initialize the library data structures from the
> @@ -105,6 +109,11 @@ _hurd_init (int flags, char **argv,
>        */
>      }
>  
> +  /* Call other things which want to do some initialization.  These are not
> +     on the _hurd_subinit hook because things there assume that things done
> +     here, like _hurd_pid, are already initialized.  */
> +  RUN_RELHOOK (_hurd_proc_subinit, ());
> +
>    /* Call other things which want to do some initialization.  These are not
>       on the __libc_subinit hook because things there like to be able to
>       assume the availability of the POSIX.1 services we provide.  */
> @@ -148,10 +157,6 @@ libc_hidden_def (_hurd_libc_proc_init)
>     sure the arguments are never visible with `ps'.  */
>  int _hide_arguments, _hide_environment;
>  
> -/* Hook for things which should be initialized as soon as the proc
> -   server is available.  */
> -DEFINE_HOOK (_hurd_proc_subinit, (void));
> -
>  /* Do startup handshaking with the proc server just installed in _hurd_ports.
>     Call _hurdsig_init to set up signal processing.  */
>  
> @@ -187,11 +192,6 @@ _hurd_new_proc_init (char **argv,
>    /* Initialize proc server-assisted fault recovery for the signal thread.  */
>    _hurdsig_fault_init ();
>  
> -  /* Call other things which want to do some initialization.  These are not
> -     on the _hurd_subinit hook because things there assume that things done
> -     here, like _hurd_pid, are already initialized.  */
> -  RUN_RELHOOK (_hurd_proc_subinit, ());
> -
>    /* XXX This code should probably be removed entirely at some point.  This
>       conditional should make it reasonably usable with old gdb's for a
>       while.  Eventually it probably makes most sense for the exec server to
> -- 
> 2.39.2
> 

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