[Devel] [PATCH VZ10 5/5] drivers/vhost/blk: rework queue/backend setup

[Andrey Zhadchenko]

vhost_blk_setup() is pretty bad: silently refusing changed vq->num
if requests are already allocated, fetching user input second time
(double-fetch vulnerability).
To handle this, tie request allocation to backend existence. After
all, if there is no backend, there is no point in having requests.
Also expand it to get rid of boilerplate drop_backend, flush,
fput sequence in a few places.

https://virtuozzo.atlassian.net/browse/VSTOR-138640
Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko at virtuozzo.com>
---
 drivers/vhost/blk.c | 126 +++++++++++++++++++-------------------------
 1 file changed, 55 insertions(+), 71 deletions(-)

diff --git a/drivers/vhost/blk.c b/drivers/vhost/blk.c
index 2f94c987c62d8..fd12d4ee317a0 100644
--- a/drivers/vhost/blk.c
+++ b/drivers/vhost/blk.c
@@ -653,11 +653,14 @@ static void vhost_blk_flush(struct vhost_blk *blk)
 	spin_unlock(&blk->flush_lock);
 }
 
-static inline void vhost_blk_drop_backends(struct vhost_blk *blk)
+static void vhost_blk_drop_backend(struct vhost_blk *blk)
 {
 	struct vhost_virtqueue *vq;
 	int i;
 
+	if (!blk->backend)
+		return;
+
 	for (i = 0; i < VHOST_BLK_VQ_MAX; i++) {
 		vq = &blk->vqs[i].vq;
 
@@ -665,6 +668,45 @@ static inline void vhost_blk_drop_backends(struct vhost_blk *blk)
 		vhost_vq_set_backend(vq, NULL);
 		mutex_unlock(&vq->mutex);
 	}
+
+	vhost_blk_flush(blk);
+
+	for (i = 0; i < VHOST_BLK_VQ_MAX; i++) {
+		kvfree(blk->vqs[i].req);
+		blk->vqs[i].req = NULL;
+	}
+
+	fput(blk->backend);
+	blk->backend = NULL;
+}
+
+static int vhost_blk_setup_vqs(struct vhost_blk *blk)
+{
+	struct vhost_virtqueue *vq;
+	int i;
+
+	for (i = 0; i < VHOST_BLK_VQ_MAX; i++) {
+		vq = &blk->vqs[i].vq;
+
+		if (!vhost_vq_is_setup(vq))
+			continue;
+
+		blk->vqs[i].req = kvmalloc_array(vq->num, sizeof(struct vhost_blk_req),
+						 GFP_KERNEL);
+		if (!blk->vqs[i].req)
+			return -ENOMEM;
+
+		mutex_lock(&vq->mutex);
+		vhost_vq_set_backend(vq, blk->backend);
+		if (vhost_vq_init_access(vq)) {
+			mutex_unlock(&vq->mutex);
+			return -EFAULT;
+		}
+		mutex_unlock(&vq->mutex);
+
+	}
+
+	return 0;
 }
 
 static int vhost_blk_open(struct inode *inode, struct file *file)
