From mboxrd@z Thu Jan 1 00:00:00 1970 Date: Fri, 10 Jan 2020 18:26:38 +0000 From: "Dr. David Alan Gilbert" Message-ID: <20200110182638.GO3901@work-vm> References: <20200107041521.75833-1-eguan@linux.alibaba.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200107041521.75833-1-eguan@linux.alibaba.com> Subject: Re: [Virtio-fs] [PATCH v3] virtiofsd: stop all queue threads on exit in virtio_loop() List-Id: Development discussions about virtio-fs List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eryu Guan Cc: virtio-fs@redhat.com, qingming.su@linux.alibaba.com * Eryu Guan (eguan@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 > Signed-off-by: Eryu Guan > --- > 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@redhat.com > https://www.redhat.com/mailman/listinfo/virtio-fs -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK