Re: [PATCH v6 2/3] vduse: Temporarily fail if control queue features requested

[Maxime Coquelin]

On 1/5/24 10:59, Eugenio Perez Martin wrote:
> On Fri, Jan 5, 2024 at 9:12 AM Maxime Coquelin
> <maxime.coquelin@xxxxxxxxxx> wrote:
> > On 1/5/24 03:45, Jason Wang wrote:
> > > On Thu, Jan 4, 2024 at 11:38 PM Maxime Coquelin
> > > <maxime.coquelin@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > > ---
> > > >    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 it
> > 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.

Thanks,
Maxime

> > Thanks,
> > Maxime
> >
> > [0]:
> > https://github.com/oasis-tcs/virtio-spec/blob/5fc35a7efb903fc352da81a6d2be5c01810b68d3/device-types/net/description.tex#L129
> > > Other than this,
> > >
> > > Acked-by: Jason Wang <jasowang@xxxxxxxxxx>
> > >
> > > 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 == VIRTIO_ID_BLOCK) &&
> > > >                           (config->features & (1ULL << VIRTIO_BLK_F_CONFIG_WCE)))
> > > >                   return false;
> > > > +       else if ((config->device_id == VIRTIO_ID_NET) &&
> > > > +                       (config->features & VDUSE_NET_INVALID_FEATURES_MASK))
> > > > +               return false;
> > > >
> > > >           return true;
> > > >    }
> > > > --
> > > > 2.43.0