From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
Date: Tue, 25 Aug 2026 15:27:52 +0200
Subject: Re: [Devel] [PATCH VZ10 v3 5/5] drivers/vhost/blk: rework
	queue/backend setup
In-Reply-To: <a248d20f-0af1-4f05-aa89-d696d5cf364c@virtuozzo.com>
References: <20260825125152.259376-1-andrey.zhadchenko@virtuozzo.com>
	<20260825125152.259376-6-andrey.zhadchenko@virtuozzo.com>
	<a248d20f-0af1-4f05-aa89-d696d5cf364c@virtuozzo.com>
Message-ID: <cdd14089-2655-4d70-959b-9d96978cb3a6@virtuozzo.com>
List-Id: <devel.openvz.org>



On 8/25/26 15:22, Konstantin Khorenko wrote:
> On 8/25/26 14:51, Andrey Zhadchenko wrote:
>> 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>
>> ---
> ...
> 
>> +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);
> 
> Well, i agree that currently vhost_vq_init_access() can return -EFAULT only as an error,
> but may be it's still worth to write a more generic return ret code?

LGTM.
Original code was using this aprroach too, no idea why have I changed that.
Can you apply this in-place or do you want me to re-spin the series?

> 
>          int ret, i;
> 
>          for (i = 0; i < VHOST_BLK_VQ_MAX; i++) {
> ...
>                  mutex_lock(&vq->mutex);
>                  vhost_vq_set_backend(vq, blk->backend);
>                  ret = vhost_vq_init_access(vq);
>                  mutex_unlock(&vq->mutex);
>                  if (ret)
>                          return ret;
> 
> 
> 
>> +	}
>> +
>> +	return 0;
>>   }
>> ...