Re: [PATCH v2] virtio-balloon: Disable free page reporting if page poison reporting is not enabled

[Alexander Duyck <alexander.duyck@gmail.com>]

On Mon, Apr 27, 2020 at 1:08 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 24.04.20 18:24, Alexander Duyck wrote:
> > From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >
> > We should disable free page reporting if page poisoning is enabled in the
> > kernel but we cannot report it via the balloon interface. This way we can
> > avoid the possibility of corrupting guest memory. Normally the page poison
> > reporting feature should always be present when free page reporting is
> > enabled on the hypervisor, however this allows us to correctly handle a
> > case of the virtio-balloon device being possibly misconfigured.
> >
> > Fixes: 5d757c8d518d ("virtio-balloon: add support for providing free page reports to host")
> > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > ---
> >
> > Changes since v1:
> > Originally this patch also modified free page hinting, that has been removed.
> > Updated patch title and description.
> > Added a comment explaining reasoning for disabling free page reporting.
> >
> >  drivers/virtio/virtio_balloon.c |    9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> > index 51086a5afdd4..1f157d2f4952 100644
> > --- a/drivers/virtio/virtio_balloon.c
> > +++ b/drivers/virtio/virtio_balloon.c
> > @@ -1107,11 +1107,18 @@ static int virtballoon_restore(struct virtio_device *vdev)
> >
> >  static int virtballoon_validate(struct virtio_device *vdev)
> >  {
> > -     /* Tell the host whether we care about poisoned pages. */
> > +     /*
> > +      * Inform the hypervisor that our pages are poisoned or
> > +      * initialized. If we cannot do that then we should disable
> > +      * page reporting as it could potentially change the contents
> > +      * of our free pages.
> > +      */
> >       if (!want_init_on_free() &&
> >           (IS_ENABLED(CONFIG_PAGE_POISONING_NO_SANITY) ||
> >            !page_poisoning_enabled()))
> >               __virtio_clear_bit(vdev, VIRTIO_BALLOON_F_PAGE_POISON);
> > +     else if (!virtio_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_POISON))
> > +             __virtio_clear_bit(vdev, VIRTIO_BALLOON_F_REPORTING);
> >
> >       __virtio_clear_bit(vdev, VIRTIO_F_IOMMU_PLATFORM);
> >       return 0;
> >
>
> Did you see my feedback on v1?
>
> https://www.spinics.net/lists/linux-virtualization/msg42783.html
>
> In case of want_init_on_free(), we don't really need VIRTIO_BALLOON_F_PAGE_POISON.

I believe we do. We had discussions earlier in which Michael had
mentioned that the guest should not assume implementation details of
the host/hypervisor.

The PAGE_POISON feature is being used to indicate that we expect the
page to contain a certain value when it is returned to us. With the
current implementation in QEMU if that value is zero we can work with
it because that is the effect that MADV_DONTNEED has. However if there
were some other effect being used by QEMU we would want to know that
the guest is expecting us to write a 0 page. So I believe it makes
sense to inform the hypervisor that we are doing something that
expects us to fill the page with zeros in the case of
want_init_on_free rather than not providing that information to QEMU.
This way, if in the future we decide to change the implementation in
some way that might effect the value contained in the pages, we might
have the flexibility to identify the want_init_on_free case so we can
work around it.

In reality it isn't too different from informing QEMU that we are
performing poison with a value of 0 anyway which if I recall is what
led me to adding the want_init_on_free check as they are all lumped
together in free_pages_prezeroed():
https://elixir.bootlin.com/linux/v5.7-rc3/source/mm/page_alloc.c#L2134

Thanks.

- Alex