Re: [PATCH v3] hurd: Make getrandom cache the server port

[Samuel Thibault]

Samuel Thibault, le ven. 02 déc. 2022 23:46:21 +0100, a ecrit:
> Fixed the indentation and commited, thanks!

I have also uploaded a glibc version 2.36-7~0 to unreleased, so people
can easily upgrade to it and get openssh 9.1 working.

Samuel

> Sergey Bugaev, le ven. 02 déc. 2022 16:55:58 +0300, a ecrit:
> > Changes since v2:
> > * Bring back support for reading /dev/random if so requested with
> >   GRND_RANDOM;
> > * Fix 'git commit' having eaten a #define in the commit message;
> > * Do not pass O_NOCTTY, because:
> >   a) O_NOCTTY is 0; I mixed it up with O_IGNORE_CTTY,
> >   b) we're not opening an fd anyway, so it's irrelevant.
> > 
> > -- >8 --
> > 
> > Previously, getrandom would, each time it's called, traverse the file
> > system to find /dev/urandom, fetch some random data from it, then throw
> > away that port. This is quite slow, while calls to getrandom are
> > genrally expected to be fast.
> > 
> > Additionally, this means that getrandom can not work when /dev/urandom
> > is unavailable, such as inside a chroot that lacks one. User programs
> > expect calls to getrandom to work inside a chroot if they first call
> > getrandom outside of the chroot.
> > 
> > In particular, this is known to break the OpenSSH server, and in that
> > case the issue is exacerbated by the API of arc4random, which prevents
> > it from properly reporting errors, forcing glibc to abort on failure.
> > This causes sshd to just die once it tries to generate a random number.
> > 
> > Caching the random server port, in a manner similar to how socket
> > server ports are cached, both improves the performance and works around
> > the chroot issue.
> > 
> > Tested on i686-gnu with the following program:
> > 
> > #define THREAD_COUNT 100
> > 
> > pthread_barrier_t barrier;
> > 
> > void *worker(void*) {
> >     pthread_barrier_wait(&barrier);
> >     uint32_t sum = 0;
> >     for (int i = 0; i < 10000; i++) {
> >         sum += arc4random();
> >     }
> >     return (void *)(uintptr_t) sum;
> > }
> > 
> > int main() {
> >     pthread_t threads[THREAD_COUNT];
> > 
> >     pthread_barrier_init(&barrier, NULL, THREAD_COUNT);
> > 
> >     for (int i = 0; i < THREAD_COUNT; i++) {
> >         pthread_create(&threads[i], NULL, worker, NULL);
> >     }
> >     for (int i = 0; i < THREAD_COUNT; i++) {
> >         void *retval;
> >         pthread_join(threads[i], &retval);
> >         printf("Thread %i: %lu\n", i, (unsigned long)(uintptr_t) retval);
> >     }
> > 
> > In my totally unscientific benchmark, with this patch, this completes
> > in about 7 seconds, whereas previously it took about 50 seconds. This
> > program was also used to test that getrandom () doesn't explode if the
> > random server dies, but instead reopens the /dev/urandom anew. I have
> > also verified that with this patch, OpenSSH can once again accept
> > connections properly.
> > 
> > Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
> > ---
> >  sysdeps/mach/hurd/getrandom.c | 117 +++++++++++++++++++++++++++++-----
> >  1 file changed, 102 insertions(+), 15 deletions(-)
> > 
> > diff --git a/sysdeps/mach/hurd/getrandom.c b/sysdeps/mach/hurd/getrandom.c
> > index ad2d3ba3..d58c691d 100644
> > --- a/sysdeps/mach/hurd/getrandom.c
> > +++ b/sysdeps/mach/hurd/getrandom.c
> > @@ -16,10 +16,13 @@
> >     License along with the GNU C Library; if not, see
> >     <https://www.gnu.org/licenses/>.  */
> >  
> > +#include <hurd.h>
> >  #include <sys/random.h>
> >  #include <fcntl.h>
> > -#include <unistd.h>
> > -#include <not-cancel.h>
> > +
> > +__libc_rwlock_define_initialized (static, lock);
> > +static file_t random_server, random_server_nonblock,
> > +              urandom_server, urandom_server_nonblock;
> >  
> >  extern char *__trivfs_server_name __attribute__((weak));
> >  
> > @@ -29,9 +32,36 @@ ssize_t
> >  __getrandom (void *buffer, size_t length, unsigned int flags)
> >  {
> >    const char *random_source = "/dev/urandom";
> > -  int open_flags = O_RDONLY | O_CLOEXEC;
> > -  size_t amount_read;
> > -  int fd;
> > +  int open_flags = O_RDONLY;
> > +  file_t server, *cached_server;
> > +  error_t err;
> > +  char *data = buffer;
> > +  mach_msg_type_number_t nread = length;
> > +
> > +  switch (flags)
> > +    {
> > +    case 0:
> > +      cached_server = &urandom_server;
> > +      break;
> > +    case GRND_RANDOM:
> > +      cached_server = &random_server;
> > +      break;
> > +    case GRND_NONBLOCK:
> > +      cached_server = &urandom_server_nonblock;
> > +      break;
> > +    case GRND_RANDOM | GRND_NONBLOCK:
> > +      cached_server = &random_server_nonblock;
> > +      break;
> > +    default:
> > +      return __hurd_fail (EINVAL);
> > +    }
> > +
> > +    if (flags & GRND_RANDOM)
> > +      random_source = "/dev/random";
> > +    if (flags & GRND_NONBLOCK)
> > +      open_flags |= O_NONBLOCK;
> > +    /* No point in passing either O_NOCTTY, O_IGNORE_CTTY, or O_CLOEXEC
> > +       to file_name_lookup, since we're not making an fd.  */
> >  
> >    if (&__trivfs_server_name && __trivfs_server_name
> >        && __trivfs_server_name[0] == 'r'
> > @@ -44,19 +74,76 @@ __getrandom (void *buffer, size_t length, unsigned int 
> > flags)
> >      /* We are random, don't try to read ourselves!  */
> >      return length;
> >  
> > -  if (flags & GRND_RANDOM)
> > -    random_source = "/dev/random";
> > +again:
> > +  __libc_rwlock_rdlock (lock);
> > +  server = *cached_server;
> > +  if (MACH_PORT_VALID (server))
> > +    /* Attempt to read some random data using this port.  */
> > +    err = __io_read (server, &data, &nread, -1, length);
> > +  else
> > +    err = MACH_SEND_INVALID_DEST;
> > +  __libc_rwlock_unlock (lock);
> > +
> > +  if (err == MACH_SEND_INVALID_DEST || err == MIG_SERVER_DIED)
> > +    {
> > +      file_t oldserver = server;
> > +      mach_port_urefs_t urefs;
> > +
> > +      /* Slow path: the cached port didn't work, or there was no
> > +         cached port in the first place.  */
> > +
> > +      __libc_rwlock_wrlock (lock);
> > +      server = *cached_server;
> > +      if (server != oldserver)
> > +        {
> > +          /* Someone else must have refetched the port while we were
> > +             waiting for the lock. */
> > +          __libc_rwlock_unlock (lock);
> > +          goto again;
> > +        }
> > +
> > +      if (MACH_PORT_VALID (server))
> > +        {
> > +          /* It could be that someone else has refetched the port and
> > +             it got the very same name.  So check whether it is a send
> > +             right (and not a dead name).  */
> > +          err = __mach_port_get_refs (__mach_task_self (), server,
> > +                                      MACH_PORT_RIGHT_SEND, &urefs);
> > +          if (!err && urefs > 0)
> > +            {
> > +              __libc_rwlock_unlock (lock);
> > +              goto again;
> > +            }
> > +
> > +          /* Now we're sure that it's dead.  */
> > +          __mach_port_deallocate (__mach_task_self (), server);
> > +        }
> > +
> > +      server = *cached_server = __file_name_lookup (random_source,
> > +                                                    open_flags, 0);
> > +      __libc_rwlock_unlock (lock);
> > +      if (!MACH_PORT_VALID (server))
> > +        /* No luck.  */
> > +        return -1;
> > +
> > +      goto again;
> > +    }
> >  
> > -  if (flags & GRND_NONBLOCK)
> > -    open_flags |= O_NONBLOCK;
> > +  if (err)
> > +    return __hurd_fail (err);
> >  
> > -  fd = __open_nocancel(random_source, open_flags);
> > -  if (fd == -1)
> > -    return -1;
> > +  if (data != buffer)
> > +    {
> > +      if (nread > length)
> > +        {
> > +          __vm_deallocate (__mach_task_self (), (vm_address_t) data, 
> > nread);
> > +          return __hurd_fail (EGRATUITOUS);
> > +        }
> > +      memcpy (buffer, data, nread);
> > +      __vm_deallocate (__mach_task_self (), (vm_address_t) data, nread);
> > +    }
> >  
> > -  amount_read = __read_nocancel(fd, buffer, length);
> > -  __close_nocancel_nostatus(fd);
> > -  return amount_read;
> > +  return nread;
> >  }
> >  
> >  libc_hidden_def (__getrandom)
> > -- 
> > 2.38.1
> > 
> 
> -- 
> Samuel
> ---
> Pour une évaluation indépendante, transparente et rigoureuse !
> Je soutiens la Commission d'Évaluation de l'Inria.

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