[Qemu-devel] [PATCH 0/4] Fix virtio memleaks

[Qemu-devel] [PATCH 0/4] Fix virtio memleaks

[Amit Shah]

The memory allocated in virtio_common_init() wasn't being freed
anywhere.  Fix that.

The balloon handler wasn't unregistering its savevm section,
adding an exit handler fixes that as well.

This patchset is on top of the two balloon series I've sent out
yesterday and today.

Amit Shah (4):
  virtio-balloon: Add exit handler, fix memleaks
  virtio-blk: Fix memleak on exit
  virtio-net: Fix potential use-after-free
  virtio: Plug memleak by freeing vdev

 hw/virtio-balloon.c |    9 +++++++++
 hw/virtio-blk.c     |    1 +
 hw/virtio-net.c     |    2 +-
 hw/virtio-pci.c     |   11 ++++++++++-
 hw/virtio.c         |    1 +
 hw/virtio.h         |    1 +
 6 files changed, 23 insertions(+), 2 deletions(-)

-- 
1.7.6

[Qemu-devel] [PATCH 1/4] virtio-balloon: Add exit handler, fix memleaks

[Amit Shah]

Add an exit handler that will free up RAM and unregister the savevm
section after a virtio-balloon device is unplugged.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-balloon.c |    9 +++++++++
 hw/virtio-pci.c     |   11 ++++++++++-
 hw/virtio.h         |    1 +
 3 files changed, 20 insertions(+), 1 deletions(-)

diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
index 304a31a..054454b 100644
--- a/hw/virtio-balloon.c
+++ b/hw/virtio-balloon.c
@@ -45,6 +45,7 @@ typedef struct VirtIOBalloon
     size_t stats_vq_offset;
     MonitorCompletion *stats_callback;
     void *stats_opaque_callback_data;
+    DeviceState *qdev;
 } VirtIOBalloon;
 
 static VirtIOBalloon *to_virtio_balloon(VirtIODevice *vdev)
@@ -293,8 +294,16 @@ VirtIODevice *virtio_balloon_init(DeviceState *dev)
 
     reset_stats(s);
 
+    s->qdev = dev;
     register_savevm(dev, "virtio-balloon", -1, 1,
                     virtio_balloon_save, virtio_balloon_load, s);
 
     return &s->vdev;
 }
+
+void virtio_balloon_exit(VirtIODevice *vdev)
+{
+    VirtIOBalloon *s = DO_UPCAST(VirtIOBalloon, vdev, vdev);
+    unregister_savevm(s->qdev, "virtio-balloon", s);
+    virtio_cleanup(vdev);
+}
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index ca5f125..316bf92 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -795,6 +795,15 @@ static int virtio_balloon_init_pci(PCIDevice *pci_dev)
     return 0;
 }
 
+static int virtio_balloon_exit_pci(PCIDevice *pci_dev)
+{
+    VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
+
+    virtio_pci_stop_ioeventfd(proxy);
+    virtio_balloon_exit(proxy->vdev);
+    return virtio_exit_pci(pci_dev);
+}
+
 static PCIDeviceInfo virtio_info[] = {
     {
         .qdev.name = "virtio-blk-pci",
@@ -869,7 +878,7 @@ static PCIDeviceInfo virtio_info[] = {
         .qdev.alias = "virtio-balloon",
         .qdev.size = sizeof(VirtIOPCIProxy),
         .init      = virtio_balloon_init_pci,
-        .exit      = virtio_exit_pci,
+        .exit      = virtio_balloon_exit_pci,
         .vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET,
         .device_id = PCI_DEVICE_ID_VIRTIO_BALLOON,
         .revision  = VIRTIO_PCI_ABI_VERSION,
diff --git a/hw/virtio.h b/hw/virtio.h
index 0fd0bb0..c129264 100644
--- a/hw/virtio.h
+++ b/hw/virtio.h
@@ -213,6 +213,7 @@ VirtIODevice *virtio_9p_init(DeviceState *dev, V9fsConf *conf);
 void virtio_net_exit(VirtIODevice *vdev);
 void virtio_blk_exit(VirtIODevice *vdev);
 void virtio_serial_exit(VirtIODevice *vdev);
+void virtio_balloon_exit(VirtIODevice *vdev);
 
 #define DEFINE_VIRTIO_COMMON_FEATURES(_state, _field) \
 	DEFINE_PROP_BIT("indirect_desc", _state, _field, \
-- 
1.7.6

[Qemu-devel] [PATCH 2/4] virtio-blk: Fix memleak on exit

[Amit Shah]

Calling virtio_cleanup() will free up memory allocated in
virtio_common_init().

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-blk.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index 6471ac8..836dbc3 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -594,4 +594,5 @@ void virtio_blk_exit(VirtIODevice *vdev)
 {
     VirtIOBlock *s = to_virtio_blk(vdev);
     unregister_savevm(s->qdev, "virtio-blk", s);
+    virtio_cleanup(vdev);
 }
-- 
1.7.6

Re: [Qemu-devel] [PATCH 2/4] virtio-blk: Fix memleak on exit

[Kevin Wolf]

Am 27.07.2011 10:30, schrieb Amit Shah:
> Calling virtio_cleanup() will free up memory allocated in
> virtio_common_init().
> 
> Signed-off-by: Amit Shah <amit.shah@redhat.com>

Acked-by: Kevin Wolf <kwolf@redhat.com>

[Qemu-devel] [PATCH 3/4] virtio-net: Fix potential use-after-free

[Amit Shah]

virtio_cleanup() will remove the VirtIONet struct that gets allocated
via virtio_common_init().  Ensure we don't dereference the structure
after calling the cleanup function.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-net.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index a32cc01..3f10391 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -1073,6 +1073,6 @@ void virtio_net_exit(VirtIODevice *vdev)
         qemu_bh_delete(n->tx_bh);
     }
 
-    virtio_cleanup(&n->vdev);
     qemu_del_vlan_client(&n->nic->nc);
+    virtio_cleanup(&n->vdev);
 }
-- 
1.7.6

Re: [Qemu-devel] [PATCH 3/4] virtio-net: Fix potential use-after-free

[Michael S. Tsirkin]

On Wed, Jul 27, 2011 at 02:00:31PM +0530, Amit Shah wrote:
> virtio_cleanup() will remove the VirtIONet struct that gets allocated
> via virtio_common_init().  Ensure we don't dereference the structure
> after calling the cleanup function.
> 
> Signed-off-by: Amit Shah <amit.shah@redhat.com>

I see. It's not a use after free but will be once
you make virtio_cleanup free the vdev?

> ---
>  hw/virtio-net.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> index a32cc01..3f10391 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -1073,6 +1073,6 @@ void virtio_net_exit(VirtIODevice *vdev)
>          qemu_bh_delete(n->tx_bh);
>      }
>  
> -    virtio_cleanup(&n->vdev);
>      qemu_del_vlan_client(&n->nic->nc);
> +    virtio_cleanup(&n->vdev);
>  }
> -- 
> 1.7.6

Re: [Qemu-devel] [PATCH 3/4] virtio-net: Fix potential use-after-free

[Amit Shah]

On (Wed) 27 Jul 2011 [11:43:44], Michael S. Tsirkin wrote:
> On Wed, Jul 27, 2011 at 02:00:31PM +0530, Amit Shah wrote:
> > virtio_cleanup() will remove the VirtIONet struct that gets allocated
> > via virtio_common_init().  Ensure we don't dereference the structure
> > after calling the cleanup function.
> > 
> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> 
> I see. It's not a use after free but will be once
> you make virtio_cleanup free the vdev?

Yes, the next patch.

		Amit

[Qemu-devel] [PATCH 4/4] virtio: Plug memleak by freeing vdev

[Amit Shah]

virtio_common_init() allocates RAM for the vdev struct (and any
additional memory, depending on the size passed to the function).  This
memory wasn't being freed until now.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/hw/virtio.c b/hw/virtio.c
index a8f4940..93dfb1e 100644
--- a/hw/virtio.c
+++ b/hw/virtio.c
@@ -834,6 +834,7 @@ void virtio_cleanup(VirtIODevice *vdev)
     if (vdev->config)
         qemu_free(vdev->config);
     qemu_free(vdev->vq);
+    qemu_free(vdev);
 }
 
 static void virtio_vmstate_change(void *opaque, int running, int reason)
-- 
1.7.6

Re: [Qemu-devel] [PATCH 0/4] Fix virtio memleaks

[Michael S. Tsirkin]

On Wed, Jul 27, 2011 at 02:00:28PM +0530, Amit Shah wrote:
> The memory allocated in virtio_common_init() wasn't being freed
> anywhere.  Fix that.
> 
> The balloon handler wasn't unregistering its savevm section,
> adding an exit handler fixes that as well.
> 
> This patchset is on top of the two balloon series I've sent out
> yesterday and today.

This looks good to me. What do you say I put
patches 2-4 on my tree, and you put patch 1 on yours?

> Amit Shah (4):
>   virtio-balloon: Add exit handler, fix memleaks
>   virtio-blk: Fix memleak on exit
>   virtio-net: Fix potential use-after-free
>   virtio: Plug memleak by freeing vdev
> 
>  hw/virtio-balloon.c |    9 +++++++++
>  hw/virtio-blk.c     |    1 +
>  hw/virtio-net.c     |    2 +-
>  hw/virtio-pci.c     |   11 ++++++++++-
>  hw/virtio.c         |    1 +
>  hw/virtio.h         |    1 +
>  6 files changed, 23 insertions(+), 2 deletions(-)
> 
> -- 
> 1.7.6
> 

Re: [Qemu-devel] [PATCH 0/4] Fix virtio memleaks

[Amit Shah]

On (Wed) 27 Jul 2011 [12:08:52], Michael S. Tsirkin wrote:
> On Wed, Jul 27, 2011 at 02:00:28PM +0530, Amit Shah wrote:
> > The memory allocated in virtio_common_init() wasn't being freed
> > anywhere.  Fix that.
> > 
> > The balloon handler wasn't unregistering its savevm section,
> > adding an exit handler fixes that as well.
> > 
> > This patchset is on top of the two balloon series I've sent out
> > yesterday and today.
> 
> This looks good to me. What do you say I put
> patches 2-4 on my tree, and you put patch 1 on yours?

Sure.

		Amit