Received-SPF: pass (google.com: domain of linux-kernel+bounces-19072-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240104153753.2931026-1-maxime.coquelin@redhat.com>
 <20240104153753.2931026-3-maxime.coquelin@redhat.com> <CACGkMEtmTY0ux8pw8VQ8SAdgGty=rM1VRkh6c-qBVSaqhYuURw@mail.gmail.com>
 <888255d7-e45b-44db-8561-5e9f386603c3@redhat.com> <CAJaqyWfrO3c9GD2k+aX=fA8OQLg2aQPHHb4WyTbE09KkmoOesA@mail.gmail.com>
 <0322ee9e-f6db-45ed-855f-9ebcdfca30a9@redhat.com>
In-Reply-To: <0322ee9e-f6db-45ed-855f-9ebcdfca30a9@redhat.com>
From: Jason Wang <jasowang@redhat.com>
Date: Mon, 8 Jan 2024 11:44:26 +0800
Message-ID: <CACGkMEuYVw0QvbmNCQ-43bn1nhVH4OS7tTpf0a-mvmOmvXaPkQ@mail.gmail.com>
Subject: Re: [PATCH v6 2/3] vduse: Temporarily fail if control queue features requested
To: Maxime Coquelin <maxime.coquelin@redhat.com>
Cc: Eugenio Perez Martin <eperezma@redhat.com>, mst@redhat.com, xuanzhuo@linux.alibaba.com, 
	xieyongji@bytedance.com, virtualization@lists.linux-foundation.org, 
	linux-kernel@vger.kernel.org, david.marchand@redhat.com, lulu@redhat.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Jan 5, 2024 at 6:14=E2=80=AFPM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
>
>
>
> On 1/5/24 10:59, Eugenio Perez Martin wrote:
> > On Fri, Jan 5, 2024 at 9:12=E2=80=AFAM Maxime Coquelin
> > <maxime.coquelin@redhat.com> wrote:
> >>
> >>
> >>
> >> On 1/5/24 03:45, Jason Wang wrote:
> >>> On Thu, Jan 4, 2024 at 11:38=E2=80=AFPM Maxime Coquelin
> >>> <maxime.coquelin@redhat.com> wrote:
> >>>>
> >>>> Virtio-net driver control queue implementation is not safe
> >>>> when used with VDUSE. If the VDUSE application does not
> >>>> reply to control queue messages, it currently ends up
> >>>> hanging the kernel thread sending this command.
> >>>>
> >>>> Some work is on-going to make the control queue
> >>>> implementation robust with VDUSE. Until it is completed,
> >>>> let's fail features check if any control-queue related
> >>>> feature is requested.
> >>>>
> >>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>>> ---
> >>>>    drivers/vdpa/vdpa_user/vduse_dev.c | 13 +++++++++++++
> >>>>    1 file changed, 13 insertions(+)
> >>>>
> >>>> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_=
user/vduse_dev.c
> >>>> index 0486ff672408..94f54ea2eb06 100644
> >>>> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> >>>> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> >>>> @@ -28,6 +28,7 @@
> >>>>    #include <uapi/linux/virtio_config.h>
> >>>>    #include <uapi/linux/virtio_ids.h>
> >>>>    #include <uapi/linux/virtio_blk.h>
> >>>> +#include <uapi/linux/virtio_ring.h>
> >>>>    #include <linux/mod_devicetable.h>
> >>>>
> >>>>    #include "iova_domain.h"
> >>>> @@ -46,6 +47,15 @@
> >>>>
> >>>>    #define IRQ_UNBOUND -1
> >>>>
> >>>> +#define VDUSE_NET_INVALID_FEATURES_MASK         \
> >>>> +       (BIT_ULL(VIRTIO_NET_F_CTRL_VQ) |        \
> >>>> +        BIT_ULL(VIRTIO_NET_F_CTRL_RX)   |      \
> >>>> +        BIT_ULL(VIRTIO_NET_F_CTRL_VLAN) |      \
> >>>> +        BIT_ULL(VIRTIO_NET_F_GUEST_ANNOUNCE) | \
> >>>> +        BIT_ULL(VIRTIO_NET_F_MQ) |             \
> >>>> +        BIT_ULL(VIRTIO_NET_F_CTRL_MAC_ADDR) |  \
> >>>> +        BIT_ULL(VIRTIO_NET_F_RSS))
> >>>
> >>> We need to make this as well:
> >>>
> >>> VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
> >>
> >> I missed it, and see others have been added in the Virtio spec
> >> repository (BTW, I see this specific one is missing in the dependency
> >> list [0], I will submit a patch).
> >>
> >> I wonder if it is not just simpler to just check for
> >> VIRTIO_NET_F_CTRL_VQ is requested. As we fail instead of masking out,
> >> the VDUSE driver won't be the one violating the spec so it should be
> >> good?
> >>
> >> It will avoid having to update the mask if new features depending on i=
t
> >> are added (or forgetting to update it).
> >>
> >> WDYT?
> >>
> >
> > I think it is safer to work with a whitelist, instead of a blacklist.
> > As any new feature might require code changes in QEMU. Is that
> > possible?
>
> Well, that's how it was done in previous revision. :)
>
> I changed to a blacklist for consistency with block device's WCE feature
> check after Jason's comment.
>
> I'm not sure moving back to a whitelist brings much advantages when
> compared to the effort of keeping it up to date. Just blacklisting
> VIRTIO_NET_F_CTRL_VQ is enough in my opinion.

I think this makes sense.

Thanks

>
> Thanks,
> Maxime
>
> >> Thanks,
> >> Maxime
> >>
> >> [0]:
> >> https://github.com/oasis-tcs/virtio-spec/blob/5fc35a7efb903fc352da81a6=
d2be5c01810b68d3/device-types/net/description.tex#L129
> >>> Other than this,
> >>>
> >>> Acked-by: Jason Wang <jasowang@redhat.com>
> >>>
> >>> Thanks
> >>>
> >>>> +
> >>>>    struct vduse_virtqueue {
> >>>>           u16 index;
> >>>>           u16 num_max;
> >>>> @@ -1680,6 +1690,9 @@ static bool features_is_valid(struct vduse_dev=
_config *config)
> >>>>           if ((config->device_id =3D=3D VIRTIO_ID_BLOCK) &&
> >>>>                           (config->features & (1ULL << VIRTIO_BLK_F_=
CONFIG_WCE)))
> >>>>                   return false;
> >>>> +       else if ((config->device_id =3D=3D VIRTIO_ID_NET) &&
> >>>> +                       (config->features & VDUSE_NET_INVALID_FEATUR=
ES_MASK))
> >>>> +               return false;
> >>>>
> >>>>           return true;
> >>>>    }
> >>>> --
> >>>> 2.43.0
> >>>>
> >>>
> >>
> >>
> >
>