@@ -717,16 +759,10 @@ static int vhost_blk_open(struct inode *inode, struct file *file)
 static int vhost_blk_release(struct inode *inode, struct file *f)
 {
 	struct vhost_blk *blk = f->private_data;
-	int i;
 
-	vhost_blk_drop_backends(blk);
-	vhost_blk_flush(blk);
+	vhost_blk_drop_backend(blk);
 	vhost_dev_stop(&blk->dev);
-	if (blk->backend)
-		fput(blk->backend);
 	vhost_dev_cleanup(&blk->dev);
-	for (i = 0; i < VHOST_BLK_VQ_MAX; i++)
-		kvfree(blk->vqs[i].req);
 	kfree(blk->dev.vqs);
 	kvfree(blk);
 
@@ -760,32 +796,19 @@ static int vhost_blk_set_features(struct vhost_blk *blk, u64 features)
 
 static long vhost_blk_set_backend(struct vhost_blk *blk, int fd)
 {
-	struct vhost_virtqueue *vq;
 	struct file *file;
 	struct inode *inode;
-	int ret, i;
+	int ret;
 
 	mutex_lock(&blk->dev.mutex);
 	ret = vhost_dev_check_owner(&blk->dev);
 	if (ret)
 		goto out_dev;
 
-	/*
-	 * fd < 0 means "stop the device".  Detach the backend from every vq so
-	 * vhost_blk_handle_guest_kick() stops fetching descriptors, drain the
-	 * in-flight requests, and release the backing file.
-	 */
+	/* fd < 0 means "stop the device" */
 	if (fd < 0) {
-		if (!blk->backend) {
-			ret = 0;		/* already stopped */
-			goto out_dev;
-		}
-		vhost_blk_drop_backends(blk);
-		vhost_blk_flush(blk);
-		fput(blk->backend);
-		blk->backend = NULL;
 		ret = 0;
-		goto out_dev;
+		goto out_drop;
 	}
 
 	if (blk->backend) {
@@ -802,31 +825,21 @@ static long vhost_blk_set_backend(struct vhost_blk *blk, int fd)
 	inode = file->f_mapping->host;
 	if (!S_ISBLK(inode->i_mode)) {
 		ret = -EFAULT;
-		goto out_file;
+		fput(file);
+		goto out_dev;
 	}
 
-	for (i = 0; i < VHOST_BLK_VQ_MAX; i++) {
-		vq = &blk->vqs[i].vq;
-		if (!vhost_vq_access_ok(vq)) {
-			ret = -EFAULT;
-			goto out_drop;
-		}
-
-		mutex_lock(&vq->mutex);
-		vhost_vq_set_backend(vq, file);
-		ret = vhost_vq_init_access(vq);
-		mutex_unlock(&vq->mutex);
-	}
 
 	blk->backend = file;
+	ret = vhost_blk_setup_vqs(blk);
+	if (ret)
+		goto out_drop;
 
 	mutex_unlock(&blk->dev.mutex);
 	return 0;
 
 out_drop:
-	vhost_blk_drop_backends(blk);
-out_file:
-	fput(file);
+	vhost_blk_drop_backend(blk);
 out_dev:
 	mutex_unlock(&blk->dev.mutex);
 	return ret;
@@ -835,7 +848,7 @@ static long vhost_blk_set_backend(struct vhost_blk *blk, int fd)
 static long vhost_blk_reset_owner(struct vhost_blk *blk)
 {
 	struct vhost_iotlb *umem;
-	int err, i;
+	int err;
 
 	mutex_lock(&blk->dev.mutex);
 	err = vhost_dev_check_owner(&blk->dev);
@@ -846,42 +859,15 @@ static long vhost_blk_reset_owner(struct vhost_blk *blk)
 		err = -ENOMEM;
 		goto done;
 	}
-	vhost_blk_drop_backends(blk);
-	if (blk->backend) {
-		fput(blk->backend);
-		blk->backend = NULL;
-	}
-	vhost_blk_flush(blk);
+	vhost_blk_drop_backend(blk);
 	vhost_dev_stop(&blk->dev);
 	vhost_dev_reset_owner(&blk->dev, umem);
 
-	for (i = 0; i < VHOST_BLK_VQ_MAX; i++) {
-		kvfree(blk->vqs[i].req);
-		blk->vqs[i].req = NULL;
-	}
-
 done:
 	mutex_unlock(&blk->dev.mutex);
 	return err;
 }
 
-static int vhost_blk_setup(struct vhost_blk *blk, void __user *argp)
-{
-	struct vhost_vring_state s;
-
-	if (copy_from_user(&s, argp, sizeof(s)))
-		return -EFAULT;
-
-	if (blk->vqs[s.index].req)
-		return 0;
-
-	blk->vqs[s.index].req = kvmalloc(sizeof(struct vhost_blk_req) * s.num, GFP_KERNEL);
-	if (!blk->vqs[s.index].req)
-		return -ENOMEM;
-
-	return 0;
-}
-
 static long vhost_blk_ioctl(struct file *f, unsigned int ioctl,
 			    unsigned long arg)
 {
@@ -919,8 +905,6 @@ static long vhost_blk_ioctl(struct file *f, unsigned int ioctl,
 		ret = vhost_dev_ioctl(&blk->dev, ioctl, argp);
 		if (ret == -ENOIOCTLCMD)
 			ret = vhost_vring_ioctl(&blk->dev, ioctl, argp);
-		if (!ret && ioctl == VHOST_SET_VRING_NUM)
-			ret = vhost_blk_setup(blk, argp);
 		vhost_blk_flush(blk);
 		mutex_unlock(&blk->dev.mutex);
 		return ret;
-- 
2.43.5