| Message ID | 20190717103550.24657-4-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | virtio-balloon: fixes for PartialBalloonedPage | expand |
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
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.
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
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!
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);
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);
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(+)