Date:   Fri, 3 Feb 2023 03:09:08 -0500
From:   "Michael S. Tsirkin" <mst@redhat.com>
To:     Si-Wei Liu <si-wei.liu@oracle.com>
Cc:     jasowang@redhat.com, parav@nvidia.com, elic@nvidia.com,
        virtualization@lists.linux-foundation.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 5/7] vdpa: validate device feature provisioning
 against supported class
Message-ID: <20230203025523-mutt-send-email-mst@kernel.org>
References: <1675207345-22328-1-git-send-email-si-wei.liu@oracle.com>
 <1675207345-22328-6-git-send-email-si-wei.liu@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1675207345-22328-6-git-send-email-si-wei.liu@oracle.com>
Precedence: bulk

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