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? > 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 > >> > > > >
