[Qemu-devel] [PATCH-for-4.1 v2 0/3] virtio-balloon: fixes for PartialBalloonedPage

[Qemu-devel] [PATCH-for-4.1 v2 0/3] virtio-balloon: fixes for PartialBalloonedPage

[David Hildenbrand]

Some v4.1 (also v4.0 stable) fixes for virtio-balloon that only trigger
when the pagesize is > BALLOON_PAGE_SIZE (4k).

v1 -> v2:
- Added "virtio-balloon: fix memory leak on unrealize()"
- Added "virtio-balloon: reset pbp on device resets"

Cc: Stefan Hajnoczi <stefanha@redhat.com>
Cc: David Gibson <david@gibson.dropbear.id.au>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>

David Hildenbrand (3):
  virtio-balloon: fix QEMU crashes on pagesize > BALLOON_PAGE_SIZE
  virtio-balloon: fix memory leak on unrealize()
  virtio-balloon: reset pbp on device resets

 hw/virtio/virtio-balloon.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

-- 
2.21.0

[Qemu-devel] [PATCH-for-4.1 v2 1/3] virtio-balloon: fix QEMU crashes on pagesize > BALLOON_PAGE_SIZE

[David Hildenbrand]

We are using the wrong functions to set/clear bits, effectively touching
multiple bits, writing out of range of the bitmap, resulting in memory
corruptions. We have to use set_bit()/clear_bit() instead.

Can easily be reproduced by starting a qemu guest on hugetlbfs memory,
inflating the balloon. QEMU crashes. This never could have worked
properly - especially, also pages would have been discarded when the
first sub-page would be inflated (the whole bitmap would be set).

While testing I realized, that on hugetlbfs it is pretty much impossible
to discard a page - the guest just frees the 4k sub-pages in random order
most of the time. I was only able to discard a hugepage a handful of
times - so I hope that now works correctly.

