From: Minfei Huang <mnghuan at gmail.com>
We do a lot of memory allocation in function init_vq, and don't handle
the allocation failure properly. Then this function will return 0,
although initialization fails due to lacking memory. At that moment,
kernel will panic in guest machine, if virtio is used to drive disk.
To fix this bug, we should take care of allocation failure, and return
correct value to let caller know what happen.
Tested-by: Chao Fan <fanc.fnst at cn.fujitsu.com>
Signed-off-by: Minfei Huang <minfei.hmf at alibaba-inc.com>
Signed-off-by: Minfei Huang <mnghuan at gmail.com>
---
v2:
- Remove useless initialisation to NULL
v1:
- Refactor the patch to make code more readable
---
drivers/block/virtio_blk.c | 26 ++++++++------------------
1 file changed, 8 insertions(+), 18 deletions(-)
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 42758b5..4ee78c0 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -394,22 +394,16 @@ static int init_vq(struct virtio_blk *vblk)
num_vqs = 1;
vblk->vqs = kmalloc(sizeof(*vblk->vqs) * num_vqs, GFP_KERNEL);
- if (!vblk->vqs) {
- err = -ENOMEM;
- goto out;
- }
+ if (!vblk->vqs)
+ return -ENOMEM;
names = kmalloc(sizeof(*names) * num_vqs, GFP_KERNEL);
- if (!names)
- goto err_names;
-
callbacks = kmalloc(sizeof(*callbacks) * num_vqs, GFP_KERNEL);
- if (!callbacks)
- goto err_callbacks;
-
vqs = kmalloc(sizeof(*vqs) * num_vqs, GFP_KERNEL);
- if (!vqs)
- goto err_vqs;
+ if (!names || !callbacks || !vqs) {
+ err = -ENOMEM;
+ goto out;
+ }
for (i = 0; i < num_vqs; i++) {
callbacks[i] = virtblk_done;
@@ -420,7 +414,7 @@ static int init_vq(struct virtio_blk *vblk)
/* Discover virtqueues and write information to configuration. */
err = vdev->config->find_vqs(vdev, num_vqs, vqs, callbacks, names);
if (err)
- goto err_find_vqs;
+ goto out;
for (i = 0; i < num_vqs; i++) {
spin_lock_init(&vblk->vqs[i].lock);
@@ -428,16 +422,12 @@ static int init_vq(struct virtio_blk *vblk)
}
vblk->num_vqs = num_vqs;
- err_find_vqs:
+out:
kfree(vqs);
- err_vqs:
kfree(callbacks);
- err_callbacks:
kfree(names);
- err_names:
if (err)
kfree(vblk->vqs);
- out:
return err;
}
--
2.7.4 (Apple Git-66)
On Tue, 19 Jul 2016 12:32:42 +0800 Minfei Huang <mnfhuang at gmail.com> wrote:> From: Minfei Huang <mnghuan at gmail.com> > > We do a lot of memory allocation in function init_vq, and don't handle > the allocation failure properly. Then this function will return 0, > although initialization fails due to lacking memory. At that moment, > kernel will panic in guest machine, if virtio is used to drive disk. > > To fix this bug, we should take care of allocation failure, and return > correct value to let caller know what happen. > > Tested-by: Chao Fan <fanc.fnst at cn.fujitsu.com> > Signed-off-by: Minfei Huang <minfei.hmf at alibaba-inc.com> > Signed-off-by: Minfei Huang <mnghuan at gmail.com> > --- > v2: > - Remove useless initialisation to NULL > v1: > - Refactor the patch to make code more readable > --- > drivers/block/virtio_blk.c | 26 ++++++++------------------ > 1 file changed, 8 insertions(+), 18 deletions(-)Your changes certainly make the function more compact. Reviewed-by: Cornelia Huck <cornelia.huck at de.ibm.com>
Ping, Any comment is appreciate. Thanks Minfei> On Jul 19, 2016, at 20:22, Cornelia Huck <cornelia.huck at de.ibm.com> wrote: > > On Tue, 19 Jul 2016 12:32:42 +0800 > Minfei Huang <mnfhuang at gmail.com> wrote: > >> From: Minfei Huang <mnghuan at gmail.com> >> >> We do a lot of memory allocation in function init_vq, and don't handle >> the allocation failure properly. Then this function will return 0, >> although initialization fails due to lacking memory. At that moment, >> kernel will panic in guest machine, if virtio is used to drive disk. >> >> To fix this bug, we should take care of allocation failure, and return >> correct value to let caller know what happen. >> >> Tested-by: Chao Fan <fanc.fnst at cn.fujitsu.com> >> Signed-off-by: Minfei Huang <minfei.hmf at alibaba-inc.com> >> Signed-off-by: Minfei Huang <mnghuan at gmail.com> >> --- >> v2: >> - Remove useless initialisation to NULL >> v1: >> - Refactor the patch to make code more readable >> --- >> drivers/block/virtio_blk.c | 26 ++++++++------------------ >> 1 file changed, 8 insertions(+), 18 deletions(-) > > Your changes certainly make the function more compact. > > Reviewed-by: Cornelia Huck <cornelia.huck at de.ibm.com>-------------- next part -------------- A non-text attachment was scrubbed... Name: smime.p7s Type: application/pkcs7-signature Size: 2353 bytes Desc: not available URL: <http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20160723/e45cbac1/attachment.p7s>
On Tue, Jul 19, 2016 at 5:32 AM, Minfei Huang <mnfhuang at gmail.com> wrote:> From: Minfei Huang <mnghuan at gmail.com> > > We do a lot of memory allocation in function init_vq, and don't handle > the allocation failure properly. Then this function will return 0, > although initialization fails due to lacking memory. At that moment, > kernel will panic in guest machine, if virtio is used to drive disk. > > To fix this bug, we should take care of allocation failure, and return > correct value to let caller know what happen. > > Tested-by: Chao Fan <fanc.fnst at cn.fujitsu.com> > Signed-off-by: Minfei Huang <minfei.hmf at alibaba-inc.com> > Signed-off-by: Minfei Huang <mnghuan at gmail.com> > --- > v2: > - Remove useless initialisation to NULL > v1: > - Refactor the patch to make code more readable > --- > drivers/block/virtio_blk.c | 26 ++++++++------------------ > 1 file changed, 8 insertions(+), 18 deletions(-)Reviewed-by: Stefan Hajnoczi <stefanha at redhat.com>
Hi, Michael. Since Stefan and Cornelia have review-acked this patch, could you mind helping review this patch? Thanks Minfei> On Jul 29, 2016, at 16:26, Stefan Hajnoczi <stefanha at gmail.com> wrote: > > On Tue, Jul 19, 2016 at 5:32 AM, Minfei Huang <mnfhuang at gmail.com> wrote: >> From: Minfei Huang <mnghuan at gmail.com> >> >> We do a lot of memory allocation in function init_vq, and don't handle >> the allocation failure properly. Then this function will return 0, >> although initialization fails due to lacking memory. At that moment, >> kernel will panic in guest machine, if virtio is used to drive disk. >> >> To fix this bug, we should take care of allocation failure, and return >> correct value to let caller know what happen. >> >> Tested-by: Chao Fan <fanc.fnst at cn.fujitsu.com> >> Signed-off-by: Minfei Huang <minfei.hmf at alibaba-inc.com> >> Signed-off-by: Minfei Huang <mnghuan at gmail.com> >> --- >> v2: >> - Remove useless initialisation to NULL >> v1: >> - Refactor the patch to make code more readable >> --- >> drivers/block/virtio_blk.c | 26 ++++++++------------------ >> 1 file changed, 8 insertions(+), 18 deletions(-) > > Reviewed-by: Stefan Hajnoczi <stefanha at redhat.com>-------------- next part -------------- A non-text attachment was scrubbed... Name: smime.p7s Type: application/pkcs7-signature Size: 2353 bytes Desc: not available URL: <http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20160804/2a3bdac1/attachment.p7s>