thr3ads.net - Linux Virtualization - [PATCH] virtio_balloon: clear modern features under legacy [Jul 2020]

If this information is useful, please help other people find it:
Share via:

Michael S. Tsirkin

2020-Jul-10 11:31 UTC

[PATCH] virtio_balloon: clear modern features under legacy

Page reporting features were never supported by legacy hypervisors.
Supporting them poses a problem: should we use native endian-ness (like
current code assumes)? Or little endian-ness like the virtio spec says?
Rather than try to figure out, and since results of
incorrect endian-ness are dire, let's just block this configuration.

Cc: stable at vger.kernel.org
Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
---
 drivers/virtio/virtio_balloon.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 5d4b891bf84f..b9bc03345157 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -1107,6 +1107,15 @@ static int virtballoon_restore(struct virtio_device
*vdev)
 
 static int virtballoon_validate(struct virtio_device *vdev)
 {
+	/*
+	 * Legacy devices never specified how modern features should behave.
+	 * E.g. which endian-ness to use? Better not to assume anything.
+	 */
+	if (!virtio_has_feature(vdev, VIRTIO_F_VERSION_1)) {
+		__virtio_clear_bit(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT);
+		__virtio_clear_bit(vdev, VIRTIO_BALLOON_F_PAGE_POISON);
+		__virtio_clear_bit(vdev, VIRTIO_BALLOON_F_REPORTING);
+	}
 	/*
 	 * Inform the hypervisor that our pages are poisoned or
 	 * initialized. If we cannot do that then we should disable
-- 
MST

David Hildenbrand

2020-Jul-10 11:32 UTC

head link

[PATCH] virtio_balloon: clear modern features under legacy

On 10.07.20 13:31, Michael S. Tsirkin wrote:> Page reporting features were never supported by legacy hypervisors.
> Supporting them poses a problem: should we use native endian-ness (like
> current code assumes)? Or little endian-ness like the virtio spec says?
> Rather than try to figure out, and since results of
> incorrect endian-ness are dire, let's just block this configuration.
> 
> Cc: stable at vger.kernel.org
> Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
> ---
>  drivers/virtio/virtio_balloon.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/virtio/virtio_balloon.c
b/drivers/virtio/virtio_balloon.c
> index 5d4b891bf84f..b9bc03345157 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -1107,6 +1107,15 @@ static int virtballoon_restore(struct virtio_device
*vdev)
>  
>  static int virtballoon_validate(struct virtio_device *vdev)
>  {
> +	/*
> +	 * Legacy devices never specified how modern features should behave.
> +	 * E.g. which endian-ness to use? Better not to assume anything.
> +	 */
> +	if (!virtio_has_feature(vdev, VIRTIO_F_VERSION_1)) {
> +		__virtio_clear_bit(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT);
> +		__virtio_clear_bit(vdev, VIRTIO_BALLOON_F_PAGE_POISON);
> +		__virtio_clear_bit(vdev, VIRTIO_BALLOON_F_REPORTING);
> +	}
>  	/*
>  	 * Inform the hypervisor that our pages are poisoned or
>  	 * initialized. If we cannot do that then we should disable
> 
Very right

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

-- 
Thanks,

David / dhildenb

Alexander Duyck

2020-Jul-10 16:13 UTC

head link

[PATCH] virtio_balloon: clear modern features under legacy

On Fri, Jul 10, 2020 at 4:31 AM Michael S. Tsirkin <mst at redhat.com>
wrote:>
> Page reporting features were never supported by legacy hypervisors.
> Supporting them poses a problem: should we use native endian-ness (like
> current code assumes)? Or little endian-ness like the virtio spec says?
> Rather than try to figure out, and since results of
> incorrect endian-ness are dire, let's just block this configuration.
>
> Cc: stable at vger.kernel.org
> Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
So I am not sure about the patch description. In the case of page
poison and free page reporting I don't think we are defining anything
that doesn't already have a definition of how to use in legacy.
Specifically the virtio_balloon_config is already defined as having
all fields as little endian in legacy mode, and there is a definition
for all of the fields in a virtqueue and how they behave in legacy
mode.

As far as I can see the only item that may be an issue is the command
ID being supplied via the virtqueue for free page hinting, which
appears to be in native endian-ness. Otherwise it would have fallen
into the same category since it is making use of virtio_balloon_config
and a virtqueue for supplying the page location and length.
> ---
>  drivers/virtio/virtio_balloon.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/drivers/virtio/virtio_balloon.c
b/drivers/virtio/virtio_balloon.c
> index 5d4b891bf84f..b9bc03345157 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -1107,6 +1107,15 @@ static int virtballoon_restore(struct virtio_device
*vdev)
>
>  static int virtballoon_validate(struct virtio_device *vdev)
>  {
> +       /*
> +        * Legacy devices never specified how modern features should
behave.
> +        * E.g. which endian-ness to use? Better not to assume anything.
> +        */
> +       if (!virtio_has_feature(vdev, VIRTIO_F_VERSION_1)) {
> +               __virtio_clear_bit(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT);
> +               __virtio_clear_bit(vdev, VIRTIO_BALLOON_F_PAGE_POISON);
> +               __virtio_clear_bit(vdev, VIRTIO_BALLOON_F_REPORTING);
> +       }
>         /*
>          * Inform the hypervisor that our pages are poisoned or
>          * initialized. If we cannot do that then we should disable
The patch content itself I am fine with since odds are nobody would
expect to use these features with a legacy device.

Acked-by: Alexander Duyck <alexander.h.duyck at linux.intel.com>

Michael S. Tsirkin

2020-Jul-12 15:09 UTC

head link

[PATCH] virtio_balloon: clear modern features under legacy

On Fri, Jul 10, 2020 at 09:13:41AM -0700, Alexander Duyck
wrote:> On Fri, Jul 10, 2020 at 4:31 AM Michael S. Tsirkin <mst at
redhat.com> wrote:
> >
> > Page reporting features were never supported by legacy hypervisors.
> > Supporting them poses a problem: should we use native endian-ness
(like
> > current code assumes)? Or little endian-ness like the virtio spec
says?
> > Rather than try to figure out, and since results of
> > incorrect endian-ness are dire, let's just block this
configuration.
> >
> > Cc: stable at vger.kernel.org
> > Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
> 
> So I am not sure about the patch description. In the case of page
> poison and free page reporting I don't think we are defining anything
> that doesn't already have a definition of how to use in legacy.
> Specifically the virtio_balloon_config is already defined as having
> all fields as little endian in legacy mode, and there is a definition
> for all of the fields in a virtqueue and how they behave in legacy
> mode.
> 
> As far as I can see the only item that may be an issue is the command
> ID being supplied via the virtqueue for free page hinting, which
> appears to be in native endian-ness. Otherwise it would have fallen
> into the same category since it is making use of virtio_balloon_config
> and a virtqueue for supplying the page location and length.


So as you point out correctly balloon spec says all fields are little
endian.  Fair enough.
Problem is when virtio 1 is not negotiated, then this is not what the
driver assumes for any except a handlful of fields.

But yes it mostly works out.

For example:


static void update_balloon_size(struct virtio_balloon *vb)
{
        u32 actual = vb->num_pages;

        /* Legacy balloon config space is LE, unlike all other devices. */
        if (!virtio_has_feature(vb->vdev, VIRTIO_F_VERSION_1))
                actual = (__force u32)cpu_to_le32(actual);

        virtio_cwrite(vb->vdev, struct virtio_balloon_config, actual,
                      &actual);
}


