[Qemu-devel] [PATCH] virtio-balloon: free pbp more aggressively

[Qemu-devel] [PATCH] virtio-balloon: free pbp more aggressively

[Michael S. Tsirkin]

Previous patches switched to a temporary pbp but that does not go far
enough: after device uses a buffer, guest is free to reuse it, so
tracking the page and freeing it later is wrong.

Free and reset the pbp after we push each element.

Fixes: ed48c59875b6 ("virtio-balloon: Safely handle BALLOON_PAGE_SIZE < host page size")
Cc: qemu-stable@nongnu.org #v4.0.0
Cc: David Hildenbrand <david@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/virtio/virtio-balloon.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index fe9664e42c..460a702463 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -39,13 +39,14 @@ typedef struct PartiallyBalloonedPage {
     unsigned long *bitmap;
 } PartiallyBalloonedPage;
 
-static void virtio_balloon_pbp_free(PartiallyBalloonedPage *pbp)
+static void virtio_balloon_pbp_free(PartiallyBalloonedPage **pbp)
 {
-    if (!pbp) {
+    if (!*pbp) {
         return;
     }
-    g_free(pbp->bitmap);
-    g_free(pbp);
+    g_free(*pbp->bitmap);
+    g_free(*pbp);
+    *pbp = NULL;
 }
 
 static PartiallyBalloonedPage *virtio_balloon_pbp_alloc(ram_addr_t base_gpa,
@@ -108,8 +109,7 @@ static void balloon_inflate_page(VirtIOBalloon *balloon,
         /* We've partially ballooned part of a host page, but now
          * we're trying to balloon part of a different one.  Too hard,
          * give up on the old partial page */
-        virtio_balloon_pbp_free(*pbp);
-        *pbp = NULL;
+        virtio_balloon_pbp_free(pbp);
     }
 
     if (!*pbp) {
@@ -127,8 +127,7 @@ static void balloon_inflate_page(VirtIOBalloon *balloon,
         /* We ignore errors from ram_block_discard_range(), because it
          * has already reported them, and failing to discard a balloon
          * page is not fatal */
-        virtio_balloon_pbp_free(*pbp);
-        *pbp = NULL;
+        virtio_balloon_pbp_free(pbp);
     }
 }
 
@@ -379,9 +378,8 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
         virtqueue_push(vq, elem, offset);
         virtio_notify(vdev, vq);
         g_free(elem);
+        virtio_balloon_pbp_free(&pbp);
     }
-
-    virtio_balloon_pbp_free(pbp);
 }
 
 static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq)
-- 
MST

Re: [Qemu-devel] [PATCH] virtio-balloon: free pbp more aggressively

[David Hildenbrand]

> -static void virtio_balloon_pbp_free(PartiallyBalloonedPage *pbp)
> +static void virtio_balloon_pbp_free(PartiallyBalloonedPage **pbp)
>  {
> -    if (!pbp) {
> +    if (!*pbp) {
>          return;
>      }
> -    g_free(pbp->bitmap);
> -    g_free(pbp);
> +    g_free(*pbp->bitmap);

This has to be

g_free((*pbp)->bitmap);

to compile.

> +    g_free(*pbp);
> +    *pbp = NULL;
>  }
>  
>  static PartiallyBalloonedPage *virtio_balloon_pbp_alloc(ram_addr_t base_gpa,
> @@ -108,8 +109,7 @@ static void balloon_inflate_page(VirtIOBalloon *balloon,
>          /* We've partially ballooned part of a host page, but now
>           * we're trying to balloon part of a different one.  Too hard,
>           * give up on the old partial page */
> -        virtio_balloon_pbp_free(*pbp);
> -        *pbp = NULL;
> +        virtio_balloon_pbp_free(pbp);
>      }
>  
>      if (!*pbp) {
> @@ -127,8 +127,7 @@ static void balloon_inflate_page(VirtIOBalloon *balloon,
>          /* We ignore errors from ram_block_discard_range(), because it
>           * has already reported them, and failing to discard a balloon
>           * page is not fatal */
> -        virtio_balloon_pbp_free(*pbp);
> -        *pbp = NULL;
> +        virtio_balloon_pbp_free(pbp);
>      }
>  }
>  
> @@ -379,9 +378,8 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
>          virtqueue_push(vq, elem, offset);
>          virtio_notify(vdev, vq);
>          g_free(elem);
> +        virtio_balloon_pbp_free(&pbp);

You could move the "PartiallyBalloonedPage *pbp = NULL;" now into the loop.

>      }
> -
> -    virtio_balloon_pbp_free(pbp);
>  }
>  
>  static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq)
> 

Gave it a quick test with a hugepage backing on x86 - still works. Would
be good if somebody could test with 64k PPC guest.

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David / dhildenb