diff mbox series

[v5,2/4] vduse: Temporarily disable control queue features

Message ID 20231212131712.1816324-3-maxime.coquelin@redhat.com (mailing list archive)
State Rejected
Delegated to: Paul Moore
Headers show
Series vduse: add support for networking devices | expand

Commit Message

Maxime Coquelin Dec. 12, 2023, 1:17 p.m. UTC
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 disable control virtqueue and features that depend on
it.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/vdpa/vdpa_user/vduse_dev.c | 37 ++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

Comments

Jason Wang Dec. 13, 2023, 4:52 a.m. UTC | #1
On Tue, Dec 12, 2023 at 9:17 PM 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 disable control virtqueue and features that depend on
> it.
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>

I wonder if it's better to fail instead of a mask as a start.

Thanks

> ---
>  drivers/vdpa/vdpa_user/vduse_dev.c | 37 ++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> index 0486ff672408..fe4b5c8203fd 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,30 @@
>
>  #define IRQ_UNBOUND -1
>
> +#define VDUSE_NET_VALID_FEATURES_MASK           \
> +       (BIT_ULL(VIRTIO_NET_F_CSUM) |           \
> +        BIT_ULL(VIRTIO_NET_F_GUEST_CSUM) |     \
> +        BIT_ULL(VIRTIO_NET_F_MTU) |            \
> +        BIT_ULL(VIRTIO_NET_F_MAC) |            \
> +        BIT_ULL(VIRTIO_NET_F_GUEST_TSO4) |     \
> +        BIT_ULL(VIRTIO_NET_F_GUEST_TSO6) |     \
> +        BIT_ULL(VIRTIO_NET_F_GUEST_ECN) |      \
> +        BIT_ULL(VIRTIO_NET_F_GUEST_UFO) |      \
> +        BIT_ULL(VIRTIO_NET_F_HOST_TSO4) |      \
> +        BIT_ULL(VIRTIO_NET_F_HOST_TSO6) |      \
> +        BIT_ULL(VIRTIO_NET_F_HOST_ECN) |       \
> +        BIT_ULL(VIRTIO_NET_F_HOST_UFO) |       \
> +        BIT_ULL(VIRTIO_NET_F_MRG_RXBUF) |      \
> +        BIT_ULL(VIRTIO_NET_F_STATUS) |         \
> +        BIT_ULL(VIRTIO_NET_F_HOST_USO) |       \
> +        BIT_ULL(VIRTIO_F_ANY_LAYOUT) |         \
> +        BIT_ULL(VIRTIO_RING_F_INDIRECT_DESC) | \
> +        BIT_ULL(VIRTIO_RING_F_EVENT_IDX) |          \
> +        BIT_ULL(VIRTIO_F_VERSION_1) |          \
> +        BIT_ULL(VIRTIO_F_ACCESS_PLATFORM) |     \
> +        BIT_ULL(VIRTIO_F_RING_PACKED) |        \
> +        BIT_ULL(VIRTIO_F_IN_ORDER))
> +
>  struct vduse_virtqueue {
>         u16 index;
>         u16 num_max;
> @@ -1782,6 +1807,16 @@ static struct attribute *vduse_dev_attrs[] = {
>
>  ATTRIBUTE_GROUPS(vduse_dev);
>
> +static void vduse_dev_features_filter(struct vduse_dev_config *config)
> +{
> +       /*
> +        * Temporarily filter out virtio-net's control virtqueue and features
> +        * that depend on it while CVQ is being made more robust for VDUSE.
> +        */
> +       if (config->device_id == VIRTIO_ID_NET)
> +               config->features &= VDUSE_NET_VALID_FEATURES_MASK;
> +}
> +
>  static int vduse_create_dev(struct vduse_dev_config *config,
>                             void *config_buf, u64 api_version)
>  {
> @@ -1797,6 +1832,8 @@ static int vduse_create_dev(struct vduse_dev_config *config,
>         if (!dev)
>                 goto err;
>
> +       vduse_dev_features_filter(config);
> +
>         dev->api_version = api_version;
>         dev->device_features = config->features;
>         dev->device_id = config->device_id;
> --
> 2.43.0
>
Maxime Coquelin Dec. 13, 2023, 11:23 a.m. UTC | #2
Hi Jason,

On 12/13/23 05:52, Jason Wang wrote:
> On Tue, Dec 12, 2023 at 9:17 PM 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 disable control virtqueue and features that depend on
>> it.
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> 
> I wonder if it's better to fail instead of a mask as a start.

I think it is better to use a mask and not fail, so that we can in the
future use a recent VDUSE application with an older kernel.

Why would it be better to fail than negotiating?

Thanks,
Maxime

> Thanks
> 
>> ---
>>   drivers/vdpa/vdpa_user/vduse_dev.c | 37 ++++++++++++++++++++++++++++++
>>   1 file changed, 37 insertions(+)
>>
>> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
>> index 0486ff672408..fe4b5c8203fd 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,30 @@
>>
>>   #define IRQ_UNBOUND -1
>>
>> +#define VDUSE_NET_VALID_FEATURES_MASK           \
>> +       (BIT_ULL(VIRTIO_NET_F_CSUM) |           \
>> +        BIT_ULL(VIRTIO_NET_F_GUEST_CSUM) |     \
>> +        BIT_ULL(VIRTIO_NET_F_MTU) |            \
>> +        BIT_ULL(VIRTIO_NET_F_MAC) |            \
>> +        BIT_ULL(VIRTIO_NET_F_GUEST_TSO4) |     \
>> +        BIT_ULL(VIRTIO_NET_F_GUEST_TSO6) |     \
>> +        BIT_ULL(VIRTIO_NET_F_GUEST_ECN) |      \
>> +        BIT_ULL(VIRTIO_NET_F_GUEST_UFO) |      \
>> +        BIT_ULL(VIRTIO_NET_F_HOST_TSO4) |      \
>> +        BIT_ULL(VIRTIO_NET_F_HOST_TSO6) |      \
>> +        BIT_ULL(VIRTIO_NET_F_HOST_ECN) |       \
>> +        BIT_ULL(VIRTIO_NET_F_HOST_UFO) |       \
>> +        BIT_ULL(VIRTIO_NET_F_MRG_RXBUF) |      \
>> +        BIT_ULL(VIRTIO_NET_F_STATUS) |         \
>> +        BIT_ULL(VIRTIO_NET_F_HOST_USO) |       \
>> +        BIT_ULL(VIRTIO_F_ANY_LAYOUT) |         \
>> +        BIT_ULL(VIRTIO_RING_F_INDIRECT_DESC) | \
>> +        BIT_ULL(VIRTIO_RING_F_EVENT_IDX) |          \
>> +        BIT_ULL(VIRTIO_F_VERSION_1) |          \
>> +        BIT_ULL(VIRTIO_F_ACCESS_PLATFORM) |     \
>> +        BIT_ULL(VIRTIO_F_RING_PACKED) |        \
>> +        BIT_ULL(VIRTIO_F_IN_ORDER))
>> +
>>   struct vduse_virtqueue {
>>          u16 index;
>>          u16 num_max;
>> @@ -1782,6 +1807,16 @@ static struct attribute *vduse_dev_attrs[] = {
>>
>>   ATTRIBUTE_GROUPS(vduse_dev);
>>
>> +static void vduse_dev_features_filter(struct vduse_dev_config *config)
>> +{
>> +       /*
>> +        * Temporarily filter out virtio-net's control virtqueue and features
>> +        * that depend on it while CVQ is being made more robust for VDUSE.
>> +        */
>> +       if (config->device_id == VIRTIO_ID_NET)
>> +               config->features &= VDUSE_NET_VALID_FEATURES_MASK;
>> +}
>> +
>>   static int vduse_create_dev(struct vduse_dev_config *config,
>>                              void *config_buf, u64 api_version)
>>   {
>> @@ -1797,6 +1832,8 @@ static int vduse_create_dev(struct vduse_dev_config *config,
>>          if (!dev)
>>                  goto err;
>>
>> +       vduse_dev_features_filter(config);
>> +
>>          dev->api_version = api_version;
>>          dev->device_features = config->features;
>>          dev->device_id = config->device_id;
>> --
>> 2.43.0
>>
>
Jason Wang Dec. 18, 2023, 2:50 a.m. UTC | #3
On Wed, Dec 13, 2023 at 7:23 PM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
>
> Hi Jason,
>
> On 12/13/23 05:52, Jason Wang wrote:
> > On Tue, Dec 12, 2023 at 9:17 PM 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 disable control virtqueue and features that depend on
> >> it.
> >>
> >> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> >
> > I wonder if it's better to fail instead of a mask as a start.
>
> I think it is better to use a mask and not fail, so that we can in the
> future use a recent VDUSE application with an older kernel.

It may confuse the userspace unless userspace can do post check after
CREATE_DEV.

And for blk we fail when WCE is set in feature_is_valid():

static bool features_is_valid(u64 features)
{
        if (!(features & (1ULL << VIRTIO_F_ACCESS_PLATFORM)))
                return false;

        /* Now we only support read-only configuration space */
        if (features & (1ULL << VIRTIO_BLK_F_CONFIG_WCE))
                return false;

        return true;
}

Thanks

>
> Why would it be better to fail than negotiating?
>
> Thanks,
> Maxime
>
Maxime Coquelin Dec. 18, 2023, 9:21 a.m. UTC | #4
On 12/18/23 03:50, Jason Wang wrote:
> On Wed, Dec 13, 2023 at 7:23 PM Maxime Coquelin
> <maxime.coquelin@redhat.com> wrote:
>>
>> Hi Jason,
>>
>> On 12/13/23 05:52, Jason Wang wrote:
>>> On Tue, Dec 12, 2023 at 9:17 PM 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 disable control virtqueue and features that depend on
>>>> it.
>>>>
>>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>
>>> I wonder if it's better to fail instead of a mask as a start.
>>
>> I think it is better to use a mask and not fail, so that we can in the
>> future use a recent VDUSE application with an older kernel.
> 
> It may confuse the userspace unless userspace can do post check after
> CREATE_DEV.
> 
> And for blk we fail when WCE is set in feature_is_valid():
> 
> static bool features_is_valid(u64 features)
> {
>          if (!(features & (1ULL << VIRTIO_F_ACCESS_PLATFORM)))
>                  return false;
> 
>          /* Now we only support read-only configuration space */
>          if (features & (1ULL << VIRTIO_BLK_F_CONFIG_WCE))
>                  return false;
> 
>          return true;
> }

Ok, consistency with other devices types is indeed better.

But should I fail if any of the feature advertised by the application is
not listed by the VDUSE driver, or just fail if control queue is being
advertised by the application?

Thanks,
Maxime

> Thanks
> 
>>
>> Why would it be better to fail than negotiating?
>>
>> Thanks,
>> Maxime
>>
>
Jason Wang Dec. 20, 2023, 3:50 a.m. UTC | #5
On Mon, Dec 18, 2023 at 5:21 PM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
>
>
>
> On 12/18/23 03:50, Jason Wang wrote:
> > On Wed, Dec 13, 2023 at 7:23 PM Maxime Coquelin
> > <maxime.coquelin@redhat.com> wrote:
> >>
> >> Hi Jason,
> >>
> >> On 12/13/23 05:52, Jason Wang wrote:
> >>> On Tue, Dec 12, 2023 at 9:17 PM 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 disable control virtqueue and features that depend on
> >>>> it.
> >>>>
> >>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>>
> >>> I wonder if it's better to fail instead of a mask as a start.
> >>
> >> I think it is better to use a mask and not fail, so that we can in the
> >> future use a recent VDUSE application with an older kernel.
> >
> > It may confuse the userspace unless userspace can do post check after
> > CREATE_DEV.
> >
> > And for blk we fail when WCE is set in feature_is_valid():
> >
> > static bool features_is_valid(u64 features)
> > {
> >          if (!(features & (1ULL << VIRTIO_F_ACCESS_PLATFORM)))
> >                  return false;
> >
> >          /* Now we only support read-only configuration space */
> >          if (features & (1ULL << VIRTIO_BLK_F_CONFIG_WCE))
> >                  return false;
> >
> >          return true;
> > }
>
> Ok, consistency with other devices types is indeed better.
>
> But should I fail if any of the feature advertised by the application is
> not listed by the VDUSE driver, or just fail if control queue is being
> advertised by the application?

Maybe it's better to fail for any other of the features that depend on
the control vq.

Thanks

>
> Thanks,
> Maxime
>
> > Thanks
> >
> >>
> >> Why would it be better to fail than negotiating?
> >>
> >> Thanks,
> >> Maxime
> >>
> >
>
diff mbox series

Patch

diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
index 0486ff672408..fe4b5c8203fd 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,30 @@ 
 
 #define IRQ_UNBOUND -1
 
+#define VDUSE_NET_VALID_FEATURES_MASK           \
+	(BIT_ULL(VIRTIO_NET_F_CSUM) |           \
+	 BIT_ULL(VIRTIO_NET_F_GUEST_CSUM) |     \
+	 BIT_ULL(VIRTIO_NET_F_MTU) |            \
+	 BIT_ULL(VIRTIO_NET_F_MAC) |            \
+	 BIT_ULL(VIRTIO_NET_F_GUEST_TSO4) |     \
+	 BIT_ULL(VIRTIO_NET_F_GUEST_TSO6) |     \
+	 BIT_ULL(VIRTIO_NET_F_GUEST_ECN) |      \
+	 BIT_ULL(VIRTIO_NET_F_GUEST_UFO) |      \
+	 BIT_ULL(VIRTIO_NET_F_HOST_TSO4) |      \
+	 BIT_ULL(VIRTIO_NET_F_HOST_TSO6) |      \
+	 BIT_ULL(VIRTIO_NET_F_HOST_ECN) |       \
+	 BIT_ULL(VIRTIO_NET_F_HOST_UFO) |       \
+	 BIT_ULL(VIRTIO_NET_F_MRG_RXBUF) |      \
+	 BIT_ULL(VIRTIO_NET_F_STATUS) |         \
+	 BIT_ULL(VIRTIO_NET_F_HOST_USO) |       \
+	 BIT_ULL(VIRTIO_F_ANY_LAYOUT) |         \
+	 BIT_ULL(VIRTIO_RING_F_INDIRECT_DESC) | \
+	 BIT_ULL(VIRTIO_RING_F_EVENT_IDX) |          \
+	 BIT_ULL(VIRTIO_F_VERSION_1) |          \
+	 BIT_ULL(VIRTIO_F_ACCESS_PLATFORM) |     \
+	 BIT_ULL(VIRTIO_F_RING_PACKED) |        \
+	 BIT_ULL(VIRTIO_F_IN_ORDER))
+
 struct vduse_virtqueue {
 	u16 index;
 	u16 num_max;
@@ -1782,6 +1807,16 @@  static struct attribute *vduse_dev_attrs[] = {
 
 ATTRIBUTE_GROUPS(vduse_dev);
 
+static void vduse_dev_features_filter(struct vduse_dev_config *config)
+{
+	/*
+	 * Temporarily filter out virtio-net's control virtqueue and features
+	 * that depend on it while CVQ is being made more robust for VDUSE.
+	 */
+	if (config->device_id == VIRTIO_ID_NET)
+		config->features &= VDUSE_NET_VALID_FEATURES_MASK;
+}
+
 static int vduse_create_dev(struct vduse_dev_config *config,
 			    void *config_buf, u64 api_version)
 {
@@ -1797,6 +1832,8 @@  static int vduse_create_dev(struct vduse_dev_config *config,
 	if (!dev)
 		goto err;
 
+	vduse_dev_features_filter(config);
+
 	dev->api_version = api_version;
 	dev->device_features = config->features;
 	dev->device_id = config->device_id;