On Fri, Jan 5, 2024 at 6:14 PM Maxime Coquelin <maxime.coquelin@xxxxxxxxxx> wrote: > > > > 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. I think this makes sense. Thanks > > 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 > >>>> > >>> > >> > >> > > >
