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

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

[Alexander Duyck]

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;

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

[David Hildenbrand]

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.


-- 
Thanks,

David / dhildenb

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

[Alexander Duyck]

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

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

[David Hildenbrand]

On 27.04.20 16:58, Alexander Duyck wrote:
> 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.

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

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

[Alexander Duyck]

On Mon, Apr 27, 2020 at 8:09 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 27.04.20 16:58, Alexander Duyck wrote:
> > 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.
>
> 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.

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

[David Hildenbrand]

>>>
>>> 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.
> 

Agreed.

>>> 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.
> 

I consider it very unlikely that you want to provide something else than
- the old page
- a zeroed page

But yes, while having VIRTIO_BALLOON_F_REPORTING &&
!VIRTIO_BALLOON_F_PAGE_POISON in QEMU as of now would be possible, it's
unlikely that we'll see this very often. Similar to want_init_on_free()

>>> 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.
I would encourage you to document the semantics of free page reporting
properly - especially, that we decided that the content of a reported
page with VIRTIO_BALLOON_F_REPORTING && !VIRTIO_BALLOON_F_PAGE_POISON is
*undefined* and needs a reinit. Only with VIRTIO_BALLOON_F_REPORTING &&
VIRTIO_BALLOON_F_PAGE_POISON the content is either the old page content
or a page filled with poison_val.

So take my

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb