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

[Samuel Thibault]

Fixed the indentation and commited, thanks!

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.