Minfei Huang
2016-Jun-27 02:09 UTC
[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)
Cornelia Huck
2016-Jul-06 09:18 UTC
[PATCH] virtio: Return correct errno for function init_vq's failure
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?
Minfei Huang
2016-Jul-13 11:54 UTC
[PATCH] virtio: Return correct errno for function init_vq's failure
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
Reasonably Related Threads
- [PATCH] virtio: Return correct errno for function init_vq's failure
- [PATCH v2] virtio_blk: Fix a slient kernel panic
- [PATCH v2] virtio_blk: Fix a slient kernel panic
- [PATCH v3] virtio_blk: Fix a slient kernel panic
- [PATCH v3] virtio_blk: Fix a slient kernel panic