[Qemu-devel] [PATCH 0/3] virtio-balloon: Several fixes to recent rework

[Qemu-devel] [PATCH 0/3] virtio-balloon: Several fixes to recent rework

[David Gibson]

A rework to the virtio-balloon driver was recently merged as
f6deb6d..ee1cd00.  This series addresses several minor issues in the
new code.

Changes since RFC:
 * Add a fix for the g_malloc/free mismatch reported by Coverity
 * Remove the not-really-useful hint-on-deflate flag

David Gibson (3):
  virtio-balloon: Don't mismatch g_malloc()/free (CID 1399146)
  virtio-balloon: Fix possible guest memory corruption with inflates &
    deflates
  virtio-balloon: Restore MADV_WILLNEED hint on balloon deflate

 hw/virtio/virtio-balloon.c | 65 +++++++++++++++++++++++++++++++++++---
 1 file changed, 61 insertions(+), 4 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH 1/3] virtio-balloon: Don't mismatch g_malloc()/free (CID 1399146)

[David Gibson]

ed48c59875b6 "virtio-balloon: Safely handle BALLOON_PAGE_SIZE < host
page size" introduced a new temporary data structure which tracks 4kiB
chunks which have been inserted into the balloon by the guest but
don't yet form a full host page which we can discard.

Unfortunately, I had a thinko and allocated that structure with
g_malloc0() but freed it with a plain free() rather than g_free().
This corrects the problem.

Fixes: ed48c59875b6
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/virtio/virtio-balloon.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index d3f2913a85..127289ae0e 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -81,7 +81,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 */
-        free(balloon->pbp);
+        g_free(balloon->pbp);
         balloon->pbp = NULL;
     }
 
@@ -106,7 +106,7 @@ static void balloon_inflate_page(VirtIOBalloon *balloon,
          * has already reported them, and failing to discard a balloon
          * page is not fatal */
 
-        free(balloon->pbp);
+        g_free(balloon->pbp);
         balloon->pbp = NULL;
     }
 }
-- 
2.20.1

Re: [Qemu-devel] [PATCH 1/3] virtio-balloon: Don't mismatch g_malloc()/free (CID 1399146)

[David Hildenbrand]

On 06.03.19 04:05, David Gibson wrote:
> ed48c59875b6 "virtio-balloon: Safely handle BALLOON_PAGE_SIZE < host
> page size" introduced a new temporary data structure which tracks 4kiB
> chunks which have been inserted into the balloon by the guest but
> don't yet form a full host page which we can discard.
> 
> Unfortunately, I had a thinko and allocated that structure with
> g_malloc0() but freed it with a plain free() rather than g_free().
> This corrects the problem.
> 
> Fixes: ed48c59875b6
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/virtio/virtio-balloon.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index d3f2913a85..127289ae0e 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -81,7 +81,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 */
> -        free(balloon->pbp);
> +        g_free(balloon->pbp);
>          balloon->pbp = NULL;
>      }
>  
> @@ -106,7 +106,7 @@ static void balloon_inflate_page(VirtIOBalloon *balloon,
>           * has already reported them, and failing to discard a balloon
>           * page is not fatal */
>  
> -        free(balloon->pbp);
> +        g_free(balloon->pbp);
>          balloon->pbp = NULL;
>      }
>  }
> 

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

-- 

Thanks,

David / dhildenb

[Qemu-devel] [PATCH 2/3] virtio-balloon: Fix possible guest memory corruption with inflates & deflates

[David Gibson]

This fixes a balloon bug with a nasty consequence - potentially
corrupting guest memory - but which is extremely unlikely to be
triggered in practice.

The balloon always works in 4kiB units, but the host could have a
larger page size on certain platforms.  Since ed48c59 "virtio-balloon:
Safely handle BALLOON_PAGE_SIZE < host page size" we've handled this
by accumulating requests to balloon 4kiB subpages until they formed a
full host page.  Since f6deb6d "virtio-balloon: Remove unnecessary
MADV_WILLNEED on deflate" we essentially ignore deflate requests.

