Linux Virtualization - Jul 2020 - [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

On Sun, Jul 12, 2020 at 8:10 AM Michael S. Tsirkin <mst at redhat.com>
wrote:
>
> 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.
So we are safe assuming it is a repeating value, but for correctness
maybe we should make certain to cast this as a le32 value. I can
submit a patch to do that.
> 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.
This wasn't even the one I was talking about, but now that you point
it out this is definately bug. The command ID I was talking about was
the one being passed via the descriptor ring. That one I believe is
native on both sides.
>
>
>
> > > ---
> > >  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?
So the config issues are bugs, but I don't think you saw the one I was
talking about. In the function send_cmd_id_start the cmd_id_active
value which is initialized as a virtio32 is added as a sg entry and
then sent as an outbuf to the device. I'm assuming virtio32 is a host
native byte ordering.

On Mon, Jul 13, 2020 at 08:10:14AM -0700, Alexander Duyck
wrote:
> On Sun, Jul 12, 2020 at 8:10 AM Michael S. Tsirkin <mst at
redhat.com> wrote:
> >
> > 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.
> 
> So we are safe assuming it is a repeating value, but for correctness
> maybe we should make certain to cast this as a le32 value. I can
> submit a patch to do that.
Thanks! But not yet - I am poking at the endian-ness things right now!
> > 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.
> 
> This wasn't even the one I was talking about, but now that you point
> it out this is definately bug. The command ID I was talking about was
> the one being passed via the descriptor ring. That one I believe is
> native on both sides.
Well qemu swaps it for modern devices:

        virtio_tswap32s(vdev, &id);

guest swaps it too:
        vb->cmd_id_active = cpu_to_virtio32(vb->vdev,
                                        virtio_balloon_cmd_id_received(vb));
        sg_init_one(&sg, &vb->cmd_id_active,
sizeof(vb->cmd_id_active));
        err = virtqueue_add_outbuf(vq, &sg, 1, &vb->cmd_id_active,
GFP_KERNEL);

So it's native for legacy.


> >
> >
> >
> > > > ---
> > > >  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?
> 
> So the config issues are bugs, but I don't think you saw the one I was
> talking about. In the function send_cmd_id_start the cmd_id_active
> value which is initialized as a virtio32 is added as a sg entry and
> then sent as an outbuf to the device. I'm assuming virtio32 is a host
> native byte ordering.
IIUC it isn't :) virtio32 is guest native if device is legacy, and LE if
device is modern.

-- 
MST