On 23.07.19 04:54, David Gibson wrote:
> On Mon, Jul 22, 2019 at 03:41:07PM +0200, David Hildenbrand wrote:
>> Using the address of a RAMBlock to test for a matching pbp is not really
>> safe. Instead, let's use the guest physical address of the base page
>> along with the page size (via the number of subpages).
>>
>> Also, let's allocate the bitmap separately. This makes the code
>> easier to read and maintain - we can reuse bitmap_new().
>>
>> Prepare the code to move the PBP out of the device.
>>
>> 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-sta...@nongnu.org #v4.0.0
>> Signed-off-by: David Hildenbrand <da...@redhat.com>
>> ---
>> hw/virtio/virtio-balloon.c | 69 +++++++++++++++++++++++++-------------
>> 1 file changed, 46 insertions(+), 23 deletions(-)
>>
>> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
>> index f206cc8bf7..40d493a31a 100644
>> --- a/hw/virtio/virtio-balloon.c
>> +++ b/hw/virtio/virtio-balloon.c
>> @@ -35,16 +35,44 @@
>> #define BALLOON_PAGE_SIZE (1 << VIRTIO_BALLOON_PFN_SHIFT)
>>
>> struct PartiallyBalloonedPage {
>> - RAMBlock *rb;
>> - ram_addr_t base;
>> - unsigned long bitmap[];
>> + ram_addr_t base_gpa;
>> + long subpages;
>> + unsigned long *bitmap;
>> };
>>
>> +static void virtio_balloon_pbp_free(PartiallyBalloonedPage *pbp)
>> +{
>> + if (!pbp) {
>> + return;
>> + }
>> + g_free(pbp->bitmap);
>> + g_free(pbp);
>> +}
>> +
>> +static PartiallyBalloonedPage *virtio_balloon_pbp_alloc(ram_addr_t base_gpa,
>> + long subpages)
>> +{
>> + PartiallyBalloonedPage *pbp = g_new0(PartiallyBalloonedPage, 1);
>> +
>> + pbp->base_gpa = base_gpa;
>> + pbp->subpages = subpages;
>> + pbp->bitmap = bitmap_new(subpages);
>> +
>> + return pbp;
>> +}
>> +
>> +static bool virtio_balloon_pbp_matches(PartiallyBalloonedPage *pbp,
>> + ram_addr_t base_gpa, long subpages)
>> +{
>> + return pbp->subpages == subpages && pbp->base_gpa == base_gpa;
>
> I think the test on subpages is pointless here. You've (reasonably)
> rejected handling the edge case of a ramblock being removed and
> replugged - but the only thing this handles is an edge case of that
> edge case.
I think we have to leave it in place until patch #6. We could remove it
after Patch #6 - replug cannot happen while processing the inflation
requests.
--
Thanks,
David / dhildenb
