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] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 8 Aug 2023 16:19:10 +0530
From:   "Gupta, Nipun" <nipun.gupta@....com>
To:     Alex Williamson <alex.williamson@...hat.com>
Cc:     gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org,
        git@....com, pieter.jansen-van-vuuren@....com,
        nikhil.agarwal@....com, michal.simek@....com,
        abhijit.gangurde@....com, Shubham Rohila <shubham.rohila@....com>
Subject: Re: [PATCH v5 3/3] vfio-cdx: add bus mastering device feature support



On 8/4/2023 3:47 AM, Alex Williamson wrote:
> On Thu, 3 Aug 2023 20:02:53 +0530
> Nipun Gupta <nipun.gupta@....com> wrote:
> 
>> Support Bus master enable and disable on VFIO-CDX devices using
>> VFIO_DEVICE_FEATURE_BUS_MASTER flag over VFIO_DEVICE_FEATURE IOCTL.
>>
>> Co-developed-by: Shubham Rohila <shubham.rohila@....com>
>> Signed-off-by: Shubham Rohila <shubham.rohila@....com>
>> Signed-off-by: Nipun Gupta <nipun.gupta@....com>
>> ---
>>
>> Changes v4->v5:
>> - Use device feature IOCTL instead of adding a new VFIO IOCTL
>>    for bus master feature.
>>
>> Changes in v4:
>> - This patch is newly added which uses cdx_set_master() and
>>    cdx_clear_master() APIs.
>>
>>   drivers/vfio/cdx/main.c | 40 ++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 40 insertions(+)
>>
>> diff --git a/drivers/vfio/cdx/main.c b/drivers/vfio/cdx/main.c
>> index c376a69d2db2..6420cf6eb2f9 100644
>> --- a/drivers/vfio/cdx/main.c
>> +++ b/drivers/vfio/cdx/main.c
>> @@ -52,6 +52,45 @@ static void vfio_cdx_close_device(struct vfio_device *core_vdev)
>>   	cdx_dev_reset(core_vdev->dev);
>>   }
>>   
>> +static int vfio_cdx_bm_ctrl(struct vfio_device *core_vdev, u32 flags,
>> +			    void __user *arg, size_t argsz)
>> +{
>> +	size_t minsz =
>> +		offsetofend(struct vfio_device_feature_bus_master, op);
>> +	struct cdx_device *cdx_dev = to_cdx_device(core_vdev->dev);
>> +	struct vfio_device_feature_bus_master ops;
>> +	int ret;
>> +
>> +	ret = vfio_check_feature(flags, argsz, VFIO_DEVICE_FEATURE_SET,
>> +				 sizeof(ops));
>> +	if (ret != 1)
>> +		return ret;
>> +
>> +	if (copy_from_user(&ops, arg, minsz))
>> +		return -EFAULT;
>> +
>> +	switch (ops.op) {
>> +	case VFIO_DEVICE_FEATURE_CLEAR_MASTER:
>> +		cdx_clear_master(cdx_dev);
>> +		return 0;
>> +	case VFIO_DEVICE_FEATURE_SET_MASTER:
>> +		return cdx_set_master(cdx_dev);
> 
> It's curious that the implementation of set and clear in CDX call
> through to functions with non-void returns, but we simply ignore the
> return in cdx_clear_master().  Does something prevent clear from
> failing?

Would update cdx_clear_master() with non-void return.

> 
> I also note internally that true is used for enabling and false for
> disabling, which is effectively opposite of the proposed uAPI in the
> previous patch.

Yes, will fix in the uAPI.

> 
> If the idea here is that the user should assume bus master is disabled
> when opening the device, what happens if the user closes the device
> with bus master enabled?  What would cleanup that state for the next
> user?

cdx_dev_reset() clears the bus mastering and user would enable bus 
mastering once ready for DMA. Probably we also need to add reset in 
device open. So user should assume that bus master is disabled when 
opening the device, and close would also clear the bus mastering (by 
calling reset).

> 
> Is there a use case for the GET operation in userspace?

There is no use-case for get operation in CDX as of now.

Thanks,
Nipun

> Thanks,
> 
> Alex
> 
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>> +static int vfio_cdx_ioctl_feature(struct vfio_device *device, u32 flags,
>> +				  void __user *arg, size_t argsz)
>> +{
>> +	switch (flags & VFIO_DEVICE_FEATURE_MASK) {
>> +	case VFIO_DEVICE_FEATURE_BUS_MASTER:
>> +		return vfio_cdx_bm_ctrl(device, flags, arg, argsz);
>> +	default:
>> +		return -ENOTTY;
>> +	}
>> +}
>> +
>>   static int vfio_cdx_ioctl_get_info(struct vfio_cdx_device *vdev,
>>   				   struct vfio_device_info __user *arg)
>>   {
>> @@ -169,6 +208,7 @@ static const struct vfio_device_ops vfio_cdx_ops = {
>>   	.open_device	= vfio_cdx_open_device,
>>   	.close_device	= vfio_cdx_close_device,
>>   	.ioctl		= vfio_cdx_ioctl,
>> +	.device_feature = vfio_cdx_ioctl_feature,
>>   	.mmap		= vfio_cdx_mmap,
>>   	.bind_iommufd	= vfio_iommufd_physical_bind,
>>   	.unbind_iommufd	= vfio_iommufd_physical_unbind,
> 

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