[PATCH] virtio: Return correct errno for function init_vq's failure

[PATCH] virtio: Return correct errno for function init_vq's failure

[Minfei Huang]

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@gmail.com>
Signed-off-by: Minfei Huang <minfei.hmf@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)

Re: [PATCH] virtio: Return correct errno for function init_vq's failure

[Cornelia Huck]

On Mon, 27 Jun 2016 10:09:18 +0800
Minfei Huang <mnghuan@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@gmail.com>
> Signed-off-by: Minfei Huang <minfei.hmf@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?

Re: [PATCH] virtio: Return correct errno for function init_vq's failure

[Minfei Huang]

On 07/06/16 at 11:18P, Cornelia Huck wrote:
> On Mon, 27 Jun 2016 10:09:18 +0800
> Minfei Huang <mnghuan@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

Re: [PATCH] virtio: Return correct errno for function init_vq's failure

[Cornelia Huck]

On Wed, 13 Jul 2016 19:54:00 +0800
Minfei Huang <mnghuan@gmail.com> wrote:

> On 07/06/16 at 11:18P, Cornelia Huck wrote:
> > On Mon, 27 Jun 2016 10:09:18 +0800
> > Minfei Huang <mnghuan@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.

Agreed.

> 
> > 
> > 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.

Anything to make this more readable probably helps :)