On 09-07-2021 17:13, Wei Liu wrote:
> +
> +static int mshv_vfio_set_group(struct mshv_device *dev, long attr, u64 arg)
> +{
> + struct mshv_vfio *mv = dev->private;
> + struct vfio_group *vfio_group;
> + struct mshv_vfio_group *mvg;
> + int32_t __user *argp = (int32_t __user *)(unsigned long)arg;
> + struct fd f;
> + int32_t fd;
> + int ret;
> +
> + switch (attr) {
> + case MSHV_DEV_VFIO_GROUP_ADD:
> + if (get_user(fd, argp))
> + return -EFAULT;
> +
> + f = fdget(fd);
> + if (!f.file)
> + return -EBADF;
> +
> + vfio_group = mshv_vfio_group_get_external_user(f.file);
> + fdput(f);
> +
> + if (IS_ERR(vfio_group))
> + return PTR_ERR(vfio_group);
> +
> + mutex_lock(&mv->lock);
> +
> + list_for_each_entry(mvg, &mv->group_list, node) {
> + if (mvg->vfio_group == vfio_group) {
> + mutex_unlock(&mv->lock);
> + mshv_vfio_group_put_external_user(vfio_group);
> + return -EEXIST;
> + }
> + }
> +
> + mvg = kzalloc(sizeof(*mvg), GFP_KERNEL_ACCOUNT);
> + if (!mvg) {
> + mutex_unlock(&mv->lock);
> + mshv_vfio_group_put_external_user(vfio_group);
> + return -ENOMEM;
> + }
> +
> + list_add_tail(&mvg->node, &mv->group_list);
> + mvg->vfio_group = vfio_group;
> +
> + mutex_unlock(&mv->lock);
> +
> + return 0;
> +
> + case MSHV_DEV_VFIO_GROUP_DEL:
> + if (get_user(fd, argp))
> + return -EFAULT;
> +
> + f = fdget(fd);
> + if (!f.file)
> + return -EBADF;
Can we move these both checks above switch statement and do fdput accordingly under both case statement accordingly?
> +
> + ret = -ENOENT;
> +
> + mutex_lock(&mv->lock);
> +
> + list_for_each_entry(mvg, &mv->group_list, node) {
> + if (!mshv_vfio_external_group_match_file(mvg->vfio_group,
> + f.file))
> + continue;
> +
> + list_del(&mvg->node);
> + mshv_vfio_group_put_external_user(mvg->vfio_group);
> + kfree(mvg);
> + ret = 0;
> + break;
> + }
> +
> + mutex_unlock(&mv->lock);
> +
> + fdput(f);
> +
> + return ret;
> + }
> +
> + return -ENXIO;
> +}
> +
> +static int mshv_vfio_set_attr(struct mshv_device *dev,
> + struct mshv_device_attr *attr)
> +{
> + switch (attr->group) {
> + case MSHV_DEV_VFIO_GROUP:
> + return mshv_vfio_set_group(dev, attr->attr, attr->addr);
> + }
> +
> + return -ENXIO;
> +}
> +
> +static int mshv_vfio_has_attr(struct mshv_device *dev,
> + struct mshv_device_attr *attr)
> +{
> + switch (attr->group) {
> + case MSHV_DEV_VFIO_GROUP:
> + switch (attr->attr) {
> + case MSHV_DEV_VFIO_GROUP_ADD:
> + case MSHV_DEV_VFIO_GROUP_DEL:
> + return 0;
> + }
> +
> + break;
do we need this break statement ? If not, lets remove it.
> + }
> +
> + return -ENXIO;
> +}
> +
> +static void mshv_vfio_destroy(struct mshv_device *dev)
> +{
> + struct mshv_vfio *mv = dev->private;
> + struct mshv_vfio_group *mvg, *tmp;
> +
> + list_for_each_entry_safe(mvg, tmp, &mv->group_list, node) {
> + mshv_vfio_group_put_external_user(mvg->vfio_group);
> + list_del(&mvg->node);
> + kfree(mvg);
> + }
> +
> + kfree(mv);
> + kfree(dev);
We are freeing up dev. Please ignore my comment in caller patch. Thanks.
> +}
> +
> +static int mshv_vfio_create(struct mshv_device *dev, u32 type);
> +
> +static struct mshv_device_ops mshv_vfio_ops = {
> + .name = "mshv-vfio",
> + .create = mshv_vfio_create,
> + .destroy = mshv_vfio_destroy,
> + .set_attr = mshv_vfio_set_attr,
> + .has_attr = mshv_vfio_has_attr,
> +};
Regards,
~Praveen.