this is LE even without VIRTIO_F_VERSION_1, so matches spec.

                /* Start with poison val of 0 representing general init */
                __u32 poison_val = 0;

                /*
                 * Let the hypervisor know that we are expecting a
                 * specific value to be written back in balloon pages.
                 */
                if (!want_init_on_free())
                        memset(&poison_val, PAGE_POISON,
sizeof(poison_val));

                virtio_cwrite(vb->vdev, struct virtio_balloon_config,
                              poison_val, &poison_val);


actually this writes a native endian-ness value. All bytes happen to be
the same though, and host only cares about 0 or non 0 ATM.

As you say correctly the command id is actually assumed native endian:


static u32 virtio_balloon_cmd_id_received(struct virtio_balloon *vb)
{
        if (test_and_clear_bit(VIRTIO_BALLOON_CONFIG_READ_CMD_ID,
                               &vb->config_read_bitmap))
                virtio_cread(vb->vdev, struct virtio_balloon_config,
                             free_page_hint_cmd_id,
                             &vb->cmd_id_received_cache);

        return vb->cmd_id_received_cache;
}


So guest assumes native, host assumes LE.



> > ---
> >  drivers/virtio/virtio_balloon.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/drivers/virtio/virtio_balloon.c
b/drivers/virtio/virtio_balloon.c
> > index 5d4b891bf84f..b9bc03345157 100644
> > --- a/drivers/virtio/virtio_balloon.c
> > +++ b/drivers/virtio/virtio_balloon.c
> > @@ -1107,6 +1107,15 @@ static int virtballoon_restore(struct
virtio_device *vdev)
> >
> >  static int virtballoon_validate(struct virtio_device *vdev)
> >  {
> > +       /*
> > +        * Legacy devices never specified how modern features should
behave.
> > +        * E.g. which endian-ness to use? Better not to assume
anything.
> > +        */
> > +       if (!virtio_has_feature(vdev, VIRTIO_F_VERSION_1)) {
> > +               __virtio_clear_bit(vdev,
VIRTIO_BALLOON_F_FREE_PAGE_HINT);
> > +               __virtio_clear_bit(vdev,
VIRTIO_BALLOON_F_PAGE_POISON);
> > +               __virtio_clear_bit(vdev, VIRTIO_BALLOON_F_REPORTING);
> > +       }
> >         /*
> >          * Inform the hypervisor that our pages are poisoned or
> >          * initialized. If we cannot do that then we should disable
> 
> The patch content itself I am fine with since odds are nobody would
> expect to use these features with a legacy device.
> 
> Acked-by: Alexander Duyck <alexander.h.duyck at linux.intel.com>
Hmm so now you pointed out it's just cmd id, maybe I should just fix it
instead? what do you say?

-- 
MST

Jason Wang

2020-Jul-13 03:36 UTC

head link

[PATCH] virtio_balloon: clear modern features under legacy

On 2020/7/10 ??7:31, Michael S. Tsirkin wrote:> Page reporting features were never supported by legacy hypervisors.
> Supporting them poses a problem: should we use native endian-ness (like
> current code assumes)? Or little endian-ness like the virtio spec says?
> Rather than try to figure out, and since results of
> incorrect endian-ness are dire, let's just block this configuration.
>
> Cc: stable at vger.kernel.org
> Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
> ---
>   drivers/virtio/virtio_balloon.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
>
> diff --git a/drivers/virtio/virtio_balloon.c
b/drivers/virtio/virtio_balloon.c
> index 5d4b891bf84f..b9bc03345157 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -1107,6 +1107,15 @@ static int virtballoon_restore(struct virtio_device
*vdev)
>   
>   static int virtballoon_validate(struct virtio_device *vdev)
>   {
> +	/*
> +	 * Legacy devices never specified how modern features should behave.
> +	 * E.g. which endian-ness to use? Better not to assume anything.
> +	 */
> +	if (!virtio_has_feature(vdev, VIRTIO_F_VERSION_1)) {
> +		__virtio_clear_bit(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT);
> +		__virtio_clear_bit(vdev, VIRTIO_BALLOON_F_PAGE_POISON);
> +		__virtio_clear_bit(vdev, VIRTIO_BALLOON_F_REPORTING);
> +	}
>   	/*
>   	 * Inform the hypervisor that our pages are poisoned or
>   	 * initialized. If we cannot do that then we should disable

Acked-by: Jason Wang <jasowang at redhat.com>

Possibly Parallel Threads

Search for more reasonably related threads

Linux Virtualization - Jul 2020 - [PATCH] virtio_balloon: clear modern features under legacy

[PATCH] virtio_balloon: clear modern features under legacy

[PATCH] virtio_balloon: clear modern features under legacy

[PATCH] virtio_balloon: clear modern features under legacy

[PATCH] virtio_balloon: clear modern features under legacy

[PATCH] virtio_balloon: clear modern features under legacy

Possibly Parallel Threads