[Virtio-fs] [PATCH v3] virtiofsd: stop all queue threads on exit in virtio_loop()

[Dr. David Alan Gilbert]

* Eryu Guan (eguan at linux.alibaba.com) wrote:
> On guest graceful shutdown, virtiofsd receives VHOST_USER_GET_VRING_BASE
> request from VMM and shuts down virtqueues by calling fv_set_started(),
> which joins fv_queue_thread() threads. So when virtio_loop() returns,
> there should be no thread is still accessing data in fuse session and/or
> virtio dev.
> 
> But on abnormal exit, e.g. guest got killed for whatever reason,
> vhost-user socket is closed and virtio_loop() breaks out the main loop
> and returns to main(). But it's possible fv_queue_worker()s are still
> working and accessing fuse session and virtio dev, which results in
> crash or use-after-free.
> 
> Fix it by stopping fv_queue_thread()s before virtio_loop() returns,
> to make sure there's no-one could access fuse session and virtio dev.
> 
> Reported-by: Qingming Su <qingming.su at linux.alibaba.com>
> Signed-off-by: Eryu Guan <eguan at linux.alibaba.com>
> ---
> v3:
> - stopping fv_queue_thread by writing to qi->kill_fd instead of
>   cancelling thread, as suggested by Stefan Hajnoczi
> 
> v2:
> - cancelling fv_queue_thread before exit
> 
> v1: virtiofsd: sync FUSE_DESTROY with session destroy
> https://www.redhat.com/archives/virtio-fs/2019-December/msg00051.html

Thanks; I've moved your split of 'fv_queue_cleanup_thread' out of
fv_queue_set_started into the existing 'Kill threads when queues are
stopped' and then made this patch just have the extra part in
virtio_loop.

Dave


>  tools/virtiofsd/fuse_virtio.c | 56 +++++++++++++++++++++++++++++--------------
>  1 file changed, 38 insertions(+), 18 deletions(-)
> 
> diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
> index b6edd88b00fd..b93faa963fa2 100644
> --- a/tools/virtiofsd/fuse_virtio.c
> +++ b/tools/virtiofsd/fuse_virtio.c
> @@ -775,6 +775,31 @@ static void *fv_queue_thread(void *opaque)
>      return NULL;
>  }
>  
> +static void fv_queue_cleanup_thread(struct fv_VuDev *vud, int qidx)
> +{
> +    int ret;
> +    struct fv_QueueInfo *ourqi;
> +
> +    assert(qidx < vud->nqueues);
> +    ourqi = vud->qi[qidx];
> +
> +    /* Kill the thread */
> +    if (eventfd_write(ourqi->kill_fd, 1)) {
> +        fuse_log(FUSE_LOG_ERR, "Eventfd_write for queue %d: %s\n",
> +	         qidx, strerror(errno));
> +    }
> +    ret = pthread_join(ourqi->thread, NULL);
> +    if (ret) {
> +        fuse_log(FUSE_LOG_ERR, "%s: Failed to join thread idx %d err %d\n",
> +	         __func__, qidx, ret);
> +    }
> +    pthread_mutex_destroy(&ourqi->vq_lock);
> +    close(ourqi->kill_fd);
> +    ourqi->kick_fd = -1;
> +    free(vud->qi[qidx]);
> +    vud->qi[qidx] = NULL;
> +}
> +
>  /* Callback from libvhost-user on start or stop of a queue */
>  static void fv_queue_set_started(VuDev *dev, int qidx, bool started)
>  {
> @@ -830,24 +855,7 @@ static void fv_queue_set_started(VuDev *dev, int qidx, bool started)
>              assert(0);
>          }
>      } else {
> -        int ret;
> -        assert(qidx < vud->nqueues);
> -        ourqi = vud->qi[qidx];
> -
> -        /* Kill the thread */
> -        if (eventfd_write(ourqi->kill_fd, 1)) {
> -            fuse_log(FUSE_LOG_ERR, "Eventfd_write for queue: %m\n");
> -        }
> -        ret = pthread_join(ourqi->thread, NULL);
> -        if (ret) {
> -            fuse_log(FUSE_LOG_ERR, "%s: Failed to join thread idx %d err %d\n",
> -                     __func__, qidx, ret);
> -        }
> -        pthread_mutex_destroy(&ourqi->vq_lock);
> -        close(ourqi->kill_fd);
> -        ourqi->kick_fd = -1;
> -        free(vud->qi[qidx]);
> -        vud->qi[qidx] = NULL;
> +        fv_queue_cleanup_thread(vud, qidx);
>      }
>  }
>  
> @@ -916,6 +924,18 @@ int virtio_loop(struct fuse_session *se)
>          }
>      }
>  
> +    /*
> +     * Make sure all fv_queue_thread()s quit on exit, as we're about to
> +     * free virtio dev and fuse session, no one should access them anymore.
> +     */
> +    for (int i = 0; i < se->virtio_dev->nqueues; i++) {
> +        if (!se->virtio_dev->qi[i])
> +            continue;
> +
> +        fuse_log(FUSE_LOG_INFO, "%s: Stopping queue %d thread\n", __func__, i);
> +        fv_queue_cleanup_thread(se->virtio_dev, i);
> +    }
> +
>      fuse_log(FUSE_LOG_INFO, "%s: Exit\n", __func__);
>  
>      return 0;
> -- 
> 2.14.4.44.g2045bb6
> 
> 
> _______________________________________________
> Virtio-fs mailing list
> Virtio-fs at redhat.com
> https://www.redhat.com/mailman/listinfo/virtio-fs
--
Dr. David Alan Gilbert / dgilbert at redhat.com / Manchester, UK