thr3ads.net - Virtualization - [PATCH v2 5/7] vdpa: validate device feature provisioning against supported class [Feb 2023]

If this information is useful, please help other people find it:
Share via:

Si-Wei Liu

2023-Jan-31 23:22 UTC

[PATCH v2 5/7] vdpa: validate device feature provisioning against supported class

Today when device features are explicitly provisioned, the features
user supplied may contain device class specific features that are
not supported by the parent managment device. On the other hand,
when parent managment device supports more than one class, the
device features to provision may be ambiguous if none of the class
specific attributes is provided at the same time. Validate these
cases and prompt appropriate user errors accordingly.

Signed-off-by: Si-Wei Liu <si-wei.liu at oracle.com>
---
 drivers/vdpa/vdpa.c | 51 ++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 42 insertions(+), 9 deletions(-)

diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index 1eba978..35a72d6 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -460,12 +460,30 @@ static int vdpa_nl_mgmtdev_handle_fill(struct sk_buff
*msg, const struct vdpa_mg
 	return 0;
 }
 
+static u64 vdpa_mgmtdev_get_classes(const struct vdpa_mgmt_dev *mdev,
+				    unsigned int *nclasses)
+{
+	u64 supported_classes = 0;
+	unsigned int n = 0;
+	int i = 0;
+
+	while (mdev->id_table[i].device) {
+		if (mdev->id_table[i].device <= 63) {
+			supported_classes |= BIT_ULL(mdev->id_table[i].device);
+			n++;
+		}
+		i++;
+	}
+	if (nclasses)
+		*nclasses = n;
+
+	return supported_classes;
+}
+
 static int vdpa_mgmtdev_fill(const struct vdpa_mgmt_dev *mdev, struct sk_buff
*msg,
 			     u32 portid, u32 seq, int flags)
 {
-	u64 supported_classes = 0;
 	void *hdr;
-	int i = 0;
 	int err;
 
 	hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags,
VDPA_CMD_MGMTDEV_NEW);
@@ -475,14 +493,9 @@ static int vdpa_mgmtdev_fill(const struct vdpa_mgmt_dev
*mdev, struct sk_buff *m
 	if (err)
 		goto msg_err;
 
-	while (mdev->id_table[i].device) {
-		if (mdev->id_table[i].device <= 63)
-			supported_classes |= BIT_ULL(mdev->id_table[i].device);
-		i++;
-	}
-
 	if (nla_put_u64_64bit(msg, VDPA_ATTR_MGMTDEV_SUPPORTED_CLASSES,
-			      supported_classes, VDPA_ATTR_UNSPEC)) {
+			      vdpa_mgmtdev_get_classes(mdev, NULL),
+			      VDPA_ATTR_UNSPEC)) {
 		err = -EMSGSIZE;
 		goto msg_err;
 	}
