virtio-balloon: fixes for PartialBalloonedPage

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

Posted by David Hildenbrand 6 years, 6 months ago

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

Posted by Michael S. Tsirkin 6 years, 6 months ago

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

Posted by David Hildenbrand 6 years, 6 months ago

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

Posted by Michael S. Tsirkin 6 years, 6 months ago

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

Posted by David Hildenbrand 6 years, 6 months ago

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

Posted by David Gibson 6 years, 6 months ago

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