Suppose we have a host with 8kiB pages, and one host page has subpages
A & B.  If we get this sequence of events -
	inflate A
	deflate A
	inflate B
- the current logic will discard the whole host page.  That's
incorrect because the guest has deflated subpage A, and could have
written important data to it.

This patch fixes the problem by adjusting our state information about
partially ballooned host pages when deflate requests are received.

Fixes: ed48c59 "virtio-balloon: Safely handle BALLOON_PAGE_SIZE < host page size"

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/virtio/virtio-balloon.c | 48 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 46 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 127289ae0e..7412bf8c85 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -111,6 +111,43 @@ static void balloon_inflate_page(VirtIOBalloon *balloon,
     }
 }
 
+static void balloon_deflate_page(VirtIOBalloon *balloon,
+                                 MemoryRegion *mr, hwaddr offset)
+{
+    void *addr = memory_region_get_ram_ptr(mr) + offset;
+    RAMBlock *rb;
+    size_t rb_page_size;
+    ram_addr_t ram_offset, host_page_base;
+
+    /* XXX is there a better way to get to the RAMBlock than via a
+     * host address? */
+    rb = qemu_ram_block_from_host(addr, false, &ram_offset);
+    rb_page_size = qemu_ram_pagesize(rb);
+    host_page_base = ram_offset & ~(rb_page_size - 1);
+
+    if (balloon->pbp
+        && rb == balloon->pbp->rb
+        && host_page_base == balloon->pbp->base) {
+        int subpages = rb_page_size / BALLOON_PAGE_SIZE;
+
+        /*
+         * This means the guest has asked to discard some of the 4kiB
+         * subpages of a host page, but then changed its mind and
+         * asked to keep them after all.  It's exceedingly unlikely
+         * for a guest to do this in practice, but handle it anyway,
+         * since getting it wrong could mean discarding memory the
+         * guest is still using. */
+        bitmap_clear(balloon->pbp->bitmap,
+                     (ram_offset - balloon->pbp->base) / BALLOON_PAGE_SIZE,
+                     subpages);
+
+        if (bitmap_empty(balloon->pbp->bitmap, subpages)) {
+            g_free(balloon->pbp);
+            balloon->pbp = NULL;
+        }
+    }
+}
+
 static const char *balloon_stat_names[] = {
    [VIRTIO_BALLOON_S_SWAP_IN] = "stat-swap-in",
    [VIRTIO_BALLOON_S_SWAP_OUT] = "stat-swap-out",
@@ -314,8 +351,15 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
 
             trace_virtio_balloon_handle_output(memory_region_name(section.mr),
                                                pa);
-            if (!qemu_balloon_is_inhibited() && vq != s->dvq) {
-                balloon_inflate_page(s, section.mr, section.offset_within_region);
+            if (!qemu_balloon_is_inhibited()) {
+                if (vq == s->ivq) {
+                    balloon_inflate_page(s, section.mr,
+                                         section.offset_within_region);
+                } else if (vq == s->dvq) {
+                    balloon_deflate_page(s, section.mr, section.offset_within_region);
+                } else {
+                    g_assert_not_reached();
+                }
             }
             memory_region_unref(section.mr);
         }
-- 
2.20.1

Re: [Qemu-devel] [PATCH 2/3] virtio-balloon: Fix possible guest memory corruption with inflates & deflates

[David Hildenbrand]

On 06.03.19 04:06, David Gibson wrote:
> This fixes a balloon bug with a nasty consequence - potentially
> corrupting guest memory - but which is extremely unlikely to be
> triggered in practice.
> 
> The balloon always works in 4kiB units, but the host could have a
> larger page size on certain platforms.  Since ed48c59 "virtio-balloon:
> Safely handle BALLOON_PAGE_SIZE < host page size" we've handled this
> by accumulating requests to balloon 4kiB subpages until they formed a
> full host page.  Since f6deb6d "virtio-balloon: Remove unnecessary
> MADV_WILLNEED on deflate" we essentially ignore deflate requests.
> 
> Suppose we have a host with 8kiB pages, and one host page has subpages
> A & B.  If we get this sequence of events -
> 	inflate A
> 	deflate A
> 	inflate B
> - the current logic will discard the whole host page.  That's
> incorrect because the guest has deflated subpage A, and could have
> written important data to it.
> 
> This patch fixes the problem by adjusting our state information about
> partially ballooned host pages when deflate requests are received.
> 
> Fixes: ed48c59 "virtio-balloon: Safely handle BALLOON_PAGE_SIZE < host page size"
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/virtio/virtio-balloon.c | 48 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 46 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 127289ae0e..7412bf8c85 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -111,6 +111,43 @@ static void balloon_inflate_page(VirtIOBalloon *balloon,
>      }
>  }
>  
> +static void balloon_deflate_page(VirtIOBalloon *balloon,
> +                                 MemoryRegion *mr, hwaddr offset)
> +{
> +    void *addr = memory_region_get_ram_ptr(mr) + offset;
> +    RAMBlock *rb;
> +    size_t rb_page_size;
> +    ram_addr_t ram_offset, host_page_base;
> +
> +    /* XXX is there a better way to get to the RAMBlock than via a
> +     * host address? */
> +    rb = qemu_ram_block_from_host(addr, false, &ram_offset);
> +    rb_page_size = qemu_ram_pagesize(rb);
> +    host_page_base = ram_offset & ~(rb_page_size - 1);
> +
> +    if (balloon->pbp
> +        && rb == balloon->pbp->rb
> +        && host_page_base == balloon->pbp->base) {
> +        int subpages = rb_page_size / BALLOON_PAGE_SIZE;
> +
> +        /*
> +         * This means the guest has asked to discard some of the 4kiB
> +         * subpages of a host page, but then changed its mind and
> +         * asked to keep them after all.  It's exceedingly unlikely
> +         * for a guest to do this in practice, but handle it anyway,
> +         * since getting it wrong could mean discarding memory the
> +         * guest is still using. */
> +        bitmap_clear(balloon->pbp->bitmap,
> +                     (ram_offset - balloon->pbp->base) / BALLOON_PAGE_SIZE,
> +                     subpages);
> +
> +        if (bitmap_empty(balloon->pbp->bitmap, subpages)) {
> +            g_free(balloon->pbp);
> +            balloon->pbp = NULL;
> +        }
> +    }
> +}
> +
>  static const char *balloon_stat_names[] = {
>     [VIRTIO_BALLOON_S_SWAP_IN] = "stat-swap-in",
>     [VIRTIO_BALLOON_S_SWAP_OUT] = "stat-swap-out",
> @@ -314,8 +351,15 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
>  
>              trace_virtio_balloon_handle_output(memory_region_name(section.mr),
>                                                 pa);
> -            if (!qemu_balloon_is_inhibited() && vq != s->dvq) {
> -                balloon_inflate_page(s, section.mr, section.offset_within_region);
> +            if (!qemu_balloon_is_inhibited()) {
> +                if (vq == s->ivq) {
> +                    balloon_inflate_page(s, section.mr,
> +                                         section.offset_within_region);
> +                } else if (vq == s->dvq) {
> +                    balloon_deflate_page(s, section.mr, section.offset_within_region);
> +                } else {
> +                    g_assert_not_reached();
> +                }
>              }
>              memory_region_unref(section.mr);
>          }
> 

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

-- 

Thanks,

David / dhildenb

[Qemu-devel] [PATCH 3/3] virtio-balloon: Restore MADV_WILLNEED hint on balloon deflate

[David Gibson]

Prior to f6deb6d9 "virtio-balloon: Remove unnecessary MADV_WILLNEED on
deflate", the balloon device issued an madvise() MADV_WILLNEED on
pages removed from the balloon.  That would hint to the host kernel
that the pages were likely to be needed by the guest in the near
future.

It's unclear if this is actually valuable or not, and so f6deb6d9
removed this, essentially ignoring balloon deflate requests.  However,
concerns have been raised that this might cause a performance
regression by causing extra latency for the guest in certain
configurations.

So, until we can get actual benchmark data to see if that's the case,
this restores the old behaviour, issuing a MADV_WILLNEED when a page is
removed from the balloon.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/virtio/virtio-balloon.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 7412bf8c85..ac36988605 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -118,6 +118,8 @@ static void balloon_deflate_page(VirtIOBalloon *balloon,
     RAMBlock *rb;
     size_t rb_page_size;
     ram_addr_t ram_offset, host_page_base;
+    void *host_addr;
+    int ret;
 
     /* XXX is there a better way to get to the RAMBlock than via a
      * host address? */
@@ -146,6 +148,17 @@ static void balloon_deflate_page(VirtIOBalloon *balloon,
             balloon->pbp = NULL;
         }
     }
+
+    host_addr = (void *)((uintptr_t)addr & ~(rb_page_size - 1));
+
+    /* When a page is deflated, we hint the whole host page it lives
+     * on, since we can't do anything smaller */
+    ret = qemu_madvise(host_addr, rb_page_size, QEMU_MADV_WILLNEED);
+    if (ret != 0) {
+        warn_report("Couldn't MADV_WILLNEED on balloon deflate: %s",
+                    strerror(errno));
+        /* Otherwise ignore, failing to page hint shouldn't be fatal */
+    }
 }
 
 static const char *balloon_stat_names[] = {
-- 
2.20.1

Re: [Qemu-devel] [PATCH 3/3] virtio-balloon: Restore MADV_WILLNEED hint on balloon deflate

[David Hildenbrand]

On 06.03.19 04:06, David Gibson wrote:
> Prior to f6deb6d9 "virtio-balloon: Remove unnecessary MADV_WILLNEED on
> deflate", the balloon device issued an madvise() MADV_WILLNEED on
> pages removed from the balloon.  That would hint to the host kernel
> that the pages were likely to be needed by the guest in the near
> future.
> 
> It's unclear if this is actually valuable or not, and so f6deb6d9
> removed this, essentially ignoring balloon deflate requests.  However,
> concerns have been raised that this might cause a performance
> regression by causing extra latency for the guest in certain
> configurations.
> 
> So, until we can get actual benchmark data to see if that's the case,
> this restores the old behaviour, issuing a MADV_WILLNEED when a page is
> removed from the balloon.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/virtio/virtio-balloon.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 7412bf8c85..ac36988605 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -118,6 +118,8 @@ static void balloon_deflate_page(VirtIOBalloon *balloon,
>      RAMBlock *rb;
>      size_t rb_page_size;
>      ram_addr_t ram_offset, host_page_base;
> +    void *host_addr;
> +    int ret;
>  
>      /* XXX is there a better way to get to the RAMBlock than via a
>       * host address? */
> @@ -146,6 +148,17 @@ static void balloon_deflate_page(VirtIOBalloon *balloon,
>              balloon->pbp = NULL;
>          }
>      }
> +
> +    host_addr = (void *)((uintptr_t)addr & ~(rb_page_size - 1));
> +
> +    /* When a page is deflated, we hint the whole host page it lives
> +     * on, since we can't do anything smaller */
> +    ret = qemu_madvise(host_addr, rb_page_size, QEMU_MADV_WILLNEED);
> +    if (ret != 0) {
> +        warn_report("Couldn't MADV_WILLNEED on balloon deflate: %s",
> +                    strerror(errno));
> +        /* Otherwise ignore, failing to page hint shouldn't be fatal */
> +    }

As far as I understand the man page, only MADV_DONTNEED will complain
about locked, huge or VM_PFNMAP pages. So I guess MADV_WILLNEED will
work for these cases (in the worst case be a NOP).

-- 

Thanks,

David / dhildenb