* [Virtio-fs] [PATCH v2] virtiofsd: cancel all queue threads on exit in virtio_loop()
@ 2019-12-20 3:12 Eryu Guan
2020-01-06 15:47 ` Stefan Hajnoczi
0 siblings, 1 reply; 3+ messages in thread
From: Eryu Guan @ 2019-12-20 3:12 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 cancelling 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>
---
v1: virtiofsd: sync FUSE_DESTROY with session destroy
https://www.redhat.com/archives/virtio-fs/2019-December/msg00051.html
tools/virtiofsd/fuse_virtio.c | 36 +++++++++++++++++++++++++++++++++++-
1 file changed, 35 insertions(+), 1 deletion(-)
diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
index 254b0a71cd0b..2ae38758567e 100644
--- a/tools/virtiofsd/fuse_virtio.c
+++ b/tools/virtiofsd/fuse_virtio.c
@@ -671,6 +671,11 @@ out:
free(req);
}
+static void worker_cleanup(void *data)
+{
+ g_thread_pool_free((GThreadPool *)data, FALSE, TRUE);
+}
+
/* Thread function for individual queues, created when a queue is 'started' */
static void *fv_queue_thread(void *opaque)
{
@@ -687,6 +692,8 @@ static void *fv_queue_thread(void *opaque)
return NULL;
}
+ pthread_cleanup_push(worker_cleanup, pool);
+
fuse_log(FUSE_LOG_INFO, "%s: Start for queue %d kick_fd %d\n", __func__,
qi->qidx, qi->kick_fd);
while (1) {
@@ -770,7 +777,7 @@ static void *fv_queue_thread(void *opaque)
pthread_rwlock_unlock(&qi->virtio_dev->vu_dispatch_rwlock);
}
- g_thread_pool_free(pool, FALSE, TRUE);
+ pthread_cleanup_pop(1);
return NULL;
}
@@ -916,6 +923,33 @@ 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++) {
+ int ret;
+ pthread_t tid;
+
+ if (!se->virtio_dev->qi[i])
+ continue;
+
+ tid = se->virtio_dev->qi[i]->thread;
+ fuse_log(FUSE_LOG_INFO, "%s: Canceling queue %d thread (%lu)\n",
+ __func__, i, tid);
+ ret = pthread_cancel(tid);
+ if (ret) {
+ fuse_log(FUSE_LOG_WARNING, "%s: Cancel queue %d thread: %s\n",
+ __func__, i, strerror(ret));
+ } else {
+ ret = pthread_join(tid, NULL);
+ if (ret) {
+ fuse_log(FUSE_LOG_WARNING, "%s: Join queue %d thread: %s\n",
+ __func__, i, strerror(ret));
+ }
+ }
+ }
+
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
* Re: [Virtio-fs] [PATCH v2] virtiofsd: cancel all queue threads on exit in virtio_loop()
2019-12-20 3:12 [Virtio-fs] [PATCH v2] virtiofsd: cancel all queue threads on exit in virtio_loop() Eryu Guan
@ 2020-01-06 15:47 ` Stefan Hajnoczi
2020-01-07 4:09 ` Eryu Guan
0 siblings, 1 reply; 3+ messages in thread
From: Stefan Hajnoczi @ 2020-01-06 15:47 UTC (permalink / raw)
To: Eryu Guan; +Cc: virtio-fs, qingming.su
[-- Attachment #1: Type: text/plain, Size: 1872 bytes --]
On Fri, Dec 20, 2019 at 11:12:13AM +0800, Eryu Guan 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 cancelling fv_queue_thread()s before virtio_loop() returns,
> to make sure there's no-one could access fuse session and virtio dev.
Why use pthread_cancel(3) instead of writing 1 to the thread's kill_fd?
I'm concerned about pthread_cancel(3) because the threads access
rwlocks, which may be leaked when pthread_cancel(3) terminates the
thread.
It would be safer to reuse the following code to get full thread
cleanup:
static void fv_queue_set_started(VuDev *dev, int qidx, bool started)
{
...
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_read 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;
...
}
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Virtio-fs] [PATCH v2] virtiofsd: cancel all queue threads on exit in virtio_loop()
2020-01-06 15:47 ` Stefan Hajnoczi
@ 2020-01-07 4:09 ` Eryu Guan
0 siblings, 0 replies; 3+ messages in thread
From: Eryu Guan @ 2020-01-07 4:09 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: virtio-fs, qingming.su
On Mon, Jan 06, 2020 at 03:47:10PM +0000, Stefan Hajnoczi wrote:
> On Fri, Dec 20, 2019 at 11:12:13AM +0800, Eryu Guan 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 cancelling fv_queue_thread()s before virtio_loop() returns,
> > to make sure there's no-one could access fuse session and virtio dev.
>
> Why use pthread_cancel(3) instead of writing 1 to the thread's kill_fd?
>
> I'm concerned about pthread_cancel(3) because the threads access
> rwlocks, which may be leaked when pthread_cancel(3) terminates the
> thread.
>
> It would be safer to reuse the following code to get full thread
> cleanup:
Makes sense, fixed in v3.
Thanks,
Eryu
>
> static void fv_queue_set_started(VuDev *dev, int qidx, bool started)
> {
> ...
> 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_read 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;
> ...
> }
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-01-07 4:09 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-20 3:12 [Virtio-fs] [PATCH v2] virtiofsd: cancel all queue threads on exit in virtio_loop() Eryu Guan
2020-01-06 15:47 ` Stefan Hajnoczi
2020-01-07 4:09 ` Eryu Guan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.