[v1] Vhost-vdpa Shadow Virtqueue VLAN support

[PATCH 3/3] vdpa: Support VLAN on nic control shadow virtqueue

Posted by Eugenio Pérez 3 years, 3 months ago

Update the virtio-net device model with each guest's update of vlan
through control virtqueue, and accept creating a SVQ with a device
exposing vlan feature bit.

Done in the same commit since a malicious guest could send vlan
commands otherwise.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 net/vhost-vdpa.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index ecbfd08eb9..40f7c60399 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -94,6 +94,7 @@ static const uint64_t vdpa_svq_device_features =
     BIT_ULL(VIRTIO_NET_F_MRG_RXBUF) |
     BIT_ULL(VIRTIO_NET_F_STATUS) |
     BIT_ULL(VIRTIO_NET_F_CTRL_VQ) |
+    BIT_ULL(VIRTIO_NET_F_CTRL_VLAN) |
     BIT_ULL(VIRTIO_NET_F_MQ) |
     BIT_ULL(VIRTIO_F_ANY_LAYOUT) |
     BIT_ULL(VIRTIO_NET_F_CTRL_MAC_ADDR) |
@@ -538,6 +539,16 @@ static bool vhost_vdpa_net_cvq_validate_cmd(const void *out_buf, size_t len)
                           __func__, ctrl.cmd);
         };
         break;
+    case VIRTIO_NET_CTRL_VLAN:
+        switch (ctrl->cmd) {
+        case VIRTIO_NET_CTRL_VLAN_ADD:
+        case VIRTIO_NET_CTRL_VLAN_DEL:
+            return true;
+        default:
+            qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid vlan cmd %u\n",
+                          __func__, ctrl->cmd);
+        };
+        break;
     default:
         qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid control class %u\n",
                       __func__, ctrl.class);
-- 
2.31.1

Re: [PATCH 3/3] vdpa: Support VLAN on nic control shadow virtqueue

Posted by Jason Wang 3 years, 3 months ago

On Wed, Sep 7, 2022 at 12:36 AM Eugenio Pérez <eperezma@redhat.com> wrote:
>
> Update the virtio-net device model with each guest's update of vlan
> through control virtqueue, and accept creating a SVQ with a device
> exposing vlan feature bit.
>
> Done in the same commit since a malicious guest could send vlan
> commands otherwise.
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>  net/vhost-vdpa.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index ecbfd08eb9..40f7c60399 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -94,6 +94,7 @@ static const uint64_t vdpa_svq_device_features =
>      BIT_ULL(VIRTIO_NET_F_MRG_RXBUF) |
>      BIT_ULL(VIRTIO_NET_F_STATUS) |
>      BIT_ULL(VIRTIO_NET_F_CTRL_VQ) |
> +    BIT_ULL(VIRTIO_NET_F_CTRL_VLAN) |
>      BIT_ULL(VIRTIO_NET_F_MQ) |
>      BIT_ULL(VIRTIO_F_ANY_LAYOUT) |
>      BIT_ULL(VIRTIO_NET_F_CTRL_MAC_ADDR) |
> @@ -538,6 +539,16 @@ static bool vhost_vdpa_net_cvq_validate_cmd(const void *out_buf, size_t len)
>                            __func__, ctrl.cmd);
>          };
>          break;
> +    case VIRTIO_NET_CTRL_VLAN:
> +        switch (ctrl->cmd) {
> +        case VIRTIO_NET_CTRL_VLAN_ADD:
> +        case VIRTIO_NET_CTRL_VLAN_DEL:
> +            return true;
> +        default:
> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid vlan cmd %u\n",
> +                          __func__, ctrl->cmd);
> +        };

Considering we may add more features here, is it still worthwhile to
keep a whitelist like this?

Thanks

> +        break;
>      default:
>          qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid control class %u\n",
>                        __func__, ctrl.class);
> --
> 2.31.1
>

Re: [PATCH 3/3] vdpa: Support VLAN on nic control shadow virtqueue

Posted by Eugenio Perez Martin 3 years, 3 months ago

On Fri, Sep 9, 2022 at 8:39 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Wed, Sep 7, 2022 at 12:36 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> >
> > Update the virtio-net device model with each guest's update of vlan
> > through control virtqueue, and accept creating a SVQ with a device
> > exposing vlan feature bit.
> >
> > Done in the same commit since a malicious guest could send vlan
> > commands otherwise.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >  net/vhost-vdpa.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > index ecbfd08eb9..40f7c60399 100644
> > --- a/net/vhost-vdpa.c
> > +++ b/net/vhost-vdpa.c
> > @@ -94,6 +94,7 @@ static const uint64_t vdpa_svq_device_features =
> >      BIT_ULL(VIRTIO_NET_F_MRG_RXBUF) |
> >      BIT_ULL(VIRTIO_NET_F_STATUS) |
> >      BIT_ULL(VIRTIO_NET_F_CTRL_VQ) |
> > +    BIT_ULL(VIRTIO_NET_F_CTRL_VLAN) |
> >      BIT_ULL(VIRTIO_NET_F_MQ) |
> >      BIT_ULL(VIRTIO_F_ANY_LAYOUT) |
> >      BIT_ULL(VIRTIO_NET_F_CTRL_MAC_ADDR) |
> > @@ -538,6 +539,16 @@ static bool vhost_vdpa_net_cvq_validate_cmd(const void *out_buf, size_t len)
> >                            __func__, ctrl.cmd);
> >          };
> >          break;
> > +    case VIRTIO_NET_CTRL_VLAN:
> > +        switch (ctrl->cmd) {

Rebase mistake by my side: This must be ctrl.cmd.

> > +        case VIRTIO_NET_CTRL_VLAN_ADD:
> > +        case VIRTIO_NET_CTRL_VLAN_DEL:
> > +            return true;
> > +        default:
> > +            qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid vlan cmd %u\n",
> > +                          __func__, ctrl->cmd);

Same here (s/ctrl->cmd/ctrl.cmd/).


> > +        };
>
> Considering we may add more features here, is it still worthwhile to
> keep a whitelist like this?
>

I guess we can remove it, let me test without it.

Thanks!

> Thanks
>
> > +        break;
> >      default:
> >          qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid control class %u\n",
> >                        __func__, ctrl.class);
> > --
> > 2.31.1
> >
>