[Devel] [PATCH VZ10 0/5] vhost-blk: fix protocol handling and backend setup

[Devel] [PATCH VZ10 0/5] vhost-blk: fix protocol handling and backend setup

[Andrey Zhadchenko]

This series fixes several vhost-blk bugs and harden some userspace
interactions.

First patch hardens VIRTIO_BLK_T_GET_ID. Leave the serial empty and
copy the null terminator, just as QEMU.

The second patch reports the correct used-ring length: the number of
bytes written to the guest, including status.

The third patch fixes flush. REQ_OP_FLUSH is not a valid bio op; use
REQ_OP_WRITE | REQ_PREFLUSH as blkdev_issue_flush() does.

The fourth patch fixes sector alignment checks. SECTOR_MASK is 7, not
a bitmask, so the code was checking 8-byte alignment. Use
(SECTOR_SIZE - 1).

The last patch reworks queue/backend setup. vhost_blk_setup() silently
ignored a changed vq->num once requests were allocated and
double-fetched user input. Request allocation is now tied to backend
existence, and drop_backend/flush/fput are consolidated.

Andrey Zhadchenko (5):
  drivers/vhost/blk: harden get_id command
  drivers/vhost/blk: report correct used-ring lengths
  drivers/vhost/blk: fix flush support
  drivers/vhost/blk: fix sector alignment calculation
  drivers/vhost/blk: rework queue/backend setup

 drivers/vhost/blk.c | 162 ++++++++++++++++++++------------------------
 1 file changed, 74 insertions(+), 88 deletions(-)

-- 
2.43.5

[Devel] [PATCH VZ10 1/5] drivers/vhost/blk: harden get_id command

[Andrey Zhadchenko]

A defensive patch.
QEMU is fine with reporting empty serial and including null
terminator. So we will do the same.

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

diff --git a/drivers/vhost/blk.c b/drivers/vhost/blk.c
index dac03566bfca5..edd3e75873ff5 100644
--- a/drivers/vhost/blk.c
+++ b/drivers/vhost/blk.c
@@ -105,8 +105,6 @@ struct vhost_blk {
 	pid_t owner_pid;
 };
 
