Re: [PATCH v7 2/4] vfio: VFIO driver for mediated devices

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 8/25/2016 2:52 PM, Dong Jia wrote:
> On Thu, 25 Aug 2016 09:23:53 +0530
> Kirti Wankhede <kwankhede@xxxxxxxxxx> wrote:
> 
> [...]
> 
> Dear Kirti,
> 
> I just rebased my vfio-ccw patches to this series.
> With a little fix, which was pointed it out in my reply to the #3
> patch, it works fine.
> 

Thanks for update. Glad to know this works for you.


>> +static long vfio_mdev_unlocked_ioctl(void *device_data,
>> +				     unsigned int cmd, unsigned long arg)
>> +{
>> +	int ret = 0;
>> +	struct vfio_mdev *vmdev = device_data;
>> +	struct parent_device *parent = vmdev->mdev->parent;
>> +	unsigned long minsz;
>> +
>> +	switch (cmd) {
>> +	case VFIO_DEVICE_GET_INFO:
>> +	{
>> +		struct vfio_device_info info;
>> +
>> +		minsz = offsetofend(struct vfio_device_info, num_irqs);
>> +
>> +		if (copy_from_user(&info, (void __user *)arg, minsz))
>> +			return -EFAULT;
>> +
>> +		if (info.argsz < minsz)
>> +			return -EINVAL;
>> +
>> +		if (parent->ops->get_device_info)
>> +			ret = parent->ops->get_device_info(vmdev->mdev, &info);
>> +		else
>> +			return -EINVAL;
>> +
>> +		if (ret)
>> +			return ret;
>> +
>> +		if (parent->ops->reset)
>> +			info.flags |= VFIO_DEVICE_FLAGS_RESET;
> Shouldn't this be done inside the get_device_info callback?
> 

I would like Vendor driver to set device type only. Reset flag should be
set on basis of reset() callback provided.

>> +
>> +		memcpy(&vmdev->dev_info, &info, sizeof(info));
>> +
>> +		return copy_to_user((void __user *)arg, &info, minsz);
>> +	}
> [...]
> 
>> +
>> +static ssize_t vfio_mdev_read(void *device_data, char __user *buf,
>> +			      size_t count, loff_t *ppos)
>> +{
>> +	struct vfio_mdev *vmdev = device_data;
>> +	struct mdev_device *mdev = vmdev->mdev;
>> +	struct parent_device *parent = mdev->parent;
>> +	unsigned int done = 0;
>> +	int ret;
>> +
>> +	if (!parent->ops->read)
>> +		return -EINVAL;
>> +
>> +	while (count) {
> Here, I have to say sorry to you guys for that I didn't notice the
> bad impact of this change to my patches during the v6 discussion.
> 
> For vfio-ccw, I introduced an I/O region to input/output I/O
> instruction parameters and results for Qemu. The @count of these data
> currently is 140. So supporting arbitrary lengths in one shot here, and
> also in vfio_mdev_write, seems the better option for this case.
> 
> I believe that if the pci drivers want to iterate in a 4 bytes step, you
> can do that in the parent read/write callbacks instead.
> 
> What do you think?
> 

I would like to know Alex's thought on this. He raised concern with this
approach in v6 reviews:
"But I think this is exploitable, it lets the user make the kernel
allocate an arbitrarily sized buffer."

Thanks,
Kirti

>> +		size_t filled;
>> +
>> +		if (count >= 4 && !(*ppos % 4)) {
>> +			u32 val;
>> +
>> +			ret = parent->ops->read(mdev, (char *)&val, sizeof(val),
>> +						*ppos);
>> +			if (ret <= 0)
>> +				goto read_err;
>> +
>> +			if (copy_to_user(buf, &val, sizeof(val)))
>> +				goto read_err;
>> +
>> +			filled = 4;
>> +		} else if (count >= 2 && !(*ppos % 2)) {
>> +			u16 val;
>> +
>> +			ret = parent->ops->read(mdev, (char *)&val, sizeof(val),
>> +						*ppos);
>> +			if (ret <= 0)
>> +				goto read_err;
>> +
>> +			if (copy_to_user(buf, &val, sizeof(val)))
>> +				goto read_err;
>> +
>> +			filled = 2;
>> +		} else {
>> +			u8 val;
>> +
>> +			ret = parent->ops->read(mdev, &val, sizeof(val), *ppos);
>> +			if (ret <= 0)
>> +				goto read_err;
>> +
>> +			if (copy_to_user(buf, &val, sizeof(val)))
>> +				goto read_err;
>> +
>> +			filled = 1;
>> +		}
>> +
>> +		count -= filled;
>> +		done += filled;
>> +		*ppos += filled;
>> +		buf += filled;
>> +	}
>> +
>> +	return done;
>> +
>> +read_err:
>> +	return -EFAULT;
>> +}
> [...]
> 
> --------
> Dong Jia
> 
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux