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

[David Hildenbrand]

On 17.07.19 13:06, Michael S. Tsirkin wrote:
> On Wed, Jul 17, 2019 at 12:17:57PM +0200, David Hildenbrand wrote:
>> On 17.07.19 12:04, David Hildenbrand wrote:
>>> On 17.07.19 11:57, Michael S. Tsirkin wrote:
>>>> On Wed, Jul 17, 2019 at 10:42:55AM +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: address@hidden #v4.0.0
>>>>> Cc: Stefan Hajnoczi <address@hidden>
>>>>> Cc: David Gibson <address@hidden>
>>>>> Cc: Michael S. Tsirkin <address@hidden>
>>>>> Cc: Igor Mammedov <address@hidden>
>>>>> Signed-off-by: David Hildenbrand <address@hidden>
>>>>> ---
>>>>>  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);
>>>>
>>>> I also started to wonder about this:
>>>>
>>>>     if (!balloon->pbp) {
>>>>         /* Starting on a new host page */
>>>>         size_t bitlen = BITS_TO_LONGS(subpages) * sizeof(unsigned long);
>>>>         balloon->pbp = g_malloc0(sizeof(PartiallyBalloonedPage) + bitlen);
>>>>         balloon->pbp->rb = rb;
>>>>         balloon->pbp->base = host_page_base;
>>>>     }
>>>>
>>>> Is keeping a pointer to a ram block like this safe? what if the ramblock
>>>> gets removed?
>>>>
>>>
>>> David added
>>>
>>> if (balloon->pbp
>>>     && (rb != balloon->pbp->rb ) ...
>>>
>>> So in case the rb changes (IOW replaced - delete old one, new one
>>> added), we reset the data.
>>>
>>> After a ram block was deleted, there will be no more deflation requests
>>> coming in for it. This should be fine I guess.
> 
> I think it might happen that an old dangling pointer happens
> to match a newly allocated one.
> I think we really should just cache all data we want to take into account
> and compare that.

That's true. I think just remembering and comparing the GPA base address
would be sufficient.

However, I don't consider this here to trigger easily. We would need
some crazy memory unplug+replug going on while using the balloon. So I
assume we can just rework this part after 4.1

-- 

Thanks,

David / dhildenb