Fixes: ed48c59875b6 ("virtio-balloon: Safely handle BALLOON_PAGE_SIZE <
                     host page size")
Fixes: b27b32391404 ("virtio-balloon: Fix possible guest memory corruption
                     with inflates & deflates")
Cc: qemu-stable@nongnu.org #v4.0.0
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Cc: David Gibson <david@gibson.dropbear.id.au>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/virtio/virtio-balloon.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index e85d1c0d5c..669067d661 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -94,9 +94,8 @@ static void balloon_inflate_page(VirtIOBalloon *balloon,
         balloon->pbp->base = host_page_base;
     }
 
-    bitmap_set(balloon->pbp->bitmap,
-               (ram_offset - balloon->pbp->base) / BALLOON_PAGE_SIZE,
-               subpages);
+    set_bit((ram_offset - balloon->pbp->base) / BALLOON_PAGE_SIZE,
+            balloon->pbp->bitmap);
 
     if (bitmap_full(balloon->pbp->bitmap, subpages)) {
         /* We've accumulated a full host page, we can actually discard
@@ -140,9 +139,8 @@ static void balloon_deflate_page(VirtIOBalloon *balloon,
          * 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);
+        clear_bit((ram_offset - balloon->pbp->base) / BALLOON_PAGE_SIZE,
+                  balloon->pbp->bitmap);
 
         if (bitmap_empty(balloon->pbp->bitmap, subpages)) {
             g_free(balloon->pbp);
-- 
2.21.0

Re: [Qemu-devel] [PATCH-for-4.1 v2 1/3] virtio-balloon: fix QEMU crashes on pagesize > BALLOON_PAGE_SIZE

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3157 bytes --]

On Wed, Jul 17, 2019 at 12:35:48PM +0200, David Hildenbrand wrote:
> We are using the wrong functions to set/clear bits, effectively touching
> multiple bits, writing out of range of the bitmap, resulting in memory
> corruptions. We have to use set_bit()/clear_bit() instead.
> 
> Can easily be reproduced by starting a qemu guest on hugetlbfs memory,
> inflating the balloon. QEMU crashes. This never could have worked
> properly - especially, also pages would have been discarded when the
> first sub-page would be inflated (the whole bitmap would be set).
> 
> While testing I realized, that on hugetlbfs it is pretty much impossible
> to discard a page - the guest just frees the 4k sub-pages in random order
> most of the time. I was only able to discard a hugepage a handful of
> times - so I hope that now works correctly.
> 
> Fixes: ed48c59875b6 ("virtio-balloon: Safely handle BALLOON_PAGE_SIZE <
>                      host page size")
> Fixes: b27b32391404 ("virtio-balloon: Fix possible guest memory corruption
>                      with inflates & deflates")
> Cc: qemu-stable@nongnu.org #v4.0.0
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>

Ahem.  You can pass me the brown paper bag now.

Acked-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/virtio/virtio-balloon.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index e85d1c0d5c..669067d661 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -94,9 +94,8 @@ static void balloon_inflate_page(VirtIOBalloon *balloon,
>          balloon->pbp->base = host_page_base;
>      }
>  
> -    bitmap_set(balloon->pbp->bitmap,
> -               (ram_offset - balloon->pbp->base) / BALLOON_PAGE_SIZE,
> -               subpages);
> +    set_bit((ram_offset - balloon->pbp->base) / BALLOON_PAGE_SIZE,
> +            balloon->pbp->bitmap);
>  
>      if (bitmap_full(balloon->pbp->bitmap, subpages)) {
>          /* We've accumulated a full host page, we can actually discard
> @@ -140,9 +139,8 @@ static void balloon_deflate_page(VirtIOBalloon *balloon,
>           * 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);
> +        clear_bit((ram_offset - balloon->pbp->base) / BALLOON_PAGE_SIZE,
> +                  balloon->pbp->bitmap);
>  
>          if (bitmap_empty(balloon->pbp->bitmap, subpages)) {
>              g_free(balloon->pbp);

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH-for-4.1 v2 1/3] virtio-balloon: fix QEMU crashes on pagesize > BALLOON_PAGE_SIZE

[David Hildenbrand]

On 18.07.19 05:50, David Gibson wrote:
> On Wed, Jul 17, 2019 at 12:35:48PM +0200, David Hildenbrand wrote:
>> We are using the wrong functions to set/clear bits, effectively touching
>> multiple bits, writing out of range of the bitmap, resulting in memory
>> corruptions. We have to use set_bit()/clear_bit() instead.
>>
>> Can easily be reproduced by starting a qemu guest on hugetlbfs memory,
>> inflating the balloon. QEMU crashes. This never could have worked
>> properly - especially, also pages would have been discarded when the
>> first sub-page would be inflated (the whole bitmap would be set).
>>
>> While testing I realized, that on hugetlbfs it is pretty much impossible
>> to discard a page - the guest just frees the 4k sub-pages in random order
>> most of the time. I was only able to discard a hugepage a handful of
>> times - so I hope that now works correctly.
>>
>> Fixes: ed48c59875b6 ("virtio-balloon: Safely handle BALLOON_PAGE_SIZE <
>>                      host page size")
>> Fixes: b27b32391404 ("virtio-balloon: Fix possible guest memory corruption
>>                      with inflates & deflates")
>> Cc: qemu-stable@nongnu.org #v4.0.0
>> Cc: Stefan Hajnoczi <stefanha@redhat.com>
>> Cc: David Gibson <david@gibson.dropbear.id.au>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Cc: Igor Mammedov <imammedo@redhat.com>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
> 
> Ahem.  You can pass me the brown paper bag now.
> 

No worries, BUGs are inevitable.

Thanks!

-- 

Thanks,

David / dhildenb

[Qemu-devel] [PATCH-for-4.1 v2 2/3] virtio-balloon: fix memory leak on unrealize()

[David Hildenbrand]

We could have tracking data for a pbp (PartiallyBalloonedPage)
allocated. Let's free it.

Fixes: ed48c59875b6 ("virtio-balloon: Safely handle BALLOON_PAGE_SIZE <
                     host page size")
Cc: qemu-stable@nongnu.org #v4.0.0
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Cc: David Gibson <david@gibson.dropbear.id.au>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/virtio/virtio-balloon.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 669067d661..84d01bceb3 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -40,6 +40,12 @@ struct PartiallyBalloonedPage {
     unsigned long bitmap[];
 };
 
+static void virtio_balloon_reset_pbp(VirtIOBalloon *balloon)
+{
+    g_free(balloon->pbp);
+    balloon->pbp = NULL;
+}
+
 static void balloon_inflate_page(VirtIOBalloon *balloon,
                                  MemoryRegion *mr, hwaddr offset)
 {
@@ -82,8 +88,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 */
-        g_free(balloon->pbp);
-        balloon->pbp = NULL;
+        virtio_balloon_reset_pbp(balloon);
     }
 
     if (!balloon->pbp) {
@@ -106,8 +111,7 @@ static void balloon_inflate_page(VirtIOBalloon *balloon,
          * has already reported them, and failing to discard a balloon
          * page is not fatal */
 
-        g_free(balloon->pbp);
-        balloon->pbp = NULL;
+        virtio_balloon_reset_pbp(balloon);
     }
 }
 
@@ -143,8 +147,7 @@ static void balloon_deflate_page(VirtIOBalloon *balloon,
                   balloon->pbp->bitmap);
 
         if (bitmap_empty(balloon->pbp->bitmap, subpages)) {
-            g_free(balloon->pbp);
-            balloon->pbp = NULL;
+            virtio_balloon_reset_pbp(balloon);
         }
     }
 
@@ -831,6 +834,7 @@ static void virtio_balloon_device_unrealize(DeviceState *dev, Error **errp)
         virtio_balloon_free_page_stop(s);
         precopy_remove_notifier(&s->free_page_report_notify);
     }
