Re: [PATCH] machdev, pci-arbiter, rumpdisk: Fix race condition in bootstrap

[Samuel Thibault]

Applied, thanks!  I just renamed do_machdev_trivfs_server into
machdev_trivfs_server_loop, for coherency of the machdev_ prefix and
coherency with netfs_server_loop.

Damien Zammit, le jeu. 08 sept. 2022 09:32:52 +0000, a ecrit:
> This fixes a known race condition in bootstrapping by
> separating the fsys_startup call from the server demuxer loop
> into two separate functions that the caller can decide
> when to call.
> ---
>  libmachdev/machdev.h       |  3 ++-
>  libmachdev/trivfs_server.c | 18 ++++++++++++------
>  pci-arbiter/main.c         | 31 ++++++++++++-------------------
>  rumpdisk/main.c            |  4 +++-
>  4 files changed, 29 insertions(+), 27 deletions(-)
> 
> diff --git a/libmachdev/machdev.h b/libmachdev/machdev.h
> index e1833cff..05d0c330 100644
> --- a/libmachdev/machdev.h
> +++ b/libmachdev/machdev.h
> @@ -37,7 +37,8 @@ void * machdev_server(void *);
>  error_t machdev_create_device_port (size_t size, void *result);
>  int machdev_trivfs_init(int argc, char **argv, mach_port_t 
> bootstrap_resume_task, const char *name, const char *path, mach_port_t 
> *bootstrap);
>  int machdev_demuxer(mach_msg_header_t *inp, mach_msg_header_t *outp);
> -void machdev_trivfs_server(mach_port_t bootstrap);
> +void machdev_trivfs_server_startup(mach_port_t bootstrap);
> +void * do_machdev_trivfs_server(void *);
>  boolean_t machdev_is_master_device (mach_port_t port);
> 
>  #endif
> diff --git a/libmachdev/trivfs_server.c b/libmachdev/trivfs_server.c
> index 21684dab..79bbcacc 100644
> --- a/libmachdev/trivfs_server.c
> +++ b/libmachdev/trivfs_server.c
> @@ -84,6 +84,9 @@ static task_t parent_task;
>  /* Our argument vector */
>  static char **machdev_argv;
> 
> +/* Our trivfs control port to use in server loop */
> +static struct trivfs_control *global_fsys;
> +
>  static void
>  install_as_translator (mach_port_t bootport)
>  {
> @@ -513,9 +516,8 @@ trivfs_modify_stat (struct trivfs_protid *cred, 
> io_statbuf_t *stat)
>  }
> 
>  void
> -machdev_trivfs_server(mach_port_t bootstrap)
> +machdev_trivfs_server_startup(mach_port_t bootstrap)
>  {
> -  struct trivfs_control *fsys = NULL;
>    int err;
> 
>    if (bootstrapping == FALSE)
> @@ -523,7 +525,7 @@ machdev_trivfs_server(mach_port_t bootstrap)
>        /* This path is executed when a parent exists */
>        err = trivfs_startup (bootstrap, 0,
>                              trivfs_cntl_class, port_bucket,
> -                            trivfs_protid_class, port_bucket, &fsys);
> +                            trivfs_protid_class, port_bucket, &global_fsys);
>        mach_port_deallocate (mach_task_self (), bootstrap);
>        if (err)
>          error (1, err, "Contacting parent");
> @@ -532,14 +534,18 @@ machdev_trivfs_server(mach_port_t bootstrap)
>      }
>    else
>      {
> -      fsys = control;
> +      global_fsys = control;
>      }
> +}
> 
> +void *
> +do_machdev_trivfs_server(void *arg)
> +{
>    /* Launch.  */
>    do
>      {
>        ports_manage_port_operations_one_thread (port_bucket, demuxer, 0);
> -    } while (trivfs_goaway (fsys, 0));
> +    } while (trivfs_goaway (global_fsys, 0));
> 
> -  /* Never reached */
> +  return NULL;
>  }
> diff --git a/pci-arbiter/main.c b/pci-arbiter/main.c
> index ec9be796..00180b24 100644
> --- a/pci-arbiter/main.c
> +++ b/pci-arbiter/main.c
> @@ -188,7 +188,7 @@ main (int argc, char **argv)
>    error_t err;
>    mach_port_t bootstrap;
>    mach_port_t next_task;
> -  pthread_t t, nt;
> +  pthread_t t, mt;
>    file_t underlying_node = MACH_PORT_NULL;
> 
>    /* Parse options */
> @@ -214,7 +214,7 @@ main (int argc, char **argv)
>        machdev_device_init ();
>        err = pthread_create (&t, NULL, machdev_server, NULL);
>        if (err)
> -        error (1, err, "Creating machdev thread");
> +        error (1, err, "Creating machdev_server thread");
>        pthread_detach (t);
>      }
>    else
> @@ -238,17 +238,7 @@ main (int argc, char **argv)
>      error (1, err, "Starting the PCI system");
> 
>    if (next_task != MACH_PORT_NULL)
> -    {
> -      void *run_server(void *arg) {
> -     machdev_trivfs_server(bootstrap);
> -     return NULL;
> -      }
> -
> -      pthread_t t;
> -      pthread_create(&t, NULL, run_server, NULL);
> -      pthread_detach(t);
> -      /* Timer started, quickly do all these next, before we call rump_init 
> */
> -    }
> +    machdev_trivfs_server_startup (bootstrap);
> 
>    if (next_task == MACH_PORT_NULL)
>      underlying_node = netfs_startup (bootstrap, O_READ);
> @@ -275,13 +265,16 @@ main (int argc, char **argv)
>    if (err)
>      error (1, err, "Setting permissions");
> 
> -  err = pthread_create (&nt, NULL, netfs_server_func, NULL);
> -  if (err)
> -    error (1, err, "Creating netfs loop thread");
> -  pthread_detach (nt);
> +  if (next_task != MACH_PORT_NULL)
> +    {
> +      err = pthread_create(&mt, NULL, do_machdev_trivfs_server, NULL);
> +      if (err)
> +        error(1, err, "Creating do_machdev_trivfs_server thread");
> +      pthread_detach(mt);
> +    }
> +
> +  netfs_server_func (NULL);
> 
> -  /* Let the other threads do their job */
> -  pthread_exit(NULL);
>    /* Never reached */
>    return 0;
>  }
> diff --git a/rumpdisk/main.c b/rumpdisk/main.c
> index 9a353541..a502ca2b 100644
> --- a/rumpdisk/main.c
> +++ b/rumpdisk/main.c
> @@ -141,6 +141,8 @@ main (int argc, char **argv)
>    if (err)
>      return err;
>    pthread_detach (t);
> -  machdev_trivfs_server (bootstrap);
> +  machdev_trivfs_server_startup (bootstrap);
> +  do_machdev_trivfs_server (NULL);
> +  /* Never reached */
>    return 0;
>  }
> --
> 2.34.1
> 
> 
> 

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