* [Qemu-devel] [PATCH-for-4.1 v2 0/3] virtio-balloon: fixes for PartialBalloonedPage
@ 2019-07-17 10:35 David Hildenbrand
2019-07-17 10:35 ` [Qemu-devel] [PATCH-for-4.1 v2 1/3] virtio-balloon: fix QEMU crashes on pagesize > BALLOON_PAGE_SIZE David Hildenbrand
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: David Hildenbrand @ 2019-07-17 10:35 UTC (permalink / raw)
To: qemu-devel
Cc: Igor Mammedov, David Gibson, Michael S . Tsirkin,
Stefan Hajnoczi, 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Qemu-devel] [PATCH-for-4.1 v2 1/3] virtio-balloon: fix QEMU crashes on pagesize > BALLOON_PAGE_SIZE
2019-07-17 10:35 [Qemu-devel] [PATCH-for-4.1 v2 0/3] virtio-balloon: fixes for PartialBalloonedPage David Hildenbrand
@ 2019-07-17 10:35 ` David Hildenbrand
2019-07-18 3:50 ` David Gibson
2019-07-17 10:35 ` [Qemu-devel] [PATCH-for-4.1 v2 2/3] virtio-balloon: fix memory leak on unrealize() David Hildenbrand
2019-07-17 10:35 ` [Qemu-devel] [PATCH-for-4.1 v2 3/3] virtio-balloon: reset pbp on device resets David Hildenbrand
2 siblings, 1 reply; 12+ messages in thread
From: David Hildenbrand @ 2019-07-17 10:35 UTC (permalink / raw)
To: qemu-devel
Cc: Michael S . Tsirkin, David Hildenbrand, qemu-stable,
Stefan Hajnoczi, Igor Mammedov, David Gibson
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [Qemu-devel] [PATCH-for-4.1 v2 2/3] virtio-balloon: fix memory leak on unrealize()
2019-07-17 10:35 [Qemu-devel] [PATCH-for-4.1 v2 0/3] virtio-balloon: fixes for PartialBalloonedPage David Hildenbrand
2019-07-17 10:35 ` [Qemu-devel] [PATCH-for-4.1 v2 1/3] virtio-balloon: fix QEMU crashes on pagesize > BALLOON_PAGE_SIZE David Hildenbrand
@ 2019-07-17 10:35 ` David Hildenbrand
2019-07-18 3:51 ` David Gibson
2019-07-17 10:35 ` [Qemu-devel] [PATCH-for-4.1 v2 3/3] virtio-balloon: reset pbp on device resets David Hildenbrand
2 siblings, 1 reply; 12+ messages in thread
From: David Hildenbrand @ 2019-07-17 10:35 UTC (permalink / raw)
To: qemu-devel
Cc: Michael S . Tsirkin, David Hildenbrand, qemu-stable,
Stefan Hajnoczi, Igor Mammedov, David Gibson
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [Qemu-devel] [PATCH-for-4.1 v2 3/3] virtio-balloon: reset pbp on device resets
2019-07-17 10:35 [Qemu-devel] [PATCH-for-4.1 v2 0/3] virtio-balloon: fixes for PartialBalloonedPage David Hildenbrand
2019-07-17 10:35 ` [Qemu-devel] [PATCH-for-4.1 v2 1/3] virtio-balloon: fix QEMU crashes on pagesize > BALLOON_PAGE_SIZE David Hildenbrand
2019-07-17 10:35 ` [Qemu-devel] [PATCH-for-4.1 v2 2/3] virtio-balloon: fix memory leak on unrealize() David Hildenbrand
@ 2019-07-17 10:35 ` David Hildenbrand
2019-07-17 10:48 ` Michael S. Tsirkin
2019-07-18 3:52 ` David Gibson
2 siblings, 2 replies; 12+ messages in thread
From: David Hildenbrand @ 2019-07-17 10:35 UTC (permalink / raw)
To: qemu-devel
Cc: Michael S . Tsirkin, David Hildenbrand, qemu-stable,
Stefan Hajnoczi, Igor Mammedov, David Gibson
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH-for-4.1 v2 3/3] virtio-balloon: reset pbp on device resets
2019-07-17 10:35 ` [Qemu-devel] [PATCH-for-4.1 v2 3/3] virtio-balloon: reset pbp on device resets David Hildenbrand
@ 2019-07-17 10:48 ` Michael S. Tsirkin
2019-07-17 11:06 ` David Hildenbrand
2019-07-18 3:52 ` David Gibson
1 sibling, 1 reply; 12+ messages in thread
From: Michael S. Tsirkin @ 2019-07-17 10:48 UTC (permalink / raw)
To: David Hildenbrand
Cc: Igor Mammedov, qemu-stable, qemu-devel, Stefan Hajnoczi, David Gibson
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH-for-4.1 v2 3/3] virtio-balloon: reset pbp on device resets
2019-07-17 10:48 ` Michael S. Tsirkin
@ 2019-07-17 11:06 ` David Hildenbrand
2019-07-17 11:29 ` Michael S. Tsirkin
0 siblings, 1 reply; 12+ messages in thread
From: David Hildenbrand @ 2019-07-17 11:06 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Igor Mammedov, qemu-stable, qemu-devel, Stefan Hajnoczi, David Gibson
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH-for-4.1 v2 3/3] virtio-balloon: reset pbp on device resets
2019-07-17 11:06 ` David Hildenbrand
@ 2019-07-17 11:29 ` Michael S. Tsirkin
2019-07-17 11:32 ` David Hildenbrand
0 siblings, 1 reply; 12+ messages in thread
From: Michael S. Tsirkin @ 2019-07-17 11:29 UTC (permalink / raw)
To: David Hildenbrand
Cc: Igor Mammedov, qemu-stable, qemu-devel, Stefan Hajnoczi, David Gibson
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH-for-4.1 v2 3/3] virtio-balloon: reset pbp on device resets
2019-07-17 11:29 ` Michael S. Tsirkin
@ 2019-07-17 11:32 ` David Hildenbrand
0 siblings, 0 replies; 12+ messages in thread
From: David Hildenbrand @ 2019-07-17 11:32 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Igor Mammedov, qemu-stable, qemu-devel, Stefan Hajnoczi, David Gibson
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH-for-4.1 v2 1/3] virtio-balloon: fix QEMU crashes on pagesize > BALLOON_PAGE_SIZE
2019-07-17 10:35 ` [Qemu-devel] [PATCH-for-4.1 v2 1/3] virtio-balloon: fix QEMU crashes on pagesize > BALLOON_PAGE_SIZE David Hildenbrand
@ 2019-07-18 3:50 ` David Gibson
2019-07-18 11:50 ` David Hildenbrand
0 siblings, 1 reply; 12+ messages in thread
From: David Gibson @ 2019-07-18 3:50 UTC (permalink / raw)
To: David Hildenbrand
Cc: Igor Mammedov, qemu-stable, qemu-devel, Stefan Hajnoczi,
Michael S . Tsirkin
[-- 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 --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH-for-4.1 v2 2/3] virtio-balloon: fix memory leak on unrealize()
2019-07-17 10:35 ` [Qemu-devel] [PATCH-for-4.1 v2 2/3] virtio-balloon: fix memory leak on unrealize() David Hildenbrand
@ 2019-07-18 3:51 ` David Gibson
0 siblings, 0 replies; 12+ messages in thread
From: David Gibson @ 2019-07-18 3:51 UTC (permalink / raw)
To: David Hildenbrand
Cc: Igor Mammedov, qemu-stable, qemu-devel, Stefan Hajnoczi,
Michael S . Tsirkin
[-- 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 --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH-for-4.1 v2 3/3] virtio-balloon: reset pbp on device resets
2019-07-17 10:35 ` [Qemu-devel] [PATCH-for-4.1 v2 3/3] virtio-balloon: reset pbp on device resets David Hildenbrand
2019-07-17 10:48 ` Michael S. Tsirkin
@ 2019-07-18 3:52 ` David Gibson
1 sibling, 0 replies; 12+ messages in thread
From: David Gibson @ 2019-07-18 3:52 UTC (permalink / raw)
To: David Hildenbrand
Cc: Igor Mammedov, qemu-stable, qemu-devel, Stefan Hajnoczi,
Michael S . Tsirkin
[-- 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 --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH-for-4.1 v2 1/3] virtio-balloon: fix QEMU crashes on pagesize > BALLOON_PAGE_SIZE
2019-07-18 3:50 ` David Gibson
@ 2019-07-18 11:50 ` David Hildenbrand
0 siblings, 0 replies; 12+ messages in thread
From: David Hildenbrand @ 2019-07-18 11:50 UTC (permalink / raw)
To: David Gibson
Cc: Igor Mammedov, qemu-stable, qemu-devel, Stefan Hajnoczi,
Michael S . Tsirkin
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
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2019-07-18 11:50 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-17 10:35 [Qemu-devel] [PATCH-for-4.1 v2 0/3] virtio-balloon: fixes for PartialBalloonedPage David Hildenbrand
2019-07-17 10:35 ` [Qemu-devel] [PATCH-for-4.1 v2 1/3] virtio-balloon: fix QEMU crashes on pagesize > BALLOON_PAGE_SIZE David Hildenbrand
2019-07-18 3:50 ` David Gibson
2019-07-18 11:50 ` David Hildenbrand
2019-07-17 10:35 ` [Qemu-devel] [PATCH-for-4.1 v2 2/3] virtio-balloon: fix memory leak on unrealize() David Hildenbrand
2019-07-18 3:51 ` David Gibson
2019-07-17 10:35 ` [Qemu-devel] [PATCH-for-4.1 v2 3/3] virtio-balloon: reset pbp on device resets David Hildenbrand
2019-07-17 10:48 ` Michael S. Tsirkin
2019-07-17 11:06 ` David Hildenbrand
2019-07-17 11:29 ` Michael S. Tsirkin
2019-07-17 11:32 ` David Hildenbrand
2019-07-18 3:52 ` David Gibson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).