-static int gen;
-
 static int move_iovec(struct iovec *from, struct iovec *to,
 		      size_t len, int iov_count_from, int iov_count_to)
 {
@@ -491,7 +489,9 @@ static int vhost_blk_req_handle(struct vhost_virtqueue *vq,
 		ret = vhost_blk_req_submit(req);
 		break;
 	case VIRTIO_BLK_T_GET_ID:
-		len = strnlen(blk->serial, VIRTIO_BLK_ID_BYTES);
+		len = min_t(size_t,
+			    strnlen(blk->serial, VIRTIO_BLK_ID_BYTES) + 1,
+			    min_t(size_t, req->len, VIRTIO_BLK_ID_BYTES));
 		iov_iter_init(&iter, ITER_DEST, req->iov, req->iov_nr, req->len);
 		ret = copy_to_iter(blk->serial, len, &iter);
 		status = ret != len ? VIRTIO_BLK_S_IOERR : VIRTIO_BLK_S_OK;
@@ -688,7 +688,6 @@ static int vhost_blk_open(struct inode *inode, struct file *file)
 	}
 
 	memset(blk->serial, 0, sizeof(blk->serial));
-	snprintf(blk->serial, VIRTIO_BLK_ID_BYTES, "vhost-blk%d", gen++);
 
 	atomic_set(&blk->req_inflight[0], 0);
 	atomic_set(&blk->req_inflight[1], 0);
-- 
2.43.5

[Devel] [PATCH VZ10 2/5] drivers/vhost/blk: report correct used-ring lengths

[Andrey Zhadchenko]

We are expected to return amount of bytes written to the guest,
which also includes status.

https://virtuozzo.atlassian.net/browse/VSTOR-138640
Fixes: 40a5928ec730 ("drivers/vhost: vhost-blk accelerator for virtio-blk guests")
Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
---
 drivers/vhost/blk.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/vhost/blk.c b/drivers/vhost/blk.c
index edd3e75873ff5..36ac2dce7cf6a 100644
--- a/drivers/vhost/blk.c
+++ b/drivers/vhost/blk.c
@@ -445,12 +445,14 @@ static int vhost_blk_req_submit(struct vhost_blk_req *req)
 
 static int vhost_blk_req_handle(struct vhost_virtqueue *vq,
 				struct virtio_blk_outhdr *hdr,
-				u16 head, u16 total_iov_nr)
+				u16 head, u16 out, u16 in)
 {
 	struct vhost_blk *blk = container_of(vq->dev, struct vhost_blk, dev);
 	struct vhost_blk_vq *blk_vq = container_of(vq, struct vhost_blk_vq, vq);
 	struct vhost_blk_req *req;
+	u16 total_iov_nr = out + in;
 	struct iov_iter iter;
+	size_t in_len;
 	int ret, len;
 	u8 status;
 
@@ -461,8 +463,9 @@ static int vhost_blk_req_handle(struct vhost_virtqueue *vq,
 	req->sector	= hdr->sector;
 	req->iov	= blk_vq->iov;
 	req->bio_err	= 0;
+	in_len		= iov_length(vq->iov + out, in);
 
-	if (iov_length(vq->iov, total_iov_nr) < sizeof(status))
+	if (in_len < sizeof(status) || in_len > INT_MAX)
 		return -EINVAL;
 
 	req->len	= iov_length(vq->iov, total_iov_nr) - sizeof(status);
@@ -498,14 +501,14 @@ static int vhost_blk_req_handle(struct vhost_virtqueue *vq,
 		ret = vhost_blk_set_status(req, status);
 		if (ret)
 			break;
-		vhost_add_used_and_signal(&blk->dev, vq, head, len);
+		vhost_add_used_and_signal(&blk->dev, vq, head, in_len);
 		break;
 	default:
 		status = VIRTIO_BLK_S_UNSUPP;
 		ret = vhost_blk_set_status(req, status);
 		if (ret)
 			break;
-		vhost_add_used_and_signal(&blk->dev, vq, head, 0);
+		vhost_add_used_and_signal(&blk->dev, vq, head, sizeof(status));
 	}
 
 	return ret;
@@ -561,7 +564,7 @@ static void vhost_blk_handle_guest_kick(struct vhost_work *work)
 			break;
 		}
 
-		ret = vhost_blk_req_handle(vq, &hdr, head, out + in);
+		ret = vhost_blk_req_handle(vq, &hdr, head, out, in);
 		if (ret == -EAGAIN || ret == -ENOMEM) {
 			vhost_discard_vq_desc(vq, 1);
 			vhost_poll_queue(&vq->poll);
@@ -610,7 +613,7 @@ static void vhost_blk_handle_host_kick(struct vhost_work *work)
 		if (vhost_blk_set_status(req, status)) {
 			vhostblk_vq_err(blk, vq, "Failed to write status");
 		} else {
-			vhost_add_used(vq, req->head, req->len);
+			vhost_add_used(vq, req->head, req->len + sizeof(status));
 			added = true;
 		}
 
-- 
2.43.5

Re: [Devel] [PATCH VZ10 2/5] drivers/vhost/blk: report correct used-ring lengths

[Vasileios Almpanis]

> We are expected to return amount of bytes written to the guest,
> which also includes status.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-138640
> Fixes: 40a5928ec730 ("drivers/vhost: vhost-blk accelerator for virtio-blk guests")
> Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
>
> diff --git a/drivers/vhost/blk.c b/drivers/vhost/blk.c
> index 3def988cdf18..ae6b916f96e1 100644
> --- a/drivers/vhost/blk.c
> +++ b/drivers/vhost/blk.c
> @@ -439,12 +439,14 @@ static int vhost_blk_req_submit(struct vhost_blk_req *req)
>  
>  static int vhost_blk_req_handle(struct vhost_virtqueue *vq,
>  				struct virtio_blk_outhdr *hdr,
> -				u16 head, u16 total_iov_nr)
> +				u16 head, u16 out, u16 in)
>  {
>  	struct vhost_blk *blk = container_of(vq->dev, struct vhost_blk, dev);
>  	struct vhost_blk_vq *blk_vq = container_of(vq, struct vhost_blk_vq, vq);
>  	struct vhost_blk_req *req;
> +	u16 total_iov_nr = out + in;
>  	struct iov_iter iter;
> +	size_t in_len;
>  	int ret, len;
>  	u8 status;
>  
> @@ -455,8 +457,9 @@ static int vhost_blk_req_handle(struct vhost_virtqueue *vq,
>  	req->sector	= hdr->sector;
>  	req->iov	= blk_vq->iov;
>  	req->bio_err	= 0;
> +	in_len		= iov_length(vq->iov + out, in);
>  
> -	if (iov_length(vq->iov, total_iov_nr) < sizeof(status))
> +	if (in_len < sizeof(status) || in_len > INT_MAX)
>  		return -EINVAL;
>  
>  	req->len	= iov_length(vq->iov, total_iov_nr) - sizeof(status);
> @@ -492,14 +495,14 @@ static int vhost_blk_req_handle(struct vhost_virtqueue *vq,
>  		ret = vhost_blk_set_status(req, status);
>  		if (ret)
>  			break;
> -		vhost_add_used_and_signal(&blk->dev, vq, head, len);
> +		vhost_add_used_and_signal(&blk->dev, vq, head, in_len);
>  		break;
>  	default:
>  		status = VIRTIO_BLK_S_UNSUPP;
>  		ret = vhost_blk_set_status(req, status);
>  		if (ret)
>  			break;
> -		vhost_add_used_and_signal(&blk->dev, vq, head, 0);
> +		vhost_add_used_and_signal(&blk->dev, vq, head, sizeof(status));
>  	}
>  
>  	return ret;
> @@ -555,7 +558,7 @@ static void vhost_blk_handle_guest_kick(struct vhost_work *work)
>  			break;
>  		}
>  
> -		ret = vhost_blk_req_handle(vq, &hdr, head, out + in);
> +		ret = vhost_blk_req_handle(vq, &hdr, head, out, in);
>  		if (ret == -EAGAIN || ret == -ENOMEM) {
>  			vhost_discard_vq_desc(vq, 1);
>  			vhost_poll_queue(&vq->poll);
> @@ -604,7 +607,7 @@ static void vhost_blk_handle_host_kick(struct vhost_work *work)
>  		if (vhost_blk_set_status(req, status)) {
>  			vhostblk_vq_err(blk, vq, "Failed to write status");
>  		} else {
> -			vhost_add_used(vq, req->head, req->len);
> +			vhost_add_used(vq, req->head, req->len + sizeof(status));
Is req->len + sizeof(status) the right thing to use for write requests?
We should only write the status byte not status + payload length

-- 
Vasileios Almpanis <vasileios.almpanis@virtuozzo.com>

Re: [Devel] [PATCH VZ10 2/5] drivers/vhost/blk: report correct used-ring lengths

[Andrey Zhadchenko]

On 8/20/26 16:11, Vasileios Almpanis wrote:
>> We are expected to return amount of bytes written to the guest,
>> which also includes status.
>>
>> https://virtuozzo.atlassian.net/browse/VSTOR-138640
>> Fixes: 40a5928ec730 ("drivers/vhost: vhost-blk accelerator for virtio-blk guests")
>> Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
>>
>> diff --git a/drivers/vhost/blk.c b/drivers/vhost/blk.c
>> index 3def988cdf18..ae6b916f96e1 100644
>> --- a/drivers/vhost/blk.c
>> +++ b/drivers/vhost/blk.c
>> @@ -439,12 +439,14 @@ static int vhost_blk_req_submit(struct vhost_blk_req *req)
>>   
>>   static int vhost_blk_req_handle(struct vhost_virtqueue *vq,
>>   				struct virtio_blk_outhdr *hdr,
>> -				u16 head, u16 total_iov_nr)
>> +				u16 head, u16 out, u16 in)
>>   {
>>   	struct vhost_blk *blk = container_of(vq->dev, struct vhost_blk, dev);
>>   	struct vhost_blk_vq *blk_vq = container_of(vq, struct vhost_blk_vq, vq);
>>   	struct vhost_blk_req *req;
>> +	u16 total_iov_nr = out + in;
>>   	struct iov_iter iter;
>> +	size_t in_len;
>>   	int ret, len;
>>   	u8 status;
>>   
>> @@ -455,8 +457,9 @@ static int vhost_blk_req_handle(struct vhost_virtqueue *vq,
>>   	req->sector	= hdr->sector;
>>   	req->iov	= blk_vq->iov;
>>   	req->bio_err	= 0;
>> +	in_len		= iov_length(vq->iov + out, in);
>>   
>> -	if (iov_length(vq->iov, total_iov_nr) < sizeof(status))
>> +	if (in_len < sizeof(status) || in_len > INT_MAX)
>>   		return -EINVAL;
>>   
>>   	req->len	= iov_length(vq->iov, total_iov_nr) - sizeof(status);
>> @@ -492,14 +495,14 @@ static int vhost_blk_req_handle(struct vhost_virtqueue *vq,
>>   		ret = vhost_blk_set_status(req, status);
>>   		if (ret)
>>   			break;
>> -		vhost_add_used_and_signal(&blk->dev, vq, head, len);
>> +		vhost_add_used_and_signal(&blk->dev, vq, head, in_len);
>>   		break;
>>   	default:
>>   		status = VIRTIO_BLK_S_UNSUPP;
>>   		ret = vhost_blk_set_status(req, status);
>>   		if (ret)
>>   			break;
>> -		vhost_add_used_and_signal(&blk->dev, vq, head, 0);
>> +		vhost_add_used_and_signal(&blk->dev, vq, head, sizeof(status));
>>   	}
>>   
>>   	return ret;
>> @@ -555,7 +558,7 @@ static void vhost_blk_handle_guest_kick(struct vhost_work *work)
>>   			break;
>>   		}
>>   
>> -		ret = vhost_blk_req_handle(vq, &hdr, head, out + in);
>> +		ret = vhost_blk_req_handle(vq, &hdr, head, out, in);
>>   		if (ret == -EAGAIN || ret == -ENOMEM) {
>>   			vhost_discard_vq_desc(vq, 1);
>>   			vhost_poll_queue(&vq->poll);
>> @@ -604,7 +607,7 @@ static void vhost_blk_handle_host_kick(struct vhost_work *work)
>>   		if (vhost_blk_set_status(req, status)) {
>>   			vhostblk_vq_err(blk, vq, "Failed to write status");
>>   		} else {
>> -			vhost_add_used(vq, req->head, req->len);
>> +			vhost_add_used(vq, req->head, req->len + sizeof(status));
> Is req->len + sizeof(status) the right thing to use for write requests?
> We should only write the status byte not status + payload length
> 

Oh yes. I fixed the missing status but not this bug. Thanks!

[Devel] [PATCH VZ10 3/5] drivers/vhost/blk: fix flush support

[Andrey Zhadchenko]

REQ_OP_FLUSH is a technical flag for requests. For bios the
correct combination would be REQ_OP_WRITE | REQ_PREFLUSH.
See an example in blkdev_issue_flush().

https://virtuozzo.atlassian.net/browse/VSTOR-138640
Fixes: 40a5928ec730 ("drivers/vhost: vhost-blk accelerator for virtio-blk guests")
Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
---
 drivers/vhost/blk.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/vhost/blk.c b/drivers/vhost/blk.c
index 36ac2dce7cf6a..43bc122ff17e3 100644
--- a/drivers/vhost/blk.c
+++ b/drivers/vhost/blk.c
@@ -274,7 +274,7 @@ static int vhost_blk_bio_make(struct vhost_blk_req *req,
 	sector_t sector = req->sector;
 	unsigned long pos = 0;
 
-	if (unlikely(req->bi_opf == REQ_OP_FLUSH))
+	if (unlikely(req->bi_opf & REQ_PREFLUSH))
 		return vhost_blk_bio_make_simple(req, bdev);
 
 	if (req->bi_opf == REQ_OP_WRITE && req->len & SECTOR_MASK) {
@@ -488,7 +488,7 @@ static int vhost_blk_req_handle(struct vhost_virtqueue *vq,
 		ret = vhost_blk_req_submit(req);
 		break;
 	case VIRTIO_BLK_T_FLUSH:
-		req->bi_opf = REQ_OP_FLUSH;
+		req->bi_opf = REQ_OP_WRITE | REQ_PREFLUSH;
 		ret = vhost_blk_req_submit(req);
 		break;
 	case VIRTIO_BLK_T_GET_ID:
-- 
2.43.5

[Devel] [PATCH VZ10 4/5] drivers/vhost/blk: fix sector alignment calculation

[Andrey Zhadchenko]

SECTOR_MASK is actually a number 7, not some bitmask. Hence
the code actually checked 8 byte alignment.
Use (SECTOR_SIZE - 1) to correctly check sector alignment.

https://virtuozzo.atlassian.net/browse/VSTOR-138640
Fixes: a0d3b8956fb0 ("vhost-blk: rework iov and bio handling")
Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
---
 drivers/vhost/blk.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/vhost/blk.c b/drivers/vhost/blk.c
index 43bc122ff17e3..2f94c987c62d8 100644
--- a/drivers/vhost/blk.c
+++ b/drivers/vhost/blk.c
@@ -209,13 +209,13 @@ inline static bool vhost_blk_iov_need_bb(struct vhost_blk_req *req)
 {
 	int i;
 
-	if (req->len & SECTOR_MASK)
+	if (req->len & (SECTOR_SIZE - 1))
 		return true;
 
 	for (i = 0; i < req->iov_nr; i++) {
-		if (((size_t)req->iov[i].iov_base) & SECTOR_MASK)
+		if (((size_t)req->iov[i].iov_base) & (SECTOR_SIZE - 1))
 			return true;
-		if (req->iov[i].iov_len & SECTOR_MASK)
+		if (req->iov[i].iov_len & (SECTOR_SIZE - 1))
 			return true;
 	}
 
@@ -277,7 +277,7 @@ static int vhost_blk_bio_make(struct vhost_blk_req *req,
 	if (unlikely(req->bi_opf & REQ_PREFLUSH))
 		return vhost_blk_bio_make_simple(req, bdev);
 
-	if (req->bi_opf == REQ_OP_WRITE && req->len & SECTOR_MASK) {
+	if (req->bi_opf == REQ_OP_WRITE && req->len & (SECTOR_SIZE - 1)) {
 		WARN_ONCE(1, "vhost-blk: write requests with unaligned len"
 			  " are not supported, len = %zu", req->len);
 		return -EINVAL;
@@ -339,7 +339,7 @@ static int vhost_blk_bio_make(struct vhost_blk_req *req,
 	nr_pages = bio_iov_vecs_to_alloc(&iter, BIO_MAX_VECS);
 	do {
 		/* We can't handle next bio if it's start is not sector aligned */
-		if (pos & SECTOR_MASK) {
+		if (pos & (SECTOR_SIZE - 1)) {
 			WARN_ONCE(1, "vhost-blk: guest provided unaligned buffers");
 			ret = -EINVAL;
 			goto err_bio;
-- 
2.43.5

Re: [Devel] [PATCH VZ10 4/5] drivers/vhost/blk: fix sector alignment calculation

[Andrey Zhadchenko]

Actually for v2 I will replace manual calculations with IS_ALIGNED macro 
so we don't step on this again.

On 8/18/26 17:09, Andrey Zhadchenko wrote:
> SECTOR_MASK is actually a number 7, not some bitmask. Hence
> the code actually checked 8 byte alignment.
> Use (SECTOR_SIZE - 1) to correctly check sector alignment.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-138640
> Fixes: a0d3b8956fb0 ("vhost-blk: rework iov and bio handling")
> Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
> ---
>   drivers/vhost/blk.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/vhost/blk.c b/drivers/vhost/blk.c
> index 43bc122ff17e3..2f94c987c62d8 100644
> --- a/drivers/vhost/blk.c
> +++ b/drivers/vhost/blk.c
> @@ -209,13 +209,13 @@ inline static bool vhost_blk_iov_need_bb(struct vhost_blk_req *req)
>   {
>   	int i;
>   
> -	if (req->len & SECTOR_MASK)
> +	if (req->len & (SECTOR_SIZE - 1))
>   		return true;
>   
>   	for (i = 0; i < req->iov_nr; i++) {
> -		if (((size_t)req->iov[i].iov_base) & SECTOR_MASK)
> +		if (((size_t)req->iov[i].iov_base) & (SECTOR_SIZE - 1))
>   			return true;
> -		if (req->iov[i].iov_len & SECTOR_MASK)
> +		if (req->iov[i].iov_len & (SECTOR_SIZE - 1))
>   			return true;
>   	}
>   
> @@ -277,7 +277,7 @@ static int vhost_blk_bio_make(struct vhost_blk_req *req,
>   	if (unlikely(req->bi_opf & REQ_PREFLUSH))
>   		return vhost_blk_bio_make_simple(req, bdev);
>   
> -	if (req->bi_opf == REQ_OP_WRITE && req->len & SECTOR_MASK) {
> +	if (req->bi_opf == REQ_OP_WRITE && req->len & (SECTOR_SIZE - 1)) {
>   		WARN_ONCE(1, "vhost-blk: write requests with unaligned len"
>   			  " are not supported, len = %zu", req->len);
>   		return -EINVAL;
> @@ -339,7 +339,7 @@ static int vhost_blk_bio_make(struct vhost_blk_req *req,
>   	nr_pages = bio_iov_vecs_to_alloc(&iter, BIO_MAX_VECS);
>   	do {
>   		/* We can't handle next bio if it's start is not sector aligned */
> -		if (pos & SECTOR_MASK) {
> +		if (pos & (SECTOR_SIZE - 1)) {
>   			WARN_ONCE(1, "vhost-blk: guest provided unaligned buffers");
>   			ret = -EINVAL;
>   			goto err_bio;

[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@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

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

[Vasileios Almpanis]

> 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
Fixes: 40a5928ec730 ("drivers/vhost: vhost-blk accelerator for virtio-blk guests")
> Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
>
> diff --git a/drivers/vhost/blk.c b/drivers/vhost/blk.c
> index 070e5ce30a16..17ca135867e6 100644
> --- a/drivers/vhost/blk.c
> +++ b/drivers/vhost/blk.c
> @@ -647,11 +647,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;
>  
> @@ -659,6 +662,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);
NIT: extra blank line
> +
> +	}
> +
> +	return 0;
>  }
>  
>  static int vhost_blk_open(struct inode *inode, struct file *file)
> @@ -711,16 +753,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);
>  
> @@ -754,32 +790,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) {
> @@ -796,31 +819,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);
> -	}
NIT: extra blank line

-- 
Vasileios Almpanis <vasileios.almpanis@virtuozzo.com>