linux-kernel - Re: [RFC v1 8/8] mshv: add vfio bridge device

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20210810105211.zzaamjqmcq3jecmn@liuwe-devbox-debian-v2>
Date:   Tue, 10 Aug 2021 10:52:11 +0000
From:   Wei Liu <wei.liu@...nel.org>
To:     Praveen Kumar <kumarpraveen@...ux.microsoft.com>
Cc:     Wei Liu <wei.liu@...nel.org>,
        Linux on Hyper-V List <linux-hyperv@...r.kernel.org>,
        virtualization@...ts.linux-foundation.org,
        Linux Kernel List <linux-kernel@...r.kernel.org>,
        Michael Kelley <mikelley@...rosoft.com>,
        Vineeth Pillai <viremana@...ux.microsoft.com>,
        Sunil Muthuswamy <sunilmut@...rosoft.com>,
        Nuno Das Neves <nunodasneves@...ux.microsoft.com>,
        pasha.tatashin@...een.com, "K. Y. Srinivasan" <kys@...rosoft.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        Stephen Hemminger <sthemmin@...rosoft.com>,
        Dexuan Cui <decui@...rosoft.com>
Subject: Re: [RFC v1 8/8] mshv: add vfio bridge device

On Wed, Aug 04, 2021 at 12:57:03AM +0530, Praveen Kumar wrote:
> 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?

Fair point. This can be done, albeit at the cost of having a rather
different code structure.

I was waiting to see if we should somehow merge this with KVM's
implementation so the code was deliberately kept close. If there is no
further comment I can of course make the change you suggested.

> 
> > +
> > +		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.

Will do.

Wei.