This small series enables virtio-net device type in VDUSE.
With it, basic operation have been tested, both with
virtio-vdpa and vhost-vdpa using DPDK Vhost library series
adding VDUSE support using split rings layout (merged in
DPDK v23.07-rc1).
Control queue support (and so multiqueue) has also been
tested, but requires a Kernel series from Jason Wang
relaxing control queue polling [1] to function reliably,
so while Jason rework is done, a patch is added to disable
CVQ and features that depend on it (tested also with DPDK
v23.07-rc1).
In this v5, LSM hooks introduced in previous revision are
unified into a single hook that covers below operations:
- VDUSE_CREATE_DEV ioctl on VDUSE control file,
- VDUSE_DESTROY_DEV ioctl on VDUSE control file,
- open() on VDUSE device file.
In combination with the operations permission, a device type
permission has to be associated:
- block: Virtio block device type,
- net: Virtio networking device type.
changes in v6!
==============
- Remove SELinux support from the series, will be handled
in a dedicated one.
- Require CAP_NET_ADMIN for Net devices creation (Jason).
- Fail init if control queue features are requested for
Net device type (Jason).
- Rebased on latest master.
Changes in v5:
==============
- Move control queue disablement patch before Net
devices enablement (Jason).
- Unify operations LSM hooks into a single hook.
- Rebase on latest master.
Maxime Coquelin (3):
vduse: validate block features only with block devices
vduse: Temporarily fail if control queue features requested
vduse: enable Virtio-net device type
drivers/vdpa/vdpa_user/vduse_dev.c | 32 ++++++++++++++++++++++++++----
1 file changed, 28 insertions(+), 4 deletions(-)
--
2.43.0
This patch is preliminary work to enable network device
type support to VDUSE.
As VIRTIO_BLK_F_CONFIG_WCE shares the same value as
VIRTIO_NET_F_HOST_TSO4, we need to restrict its check
to Virtio-blk device type.
Acked-by: Jason Wang <[email protected]>
Reviewed-by: Xie Yongji <[email protected]>
Signed-off-by: Maxime Coquelin <[email protected]>
---
drivers/vdpa/vdpa_user/vduse_dev.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
index 0ddd4b8abecb..0486ff672408 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -1671,13 +1671,14 @@ static bool device_is_allowed(u32 device_id)
return false;
}
-static bool features_is_valid(u64 features)
+static bool features_is_valid(struct vduse_dev_config *config)
{
- if (!(features & (1ULL << VIRTIO_F_ACCESS_PLATFORM)))
+ if (!(config->features & (1ULL << VIRTIO_F_ACCESS_PLATFORM)))
return false;
/* Now we only support read-only configuration space */
- if (features & (1ULL << VIRTIO_BLK_F_CONFIG_WCE))
+ if ((config->device_id == VIRTIO_ID_BLOCK) &&
+ (config->features & (1ULL << VIRTIO_BLK_F_CONFIG_WCE)))
return false;
return true;
@@ -1704,7 +1705,7 @@ static bool vduse_validate_config(struct vduse_dev_config *config)
if (!device_is_allowed(config->device_id))
return false;
- if (!features_is_valid(config->features))
+ if (!features_is_valid(config))
return false;
return true;
--
2.43.0
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 <[email protected]>
---
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))
+
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
This patch adds Virtio-net device type to the supported
devices types.
Initialization fails if the device does not support
VIRTIO_F_VERSION_1 feature, in order to guarantee the
configuration space is read-only. It also fails with
-EPERM if the CAP_NET_ADMIN is missing.
Signed-off-by: Maxime Coquelin <[email protected]>
---
drivers/vdpa/vdpa_user/vduse_dev.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
index 94f54ea2eb06..4057b34ff995 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -151,6 +151,7 @@ static struct workqueue_struct *vduse_irq_bound_wq;
static u32 allowed_device_id[] = {
VIRTIO_ID_BLOCK,
+ VIRTIO_ID_NET,
};
static inline struct vduse_dev *vdpa_to_vduse(struct vdpa_device *vdpa)
@@ -1694,6 +1695,10 @@ static bool features_is_valid(struct vduse_dev_config *config)
(config->features & VDUSE_NET_INVALID_FEATURES_MASK))
return false;
+ if ((config->device_id == VIRTIO_ID_NET) &&
+ !(config->features & (1ULL << VIRTIO_F_VERSION_1)))
+ return false;
+
return true;
}
@@ -1801,6 +1806,10 @@ static int vduse_create_dev(struct vduse_dev_config *config,
int ret;
struct vduse_dev *dev;
+ ret = -EPERM;
+ if ((config->device_id == VIRTIO_ID_NET) && !capable(CAP_NET_ADMIN))
+ goto err;
+
ret = -EEXIST;
if (vduse_find_dev(config->name))
goto err;
@@ -2044,6 +2053,7 @@ static const struct vdpa_mgmtdev_ops vdpa_dev_mgmtdev_ops = {
static struct virtio_device_id id_table[] = {
{ VIRTIO_ID_BLOCK, VIRTIO_DEV_ANY_ID },
+ { VIRTIO_ID_NET, VIRTIO_DEV_ANY_ID },
{ 0 },
};
--
2.43.0
On Thu, Jan 4, 2024 at 11:38 PM Maxime Coquelin
<[email protected]> 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 <[email protected]>
> ---
> 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
Other than this,
Acked-by: Jason Wang <[email protected]>
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
>
On Thu, Jan 4, 2024 at 11:38 PM Maxime Coquelin
<[email protected]> wrote:
>
> This patch adds Virtio-net device type to the supported
> devices types.
>
> Initialization fails if the device does not support
> VIRTIO_F_VERSION_1 feature, in order to guarantee the
> configuration space is read-only. It also fails with
> -EPERM if the CAP_NET_ADMIN is missing.
>
> Signed-off-by: Maxime Coquelin <[email protected]>
> ---
Acked-by: Jason Wang <[email protected]>
Thanks
On 1/5/24 03:45, Jason Wang wrote:
> On Thu, Jan 4, 2024 at 11:38 PM Maxime Coquelin
> <[email protected]> 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 <[email protected]>
>> ---
>> 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?
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 <[email protected]>
>
> 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
>>
>
On Thu, Jan 4, 2024 at 4:38 PM Maxime Coquelin
<[email protected]> wrote:
>
> This patch is preliminary work to enable network device
> type support to VDUSE.
>
> As VIRTIO_BLK_F_CONFIG_WCE shares the same value as
> VIRTIO_NET_F_HOST_TSO4, we need to restrict its check
> to Virtio-blk device type.
>
> Acked-by: Jason Wang <[email protected]>
> Reviewed-by: Xie Yongji <[email protected]>
> Signed-off-by: Maxime Coquelin <[email protected]>
Reviewed-by: Eugenio Pérez <[email protected]>
> ---
> drivers/vdpa/vdpa_user/vduse_dev.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> index 0ddd4b8abecb..0486ff672408 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -1671,13 +1671,14 @@ static bool device_is_allowed(u32 device_id)
> return false;
> }
>
> -static bool features_is_valid(u64 features)
> +static bool features_is_valid(struct vduse_dev_config *config)
> {
> - if (!(features & (1ULL << VIRTIO_F_ACCESS_PLATFORM)))
> + if (!(config->features & (1ULL << VIRTIO_F_ACCESS_PLATFORM)))
> return false;
>
> /* Now we only support read-only configuration space */
> - if (features & (1ULL << VIRTIO_BLK_F_CONFIG_WCE))
> + if ((config->device_id == VIRTIO_ID_BLOCK) &&
> + (config->features & (1ULL << VIRTIO_BLK_F_CONFIG_WCE)))
> return false;
>
> return true;
> @@ -1704,7 +1705,7 @@ static bool vduse_validate_config(struct vduse_dev_config *config)
> if (!device_is_allowed(config->device_id))
> return false;
>
> - if (!features_is_valid(config->features))
> + if (!features_is_valid(config))
> return false;
>
> return true;
> --
> 2.43.0
>
>
On Fri, Jan 5, 2024 at 9:12 AM Maxime Coquelin
<[email protected]> wrote:
>
>
>
> On 1/5/24 03:45, Jason Wang wrote:
> > On Thu, Jan 4, 2024 at 11:38 PM Maxime Coquelin
> > <[email protected]> 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 <[email protected]>
> >> ---
> >> 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 <[email protected]>
> >
> > 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
> >>
> >
>
>
On Thu, Jan 4, 2024 at 4:39 PM Maxime Coquelin
<[email protected]> wrote:
>
> This patch adds Virtio-net device type to the supported
> devices types.
>
> Initialization fails if the device does not support
> VIRTIO_F_VERSION_1 feature, in order to guarantee the
> configuration space is read-only. It also fails with
> -EPERM if the CAP_NET_ADMIN is missing.
>
> Signed-off-by: Maxime Coquelin <[email protected]>
Reviewed-by: Eugenio Pérez <[email protected]>
> ---
> drivers/vdpa/vdpa_user/vduse_dev.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> index 94f54ea2eb06..4057b34ff995 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -151,6 +151,7 @@ static struct workqueue_struct *vduse_irq_bound_wq;
>
> static u32 allowed_device_id[] = {
> VIRTIO_ID_BLOCK,
> + VIRTIO_ID_NET,
> };
>
> static inline struct vduse_dev *vdpa_to_vduse(struct vdpa_device *vdpa)
> @@ -1694,6 +1695,10 @@ static bool features_is_valid(struct vduse_dev_config *config)
> (config->features & VDUSE_NET_INVALID_FEATURES_MASK))
> return false;
>
> + if ((config->device_id == VIRTIO_ID_NET) &&
> + !(config->features & (1ULL << VIRTIO_F_VERSION_1)))
> + return false;
> +
> return true;
> }
>
> @@ -1801,6 +1806,10 @@ static int vduse_create_dev(struct vduse_dev_config *config,
> int ret;
> struct vduse_dev *dev;
>
> + ret = -EPERM;
> + if ((config->device_id == VIRTIO_ID_NET) && !capable(CAP_NET_ADMIN))
> + goto err;
> +
> ret = -EEXIST;
> if (vduse_find_dev(config->name))
> goto err;
> @@ -2044,6 +2053,7 @@ static const struct vdpa_mgmtdev_ops vdpa_dev_mgmtdev_ops = {
>
> static struct virtio_device_id id_table[] = {
> { VIRTIO_ID_BLOCK, VIRTIO_DEV_ANY_ID },
> + { VIRTIO_ID_NET, VIRTIO_DEV_ANY_ID },
> { 0 },
> };
>
> --
> 2.43.0
>
>
On 1/5/24 10:59, Eugenio Perez Martin wrote:
> On Fri, Jan 5, 2024 at 9:12 AM Maxime Coquelin
> <[email protected]> wrote:
>>
>>
>>
>> On 1/5/24 03:45, Jason Wang wrote:
>>> On Thu, Jan 4, 2024 at 11:38 PM Maxime Coquelin
>>> <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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 <[email protected]>
>>>
>>> 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
>>>>
>>>
>>
>>
>
On Fri, Jan 5, 2024 at 6:14 PM Maxime Coquelin
<[email protected]> wrote:
>
>
>
> On 1/5/24 10:59, Eugenio Perez Martin wrote:
> > On Fri, Jan 5, 2024 at 9:12 AM Maxime Coquelin
> > <[email protected]> wrote:
> >>
> >>
> >>
> >> On 1/5/24 03:45, Jason Wang wrote:
> >>> On Thu, Jan 4, 2024 at 11:38 PM Maxime Coquelin
> >>> <[email protected]> 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 <[email protected]>
> >>>> ---
> >>>> 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 <[email protected]>
> >>>
> >>> 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
> >>>>
> >>>
> >>
> >>
> >
>