@@ -571,8 +584,10 @@ static int vdpa_nl_cmd_dev_add_set_doit(struct sk_buff
*skb, struct genl_info *i
 	struct vdpa_dev_set_config config = {};
 	struct nlattr **nl_attrs = info->attrs;
 	struct vdpa_mgmt_dev *mdev;
+	unsigned int ncls = 0;
 	const u8 *macaddr;
 	const char *name;
+	u64 classes;
 	int err = 0;
 
 	if (!info->attrs[VDPA_ATTR_DEV_NAME])
@@ -649,6 +664,24 @@ static int vdpa_nl_cmd_dev_add_set_doit(struct sk_buff
*skb, struct genl_info *i
 		goto err;
 	}
 
+	classes = vdpa_mgmtdev_get_classes(mdev, &ncls);
+	if (config.mask & VDPA_DEV_NET_ATTRS_MASK &&
+	    !(classes & BIT_ULL(VIRTIO_ID_NET))) {
+		NL_SET_ERR_MSG_MOD(info->extack,
+				   "Network class attributes provided on unsupported management
device");
+		err = -EINVAL;
+		goto err;
+	}
+	if (!(config.mask & VDPA_DEV_NET_ATTRS_MASK) &&
+	    config.mask & BIT_ULL(VDPA_ATTR_DEV_FEATURES) &&
+	    classes & BIT_ULL(VIRTIO_ID_NET) && ncls > 1 &&
+	    config.device_features & VIRTIO_DEVICE_F_MASK) {
+		NL_SET_ERR_MSG_MOD(info->extack,
+				   "Management device supports multi-class while device features
specified are ambiguous");
+		err = -EINVAL;
+		goto err;
+	}
+
 	err = mdev->ops->dev_add(mdev, name, &config);
 err:
 	up_write(&vdpa_dev_lock);
-- 
1.8.3.1

Michael S. Tsirkin

2023-Feb-03 08:09 UTC

head link

[PATCH v2 5/7] vdpa: validate device feature provisioning against supported class

On Tue, Jan 31, 2023 at 03:22:23PM -0800, Si-Wei Liu
wrote:> Today when device features are explicitly provisioned, the features
> user supplied may contain device class specific features that are
> not supported by the parent managment device. On the other hand,
> when parent managment device supports more than one class, the
> device features to provision may be ambiguous if none of the class
> specific attributes is provided at the same time. Validate these
> cases and prompt appropriate user errors accordingly.
> 
> Signed-off-by: Si-Wei Liu <si-wei.liu at oracle.com>
> ---
>  drivers/vdpa/vdpa.c | 51
++++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 42 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index 1eba978..35a72d6 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -460,12 +460,30 @@ static int vdpa_nl_mgmtdev_handle_fill(struct sk_buff
*msg, const struct vdpa_mg
>  	return 0;
>  }
>  
> +static u64 vdpa_mgmtdev_get_classes(const struct vdpa_mgmt_dev *mdev,
> +				    unsigned int *nclasses)
given max value is apparently 64 how important is it that it's unsigned?
Just make it an int.

Also I'd return u64 through a pointer too for consistency.
> +{
> +	u64 supported_classes = 0;
> +	unsigned int n = 0;
> +	int i = 0;
> +
> +	while (mdev->id_table[i].device) {
> +		if (mdev->id_table[i].device <= 63) {
> +			supported_classes |= BIT_ULL(mdev->id_table[i].device);
> +			n++;
> +		}
> +		i++;
> +	}

Better as a for loop. I know you are just moving code if you
want to make it very clear it's a refactoring split
as a separate patch, but ok anyway.
> +	if (nclasses)
> +		*nclasses = n;
> +
> +	return supported_classes;
> +}
> +
>  static int vdpa_mgmtdev_fill(const struct vdpa_mgmt_dev *mdev, struct
sk_buff *msg,
>  			     u32 portid, u32 seq, int flags)
>  {
> -	u64 supported_classes = 0;
>  	void *hdr;
> -	int i = 0;
>  	int err;
>  
>  	hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags,
VDPA_CMD_MGMTDEV_NEW);
> @@ -475,14 +493,9 @@ static int vdpa_mgmtdev_fill(const struct
vdpa_mgmt_dev *mdev, struct sk_buff *m
>  	if (err)
>  		goto msg_err;
>  
> -	while (mdev->id_table[i].device) {
> -		if (mdev->id_table[i].device <= 63)
> -			supported_classes |= BIT_ULL(mdev->id_table[i].device);
> -		i++;
> -	}
> -
>  	if (nla_put_u64_64bit(msg, VDPA_ATTR_MGMTDEV_SUPPORTED_CLASSES,
> -			      supported_classes, VDPA_ATTR_UNSPEC)) {
> +			      vdpa_mgmtdev_get_classes(mdev, NULL),
> +			      VDPA_ATTR_UNSPEC)) {
>  		err = -EMSGSIZE;
>  		goto msg_err;
>  	}
> @@ -571,8 +584,10 @@ static int vdpa_nl_cmd_dev_add_set_doit(struct sk_buff
*skb, struct genl_info *i
>  	struct vdpa_dev_set_config config = {};
>  	struct nlattr **nl_attrs = info->attrs;
>  	struct vdpa_mgmt_dev *mdev;
> +	unsigned int ncls = 0;
>  	const u8 *macaddr;
>  	const char *name;
> +	u64 classes;
>  	int err = 0;
>  
>  	if (!info->attrs[VDPA_ATTR_DEV_NAME])
> @@ -649,6 +664,24 @@ static int vdpa_nl_cmd_dev_add_set_doit(struct sk_buff
*skb, struct genl_info *i
>  		goto err;
>  	}
>  
> +	classes = vdpa_mgmtdev_get_classes(mdev, &ncls);
> +	if (config.mask & VDPA_DEV_NET_ATTRS_MASK &&
> +	    !(classes & BIT_ULL(VIRTIO_ID_NET))) {
> +		NL_SET_ERR_MSG_MOD(info->extack,
> +				   "Network class attributes provided on unsupported management
device");
> +		err = -EINVAL;
> +		goto err;
> +	}
> +	if (!(config.mask & VDPA_DEV_NET_ATTRS_MASK) &&
> +	    config.mask & BIT_ULL(VDPA_ATTR_DEV_FEATURES) &&
> +	    classes & BIT_ULL(VIRTIO_ID_NET) && ncls > 1
&&
> +	    config.device_features & VIRTIO_DEVICE_F_MASK) {
> +		NL_SET_ERR_MSG_MOD(info->extack,
> +				   "Management device supports multi-class while device features
specified are ambiguous");
> +		err = -EINVAL;
> +		goto err;
> +	}
> +
>  	err = mdev->ops->dev_add(mdev, name, &config);
>  err:
>  	up_write(&vdpa_dev_lock);
> -- 
> 1.8.3.1

Virtualization - Feb 2023 - [PATCH v2 5/7] vdpa: validate device feature provisioning against supported class

[PATCH v2 5/7] vdpa: validate device feature provisioning against supported class

[PATCH v2 5/7] vdpa: validate device feature provisioning against supported class