Re: [Virtio-fs] [PATCH 4/9] virtio-fs: fix use-after-free against virtio_fs_vq's fuse_dev info

[Vivek Goyal <vgoyal@redhat.com>]

Hi,

fuse code does not really track forget requests. If anyting has not been
sent to fuse daemon, that is dropped during abort connection time.

So it makes sense not to wait for these forget requests even in case of
virtio-fs. All we are doing in completion is free request and fuse
device does not have to be around by then.

Problem seems to be dependeny on fud->fpq->lock. How about if we use
a different lock to provide locking around sending/receiving requests
from vq. That way we don't have to use fpq->lock in this path and
this problem should not happen. 

Can you try following patch and see if it fixes the problem.

Thanks
Vivek

Subject: virtio-fs: Use virtio_fs_vq->lock for locking vq

Instead of using fuse_pqueue->lock, use virtio_fs_vq->lock for locking. That
seems more logical as well as helps decouple fuse device and virtio_fs_vq.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/fuse/virtio_fs.c |   41 ++++++++++++++++++++---------------------
 1 file changed, 20 insertions(+), 21 deletions(-)

Index: rhvgoyal-linux-fuse/fs/fuse/virtio_fs.c
===================================================================
--- rhvgoyal-linux-fuse.orig/fs/fuse/virtio_fs.c	2019-04-16 11:34:34.805036786 -0400
+++ rhvgoyal-linux-fuse/fs/fuse/virtio_fs.c	2019-04-23 15:03:40.139017074 -0400
@@ -24,7 +24,8 @@ enum {
 
 /* Per-virtqueue state */
 struct virtio_fs_vq {
-	struct virtqueue *vq;     /* protected by fpq->lock */
+	spinlock_t lock;
+	struct virtqueue *vq;     /* protected by lock */
 	struct work_struct done_work;
 	struct list_head queued_reqs;
 	struct delayed_work dispatch_work;
@@ -180,11 +181,10 @@ static void virtio_fs_hiprio_done_work(s
 {
 	struct virtio_fs_vq *fsvq = container_of(work, struct virtio_fs_vq,
 						 done_work);
-	struct fuse_pqueue *fpq = &fsvq->fud->pq;
 	struct virtqueue *vq = fsvq->vq;
 
 	/* Free completed FUSE_FORGET requests */
-	spin_lock(&fpq->lock);
+	spin_lock(&fsvq->lock);
 	do {
 		unsigned len;
 		void *req;
@@ -194,7 +194,7 @@ static void virtio_fs_hiprio_done_work(s
 		while ((req = virtqueue_get_buf(vq, &len)) != NULL)
 			kfree(req);
 	} while (!virtqueue_enable_cb(vq) && likely(!virtqueue_is_broken(vq)));
-	spin_unlock(&fpq->lock);
+	spin_unlock(&fsvq->lock);
 }
 
 static void virtio_fs_dummy_dispatch_work(struct work_struct *work)
@@ -207,7 +207,6 @@ static void virtio_fs_hiprio_dispatch_wo
 	struct virtio_fs_forget *forget;
 	struct virtio_fs_vq *fsvq = container_of(work, struct virtio_fs_vq,
 						 dispatch_work.work);
-	struct fuse_pqueue *fpq = &fsvq->fud->pq;
 	struct virtqueue *vq = fsvq->vq;
 	struct scatterlist sg;
 	struct scatterlist *sgs[] = {&sg};
@@ -216,11 +215,11 @@ static void virtio_fs_hiprio_dispatch_wo
 
 	pr_debug("worker virtio_fs_hiprio_dispatch_work() called.\n");
 	while(1) {
-		spin_lock(&fpq->lock);
+		spin_lock(&fsvq->lock);
 		forget = list_first_entry_or_null(&fsvq->queued_reqs,
 					struct virtio_fs_forget, list);
 		if (!forget) {
-			spin_unlock(&fpq->lock);
+			spin_unlock(&fsvq->lock);
 			return;
 		}
 
@@ -243,12 +242,12 @@ static void virtio_fs_hiprio_dispatch_wo
 					 " err=%d. Dropping it.\n", ret);
 				kfree(forget);
 			}
-			spin_unlock(&fpq->lock);
+			spin_unlock(&fsvq->lock);
 			return;
 		}
 
 		notify = virtqueue_kick_prepare(vq);
-		spin_unlock(&fpq->lock);
+		spin_unlock(&fsvq->lock);
 
 		if (notify)
 			virtqueue_notify(vq);
@@ -335,7 +334,7 @@ static void virtio_fs_requests_done_work
 	LIST_HEAD(reqs);
 
 	/* Collect completed requests off the virtqueue */
-	spin_lock(&fpq->lock);
+	spin_lock(&fsvq->lock);
 	do {
 		unsigned len;
 
@@ -344,7 +343,7 @@ static void virtio_fs_requests_done_work
 		while ((req = virtqueue_get_buf(vq, &len)) != NULL)
 			list_move_tail(&req->list, &reqs);
 	} while (!virtqueue_enable_cb(vq) && likely(!virtqueue_is_broken(vq)));
-	spin_unlock(&fpq->lock);
+	spin_unlock(&fsvq->lock);
 
 	/* End requests */
 	list_for_each_entry_safe(req, next, &reqs, list) {
@@ -413,9 +412,11 @@ static int virtio_fs_setup_vqs(struct vi
 	INIT_LIST_HEAD(&fs->vqs[VQ_HIPRIO].queued_reqs);
 	INIT_DELAYED_WORK(&fs->vqs[VQ_HIPRIO].dispatch_work,
 			virtio_fs_hiprio_dispatch_work);
+	spin_lock_init(&fs->vqs[VQ_HIPRIO].lock);
 
 	/* Initialize the requests virtqueues */
 	for (i = VQ_REQUEST; i < fs->nvqs; i++) {
+		spin_lock_init(&fs->vqs[i].lock);
 		INIT_WORK(&fs->vqs[i].done_work, virtio_fs_requests_done_work);
 		INIT_DELAYED_WORK(&fs->vqs[i].dispatch_work,
 					virtio_fs_dummy_dispatch_work);
@@ -744,7 +745,6 @@ __releases(fiq->waitq.lock)
 {
 	struct fuse_forget_link *link;
 	struct virtio_fs_forget *forget;
-	struct fuse_pqueue *fpq;
 	struct scatterlist sg;
 	struct scatterlist *sgs[] = {&sg};
 	struct virtio_fs *fs;
@@ -788,8 +788,7 @@ __releases(fiq->waitq.lock)
 	/* Enqueue the request */
 	vq = fsvq->vq;
 	dev_dbg(&vq->vdev->dev, "%s\n", __func__);
-	fpq = vq_to_fpq(vq);
-	spin_lock(&fpq->lock);
+	spin_lock(&fsvq->lock);
 
 	ret = virtqueue_add_sgs(vq, sgs, 1, 0, forget, GFP_ATOMIC);
 	if (ret < 0) {
@@ -804,13 +803,13 @@ __releases(fiq->waitq.lock)
 				 " Dropping it.\n", ret);
 			kfree(forget);
 		}
-		spin_unlock(&fpq->lock);
+		spin_unlock(&fsvq->lock);
 		goto out;
 	}
 
 	notify = virtqueue_kick_prepare(vq);
 
-	spin_unlock(&fpq->lock);
+	spin_unlock(&fsvq->lock);
 
 	if (notify)
 		virtqueue_notify(vq);
@@ -903,7 +902,7 @@ static int virtio_fs_enqueue_req(struct
 	struct scatterlist stack_sg[ARRAY_SIZE(stack_sgs)];
 	struct scatterlist **sgs = stack_sgs;
 	struct scatterlist *sg = stack_sg;
-	struct fuse_pqueue *fpq;
+	struct virtio_fs_vq *fsvq;
 	unsigned argbuf_used = 0;
 	unsigned out_sgs = 0;
 	unsigned in_sgs = 0;
@@ -950,19 +949,19 @@ static int virtio_fs_enqueue_req(struct
 	for (i = 0; i < total_sgs; i++)
 		sgs[i] = &sg[i];
 
-	fpq = vq_to_fpq(vq);
-	spin_lock(&fpq->lock);
+	fsvq = vq_to_fsvq(vq);
+	spin_lock(&fsvq->lock);
 
 	ret = virtqueue_add_sgs(vq, sgs, out_sgs, in_sgs, req, GFP_ATOMIC);
 	if (ret < 0) {
 		/* TODO handle full virtqueue */
-		spin_unlock(&fpq->lock);
+		spin_unlock(&fsvq->lock);
 		goto out;
 	}
 
 	notify = virtqueue_kick_prepare(vq);
 
-	spin_unlock(&fpq->lock);
+	spin_unlock(&fsvq->lock);
 
 	if (notify)
 		virtqueue_notify(vq);