| Message ID | 20190306030601.21986-3-david@gibson.dropbear.id.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | virtio-balloon: Several fixes to recent rework | expand |
On 06.03.19 04:06, David Gibson wrote: > This fixes a balloon bug with a nasty consequence - potentially > corrupting guest memory - but which is extremely unlikely to be > triggered in practice. > > The balloon always works in 4kiB units, but the host could have a > larger page size on certain platforms. Since ed48c59 "virtio-balloon: > Safely handle BALLOON_PAGE_SIZE < host page size" we've handled this > by accumulating requests to balloon 4kiB subpages until they formed a > full host page. Since f6deb6d "virtio-balloon: Remove unnecessary > MADV_WILLNEED on deflate" we essentially ignore deflate requests. > > Suppose we have a host with 8kiB pages, and one host page has subpages > A & B. If we get this sequence of events - > inflate A > deflate A > inflate B > - the current logic will discard the whole host page. That's > incorrect because the guest has deflated subpage A, and could have > written important data to it. > > This patch fixes the problem by adjusting our state information about > partially ballooned host pages when deflate requests are received. > > Fixes: ed48c59 "virtio-balloon: Safely handle BALLOON_PAGE_SIZE < host page size" > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- > hw/virtio/virtio-balloon.c | 48 ++++++++++++++++++++++++++++++++++++-- > 1 file changed, 46 insertions(+), 2 deletions(-) > > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c > index 127289ae0e..7412bf8c85 100644 > --- a/hw/virtio/virtio-balloon.c > +++ b/hw/virtio/virtio-balloon.c > @@ -111,6 +111,43 @@ static void balloon_inflate_page(VirtIOBalloon *balloon, > } > } > > +static void balloon_deflate_page(VirtIOBalloon *balloon, > + MemoryRegion *mr, hwaddr offset) > +{ > + void *addr = memory_region_get_ram_ptr(mr) + offset; > + RAMBlock *rb; > + size_t rb_page_size; > + ram_addr_t ram_offset, host_page_base; > + > + /* XXX is there a better way to get to the RAMBlock than via a > + * host address? */ > + rb = qemu_ram_block_from_host(addr, false, &ram_offset); > + rb_page_size = qemu_ram_pagesize(rb); > + host_page_base = ram_offset & ~(rb_page_size - 1); > + > + if (balloon->pbp > + && rb == balloon->pbp->rb > + && host_page_base == balloon->pbp->base) { > + int subpages = rb_page_size / BALLOON_PAGE_SIZE; > + > + /* > + * This means the guest has asked to discard some of the 4kiB > + * subpages of a host page, but then changed its mind and > + * asked to keep them after all. It's exceedingly unlikely > + * 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); > + > + if (bitmap_empty(balloon->pbp->bitmap, subpages)) { > + g_free(balloon->pbp); > + balloon->pbp = NULL; > + } > + } > +} > + > static const char *balloon_stat_names[] = { > [VIRTIO_BALLOON_S_SWAP_IN] = "stat-swap-in", > [VIRTIO_BALLOON_S_SWAP_OUT] = "stat-swap-out", > @@ -314,8 +351,15 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq) > > trace_virtio_balloon_handle_output(memory_region_name(section.mr), > pa); > - if (!qemu_balloon_is_inhibited() && vq != s->dvq) { > - balloon_inflate_page(s, section.mr, section.offset_within_region); > + if (!qemu_balloon_is_inhibited()) { > + if (vq == s->ivq) { > + balloon_inflate_page(s, section.mr, > + section.offset_within_region); > + } else if (vq == s->dvq) { > + balloon_deflate_page(s, section.mr, section.offset_within_region); > + } else { > + g_assert_not_reached(); > + } > } > memory_region_unref(section.mr); > } > Acked-by: David Hildenbrand <david@redhat.com>
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c index 127289ae0e..7412bf8c85 100644 --- a/hw/virtio/virtio-balloon.c +++ b/hw/virtio/virtio-balloon.c @@ -111,6 +111,43 @@ static void balloon_inflate_page(VirtIOBalloon *balloon, } } +static void balloon_deflate_page(VirtIOBalloon *balloon, + MemoryRegion *mr, hwaddr offset) +{ + void *addr = memory_region_get_ram_ptr(mr) + offset; + RAMBlock *rb; + size_t rb_page_size; + ram_addr_t ram_offset, host_page_base; + + /* XXX is there a better way to get to the RAMBlock than via a + * host address? */ + rb = qemu_ram_block_from_host(addr, false, &ram_offset); + rb_page_size = qemu_ram_pagesize(rb); + host_page_base = ram_offset & ~(rb_page_size - 1); + + if (balloon->pbp + && rb == balloon->pbp->rb + && host_page_base == balloon->pbp->base) { + int subpages = rb_page_size / BALLOON_PAGE_SIZE; + + /* + * This means the guest has asked to discard some of the 4kiB + * subpages of a host page, but then changed its mind and + * asked to keep them after all. It's exceedingly unlikely + * 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); + + if (bitmap_empty(balloon->pbp->bitmap, subpages)) { + g_free(balloon->pbp); + balloon->pbp = NULL; + } + } +} + static const char *balloon_stat_names[] = { [VIRTIO_BALLOON_S_SWAP_IN] = "stat-swap-in", [VIRTIO_BALLOON_S_SWAP_OUT] = "stat-swap-out", @@ -314,8 +351,15 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq) trace_virtio_balloon_handle_output(memory_region_name(section.mr), pa); - if (!qemu_balloon_is_inhibited() && vq != s->dvq) { - balloon_inflate_page(s, section.mr, section.offset_within_region); + if (!qemu_balloon_is_inhibited()) { + if (vq == s->ivq) { + balloon_inflate_page(s, section.mr, + section.offset_within_region); + } else if (vq == s->dvq) { + balloon_deflate_page(s, section.mr, section.offset_within_region); + } else { + g_assert_not_reached(); + } } memory_region_unref(section.mr); }
This fixes a balloon bug with a nasty consequence - potentially corrupting guest memory - but which is extremely unlikely to be triggered in practice. The balloon always works in 4kiB units, but the host could have a larger page size on certain platforms. Since ed48c59 "virtio-balloon: Safely handle BALLOON_PAGE_SIZE < host page size" we've handled this by accumulating requests to balloon 4kiB subpages until they formed a full host page. Since f6deb6d "virtio-balloon: Remove unnecessary MADV_WILLNEED on deflate" we essentially ignore deflate requests. Suppose we have a host with 8kiB pages, and one host page has subpages A & B. If we get this sequence of events - inflate A deflate A inflate B - the current logic will discard the whole host page. That's incorrect because the guest has deflated subpage A, and could have written important data to it. This patch fixes the problem by adjusting our state information about partially ballooned host pages when deflate requests are received. Fixes: ed48c59 "virtio-balloon: Safely handle BALLOON_PAGE_SIZE < host page size" Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- hw/virtio/virtio-balloon.c | 48 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 46 insertions(+), 2 deletions(-)