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@xxxxxxxxxx>
> ---
> 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 mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