+    virtio_balloon_reset_pbp(s);
     balloon_stats_destroy_timer(s);
     qemu_remove_balloon_handler(s);
     virtio_cleanup(vdev);
-- 
2.21.0

Re: [Qemu-devel] [PATCH-for-4.1 v2 2/3] virtio-balloon: fix memory leak on unrealize()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2966 bytes --]

On Wed, Jul 17, 2019 at 12:35:49PM +0200, David Hildenbrand wrote:
> We could have tracking data for a pbp (PartiallyBalloonedPage)
> allocated. Let's free it.
> 
> Fixes: ed48c59875b6 ("virtio-balloon: Safely handle BALLOON_PAGE_SIZE <
>                      host page size")
> Cc: qemu-stable@nongnu.org #v4.0.0
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>

Acked-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/virtio/virtio-balloon.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 669067d661..84d01bceb3 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -40,6 +40,12 @@ struct PartiallyBalloonedPage {
>      unsigned long bitmap[];
>  };
>  
> +static void virtio_balloon_reset_pbp(VirtIOBalloon *balloon)
> +{
> +    g_free(balloon->pbp);
> +    balloon->pbp = NULL;
> +}
> +
>  static void balloon_inflate_page(VirtIOBalloon *balloon,
>                                   MemoryRegion *mr, hwaddr offset)
>  {
> @@ -82,8 +88,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 */
> -        g_free(balloon->pbp);
> -        balloon->pbp = NULL;
> +        virtio_balloon_reset_pbp(balloon);
>      }
>  
>      if (!balloon->pbp) {
> @@ -106,8 +111,7 @@ static void balloon_inflate_page(VirtIOBalloon *balloon,
>           * has already reported them, and failing to discard a balloon
>           * page is not fatal */
>  
> -        g_free(balloon->pbp);
> -        balloon->pbp = NULL;
> +        virtio_balloon_reset_pbp(balloon);
>      }
>  }
>  
> @@ -143,8 +147,7 @@ static void balloon_deflate_page(VirtIOBalloon *balloon,
>                    balloon->pbp->bitmap);
>  
>          if (bitmap_empty(balloon->pbp->bitmap, subpages)) {
> -            g_free(balloon->pbp);
> -            balloon->pbp = NULL;
> +            virtio_balloon_reset_pbp(balloon);
>          }
>      }
>  
> @@ -831,6 +834,7 @@ static void virtio_balloon_device_unrealize(DeviceState *dev, Error **errp)
>          virtio_balloon_free_page_stop(s);
>          precopy_remove_notifier(&s->free_page_report_notify);
>      }
> +    virtio_balloon_reset_pbp(s);
>      balloon_stats_destroy_timer(s);
>      qemu_remove_balloon_handler(s);
>      virtio_cleanup(vdev);

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[Qemu-devel] [PATCH-for-4.1 v2 3/3] virtio-balloon: reset pbp on device resets

[David Hildenbrand]

When a guest reboots (ordinary reboots, but also via kexec), it will
happily reuse any system memory, including previously inflated memory.

We could have tracking data for a pbp (PartiallyBalloonedPage). It could
happen that a new inflation request from the guest will result in a
discard of such a pbp, although the guest is (again) reusing some
memory.

