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

* [Virtio-fs] [PATCH v3] virtiofsd: stop all queue threads on exit in virtio_loop()
@ 2020-01-07  4:15 Eryu Guan
  2020-01-07 16:58 ` Stefan Hajnoczi
  2020-01-10 18:26 ` Dr. David Alan Gilbert
  0 siblings, 2 replies; 3+ messages in thread
From: Eryu Guan @ 2020-01-07  4:15 UTC (permalink / raw)
  To: virtio-fs; +Cc: qingming.su

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@linux.alibaba.com>
Signed-off-by: Eryu Guan <eguan@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

 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



^ permalink raw reply related	[flat|nested] 3+ messages in thread