Linux Virtualization - Apr 2020 - [PATCH v2] virtio-balloon: Disable free page reporting if page poison reporting is not enabled

On Mon, Apr 27, 2020 at 1:08 AM David Hildenbrand <david at redhat.com>
wrote:
>
> On 24.04.20 18:24, Alexander Duyck wrote:
> > From: Alexander Duyck <alexander.h.duyck at 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 at
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

On 27.04.20 16:58, Alexander Duyck wrote:
> On Mon, Apr 27, 2020 at 1:08 AM David Hildenbrand <david at
redhat.com> wrote:
>>
>> On 24.04.20 18:24, Alexander Duyck wrote:
>>> From: Alexander Duyck <alexander.h.duyck at 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 at
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.
We can simply document this, as I suggested already ("either the old
page, or a page filled with zero"). Perfectly fine IMHO.
> 
> 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
Informing makes sense, yes. And we inform it via the poison feature, if
possible. This discussion is about "what happens if the poison feature
is not there, but we do have want_init_on_free()."
> 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
If the hypervisor can no longer guarantee "either the old page, or a
page filled with zero", it would have to disable the feature. I don't
see that happening, really.
> have the flexibility to identify the want_init_on_free case so we can
> work around it.
I don't have a too strong opinion here, but this sounds like one of the
improvements we wanted to have for free page hinting, but learned that
it's not possible because it was *underspecified*.


-- 
Thanks,

David / dhildenb

On Mon, Apr 27, 2020 at 8:09 AM David Hildenbrand <david at redhat.com>
wrote:
>
> On 27.04.20 16:58, Alexander Duyck wrote:
> > On Mon, Apr 27, 2020 at 1:08 AM David Hildenbrand <david at
redhat.com> wrote:
> >>
> >> On 24.04.20 18:24, Alexander Duyck wrote:
> >>> From: Alexander Duyck <alexander.h.duyck at
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 at
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.
>
> We can simply document this, as I suggested already ("either the old
> page, or a page filled with zero"). Perfectly fine IMHO.
>
> >
> > 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
>
> Informing makes sense, yes. And we inform it via the poison feature, if
> possible. This discussion is about "what happens if the poison feature
> is not there, but we do have want_init_on_free()."
My preference is to err on the side of caution.  I view want_init_on
free() as equivalent to page_poison_enabled with
CONFIG_PAGE_POISONING_ZERO and treated the same way. So if we are
going to let one by then we would want to let the other by as well.
> > 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
>
> If the hypervisor can no longer guarantee "either the old page, or a
> page filled with zero", it would have to disable the feature. I
don't
> see that happening, really.
My concern is we can never see how things might be used in the future.
The advantage of setting the poison feature flag and leaving the value
as 0 is that it indicates that we are strictly expecting a zero page
versus allowing the page to contain some other content. I'm expecting
that in most cases that we should see that the page poisoning feature
will be present if page reporting is present so my thought is it is
safer to put the limitation on the guest rather than on the host as it
allows us more flexibility for future implementations.
> > have the flexibility to identify the want_init_on_free case so we can
> > work around it.
>
> I don't have a too strong opinion here, but this sounds like one of the
> improvements we wanted to have for free page hinting, but learned that
> it's not possible because it was *underspecified*.
Right. For free page hinting it is underspecified. By defining a tight
relationship between free page reporting and page poison we might be
able to better handle those sorts of cases. I suspect there may be
interest in having free page reporting eventually take on many of the
responsibilities of free page hinting as it doesn't have the extra
complication of having to consume all of the guests memory. If we
dictate that free page reporting has to have a very clear definition
in relation to any page initialization it lays the groundwork for us
to be able to eventually handle those kind of cases.