We should reset the pbp on any device resets.

Fixes: ed48c59875b6 ("virtio-balloon: Safely handle BALLOON_PAGE_SIZE <
                     host page size")
Cc: qemu-stable@nongnu.org #v4.0.0
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Cc: David Gibson <david@gibson.dropbear.id.au>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/virtio/virtio-balloon.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 84d01bceb3..9de3c030bf 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -847,6 +847,7 @@ static void virtio_balloon_device_reset(VirtIODevice *vdev)
     if (virtio_balloon_free_page_support(s)) {
         virtio_balloon_free_page_stop(s);
     }
+    virtio_balloon_reset_pbp(s);
 
     if (s->stats_vq_elem != NULL) {
         virtqueue_unpop(s->svq, s->stats_vq_elem, 0);
-- 
2.21.0

Re: [Qemu-devel] [PATCH-for-4.1 v2 3/3] virtio-balloon: reset pbp on device resets

[Michael S. Tsirkin]

On Wed, Jul 17, 2019 at 12:35:50PM +0200, David Hildenbrand wrote:
> When a guest reboots (ordinary reboots, but also via kexec), it will
> happily reuse any system memory, including previously inflated memory.
> 
> We could have tracking data for a pbp (PartiallyBalloonedPage). It could
> happen that a new inflation request from the guest will result in a
> discard of such a pbp, although the guest is (again) reusing some
> memory.
> 
> We should reset the pbp on any device resets.
> 
> Fixes: ed48c59875b6 ("virtio-balloon: Safely handle BALLOON_PAGE_SIZE <
>                      host page size")
> Cc: qemu-stable@nongnu.org #v4.0.0
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>

Can't something else remove a ramblock besides a reset?


> ---
>  hw/virtio/virtio-balloon.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 84d01bceb3..9de3c030bf 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -847,6 +847,7 @@ static void virtio_balloon_device_reset(VirtIODevice *vdev)
>      if (virtio_balloon_free_page_support(s)) {
>          virtio_balloon_free_page_stop(s);
>      }
> +    virtio_balloon_reset_pbp(s);
>  
>      if (s->stats_vq_elem != NULL) {
>          virtqueue_unpop(s->svq, s->stats_vq_elem, 0);
> -- 
> 2.21.0

Re: [Qemu-devel] [PATCH-for-4.1 v2 3/3] virtio-balloon: reset pbp on device resets

[David Hildenbrand]

On 17.07.19 12:48, Michael S. Tsirkin wrote:
> On Wed, Jul 17, 2019 at 12:35:50PM +0200, David Hildenbrand wrote:
>> When a guest reboots (ordinary reboots, but also via kexec), it will
>> happily reuse any system memory, including previously inflated memory.
>>
>> We could have tracking data for a pbp (PartiallyBalloonedPage). It could
>> happen that a new inflation request from the guest will result in a
>> discard of such a pbp, although the guest is (again) reusing some
>> memory.
>>
>> We should reset the pbp on any device resets.
>>
>> Fixes: ed48c59875b6 ("virtio-balloon: Safely handle BALLOON_PAGE_SIZE <
>>                      host page size")
>> Cc: qemu-stable@nongnu.org #v4.0.0
>> Cc: Stefan Hajnoczi <stefanha@redhat.com>
>> Cc: David Gibson <david@gibson.dropbear.id.au>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Cc: Igor Mammedov <imammedo@redhat.com>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
> 
> Can't something else remove a ramblock besides a reset?

Yes, however this patch is not about ramblocks getting removed.

Take a close look, "balloon->pbp->rb" is only used as a token, it is
never used besides for comparisons.

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH-for-4.1 v2 3/3] virtio-balloon: reset pbp on device resets

[Michael S. Tsirkin]

On Wed, Jul 17, 2019 at 01:06:29PM +0200, David Hildenbrand wrote:
> On 17.07.19 12:48, Michael S. Tsirkin wrote:
> > On Wed, Jul 17, 2019 at 12:35:50PM +0200, David Hildenbrand wrote:
> >> When a guest reboots (ordinary reboots, but also via kexec), it will
> >> happily reuse any system memory, including previously inflated memory.
> >>
> >> We could have tracking data for a pbp (PartiallyBalloonedPage). It could
> >> happen that a new inflation request from the guest will result in a
> >> discard of such a pbp, although the guest is (again) reusing some
> >> memory.
> >>
> >> We should reset the pbp on any device resets.
> >>
> >> Fixes: ed48c59875b6 ("virtio-balloon: Safely handle BALLOON_PAGE_SIZE <
> >>                      host page size")
> >> Cc: qemu-stable@nongnu.org #v4.0.0
> >> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> >> Cc: David Gibson <david@gibson.dropbear.id.au>
> >> Cc: Michael S. Tsirkin <mst@redhat.com>
> >> Cc: Igor Mammedov <imammedo@redhat.com>
> >> Signed-off-by: David Hildenbrand <david@redhat.com>
> > 
> > Can't something else remove a ramblock besides a reset?
> 
> Yes, however this patch is not about ramblocks getting removed.
> 
> Take a close look, "balloon->pbp->rb" is only used as a token, it is
> never used besides for comparisons.


You are right but that's still not safe :)

E.g. the bit we are going to set could be out of range of the bitmap because
the backing page size changed.



> -- 
> 
> Thanks,
> 
> David / dhildenb

Re: [Qemu-devel] [PATCH-for-4.1 v2 3/3] virtio-balloon: reset pbp on device resets

[David Hildenbrand]

On 17.07.19 13:29, Michael S. Tsirkin wrote:
> On Wed, Jul 17, 2019 at 01:06:29PM +0200, David Hildenbrand wrote:
>> On 17.07.19 12:48, Michael S. Tsirkin wrote:
>>> On Wed, Jul 17, 2019 at 12:35:50PM +0200, David Hildenbrand wrote:
>>>> When a guest reboots (ordinary reboots, but also via kexec), it will
>>>> happily reuse any system memory, including previously inflated memory.
>>>>
>>>> We could have tracking data for a pbp (PartiallyBalloonedPage). It could
>>>> happen that a new inflation request from the guest will result in a
>>>> discard of such a pbp, although the guest is (again) reusing some
>>>> memory.
>>>>
>>>> We should reset the pbp on any device resets.
>>>>
>>>> Fixes: ed48c59875b6 ("virtio-balloon: Safely handle BALLOON_PAGE_SIZE <
>>>>                      host page size")
>>>> Cc: qemu-stable@nongnu.org #v4.0.0
>>>> Cc: Stefan Hajnoczi <stefanha@redhat.com>
>>>> Cc: David Gibson <david@gibson.dropbear.id.au>
>>>> Cc: Michael S. Tsirkin <mst@redhat.com>
>>>> Cc: Igor Mammedov <imammedo@redhat.com>
>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>
>>> Can't something else remove a ramblock besides a reset?
>>
>> Yes, however this patch is not about ramblocks getting removed.
>>
>> Take a close look, "balloon->pbp->rb" is only used as a token, it is
>> never used besides for comparisons.
> 
> 
> You are right but that's still not safe :)
> 
> E.g. the bit we are going to set could be out of range of the bitmap because
> the backing page size changed.

As replied to the other thread, I agree. Will look into fixing this,
too, tomorrow!

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH-for-4.1 v2 3/3] virtio-balloon: reset pbp on device resets

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1742 bytes --]

On Wed, Jul 17, 2019 at 12:35:50PM +0200, David Hildenbrand wrote:
> When a guest reboots (ordinary reboots, but also via kexec), it will
> happily reuse any system memory, including previously inflated memory.
> 
> We could have tracking data for a pbp (PartiallyBalloonedPage). It could
> happen that a new inflation request from the guest will result in a
> discard of such a pbp, although the guest is (again) reusing some
> memory.
> 
> We should reset the pbp on any device resets.
> 
> Fixes: ed48c59875b6 ("virtio-balloon: Safely handle BALLOON_PAGE_SIZE <
>                      host page size")
> Cc: qemu-stable@nongnu.org #v4.0.0
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>

Acked-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/virtio/virtio-balloon.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 84d01bceb3..9de3c030bf 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -847,6 +847,7 @@ static void virtio_balloon_device_reset(VirtIODevice *vdev)
>      if (virtio_balloon_free_page_support(s)) {
>          virtio_balloon_free_page_stop(s);
>      }
> +    virtio_balloon_reset_pbp(s);
>  
>      if (s->stats_vq_elem != NULL) {
>          virtqueue_unpop(s->svq, s->stats_vq_elem, 0);

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]