Linux Virtualization - Jun 2016 - [PATCH] virtio: Return correct errno for function init_vq's failure

The error number -ENOENT or 0 will be returned, if we can not allocate
more memory in function init_vq. If host can support multiple virtual
queues, and we fails to allocate necessary memory structures for vq,
kernel may crash due to incorrect returning.

To fix it, kernel will return correct value in init_vq.

Signed-off-by: Minfei Huang <mnghuan at gmail.com>
Signed-off-by: Minfei Huang <minfei.hmf at alibaba-inc.com>
---
 drivers/block/virtio_blk.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 42758b5..40ecb2b 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -393,11 +393,10 @@ static int init_vq(struct virtio_blk *vblk)
 	if (err)
 		num_vqs = 1;
 
+	err = -ENOMEM;
 	vblk->vqs = kmalloc(sizeof(*vblk->vqs) * num_vqs, GFP_KERNEL);
-	if (!vblk->vqs) {
-		err = -ENOMEM;
+	if (!vblk->vqs)
 		goto out;
-	}
 
 	names = kmalloc(sizeof(*names) * num_vqs, GFP_KERNEL);
 	if (!names)
-- 
2.7.4 (Apple Git-66)

On Mon, 27 Jun 2016 10:09:18 +0800
Minfei Huang <mnghuan at gmail.com> wrote:
> The error number -ENOENT or 0 will be returned, if we can not allocate
> more memory in function init_vq. If host can support multiple virtual
> queues, and we fails to allocate necessary memory structures for vq,
> kernel may crash due to incorrect returning.
> 
> To fix it, kernel will return correct value in init_vq.
> 
> Signed-off-by: Minfei Huang <mnghuan at gmail.com>
> Signed-off-by: Minfei Huang <minfei.hmf at alibaba-inc.com>
> ---
>  drivers/block/virtio_blk.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 42758b5..40ecb2b 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -393,11 +393,10 @@ static int init_vq(struct virtio_blk *vblk)
>  	if (err)
>  		num_vqs = 1;
> 
> +	err = -ENOMEM;
>  	vblk->vqs = kmalloc(sizeof(*vblk->vqs) * num_vqs, GFP_KERNEL);
> -	if (!vblk->vqs) {
> -		err = -ENOMEM;
> +	if (!vblk->vqs)
>  		goto out;
> -	}
> 
>  	names = kmalloc(sizeof(*names) * num_vqs, GFP_KERNEL);
>  	if (!names)
The error handling in this function looks horrible.

When mq was introduced, init_vq started mixing up several things:
- The mq feature is not available - which is not an error, and
therefore should not have any influence on the return code.
- One of the several memory allocations failed - only ->vqs gets
special treatment, however.
- The ->find_vqs callback failed.

Your patch fixes the code, but it is still very convoluted due to the
temporary arrays.

May it be worthwile to introduce a helper for setting up the virtqueues
where all virtqueues are essentially the same and just get a
consecutive number? Michael?

On 07/06/16 at 11:18P, Cornelia Huck wrote:
> On Mon, 27 Jun 2016 10:09:18 +0800
> Minfei Huang <mnghuan at gmail.com> wrote:
> 
> > The error number -ENOENT or 0 will be returned, if we can not allocate
> > more memory in function init_vq. If host can support multiple virtual
> > queues, and we fails to allocate necessary memory structures for vq,
> > kernel may crash due to incorrect returning.
> > 
> > To fix it, kernel will return correct value in init_vq.
> The error handling in this function looks horrible.
> 
> When mq was introduced, init_vq started mixing up several things:
> - The mq feature is not available - which is not an error, and
> therefore should not have any influence on the return code.
> - One of the several memory allocations failed - only ->vqs gets
> special treatment, however.
> - The ->find_vqs callback failed.
Yep. And without this patch, it is silent for boot failure. I think it
makes sense to let user notify about this failure.
> 
> Your patch fixes the code, but it is still very convoluted due to the
> temporary arrays.
> 
> May it be worthwile to introduce a helper for setting up the virtqueues
> where all virtqueues are essentially the same and just get a
> consecutive number? Michael?
> 
Hmm, How about refactor this function to make it more readable, since we
do a lot of work in it.

I will post an update to refactor this function.

Thanks
Minfei