netdev - Re: [PATCH V4 vfio 05/10] vfio: Introduce the DMA logging feature support

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]

Date:   Sun, 28 Aug 2022 16:29:06 +0300
From:   Yishai Hadas <yishaih@...dia.com>
To:     Jason Gunthorpe <jgg@...dia.com>,
        Alex Williamson <alex.williamson@...hat.com>
CC:     Joao Martins <joao.m.martins@...cle.com>, <saeedm@...dia.com>,
        <kvm@...r.kernel.org>, <netdev@...r.kernel.org>, <kuba@...nel.org>,
        <kevin.tian@...el.com>, <leonro@...dia.com>, <maorg@...dia.com>,
        <cohuck@...hat.com>
Subject: Re: [PATCH V4 vfio 05/10] vfio: Introduce the DMA logging feature
 support

On 26/08/2022 15:52, Jason Gunthorpe wrote:
> On Thu, Aug 25, 2022 at 04:46:51PM -0600, Alex Williamson wrote:
>> On Thu, 25 Aug 2022 23:26:04 +0100
>> Joao Martins <joao.m.martins@...cle.com> wrote:
>>
>>> On 8/25/22 21:49, Alex Williamson wrote:
>>>> On Mon, 15 Aug 2022 18:11:04 +0300
>>>> Yishai Hadas <yishaih@...dia.com> wrote:
>>>>> +static int
>>>>> +vfio_ioctl_device_feature_logging_report(struct vfio_device *device,
>>>>> +					 u32 flags, void __user *arg,
>>>>> +					 size_t argsz)
>>>>> +{
>>>>> +	size_t minsz =
>>>>> +		offsetofend(struct vfio_device_feature_dma_logging_report,
>>>>> +			    bitmap);
>>>>> +	struct vfio_device_feature_dma_logging_report report;
>>>>> +	struct iova_bitmap_iter iter;
>>>>> +	int ret;
>>>>> +
>>>>> +	if (!device->log_ops)
>>>>> +		return -ENOTTY;
>>>>> +
>>>>> +	ret = vfio_check_feature(flags, argsz,
>>>>> +				 VFIO_DEVICE_FEATURE_GET,
>>>>> +				 sizeof(report));
>>>>> +	if (ret != 1)
>>>>> +		return ret;
>>>>> +
>>>>> +	if (copy_from_user(&report, arg, minsz))
>>>>> +		return -EFAULT;
>>>>> +
>>>>> +	if (report.page_size < PAGE_SIZE || !is_power_of_2(report.page_size))
>>>> Why is PAGE_SIZE a factor here?  I'm under the impression that
>>>> iova_bitmap is intended to handle arbitrary page sizes.  Thanks,
>>> Arbritary page sizes ... which are powers of 2. We use page shift in iova bitmap.
>>> While it's not hard to lose this restriction (trading a shift over a slower mul)
>>> ... I am not sure it is worth supporting said use considering that there aren't
>>> non-powers of 2 page sizes right now?
>>>
>>> The PAGE_SIZE restriction might be that it's supposed to be the lowest possible page_size.
>> Sorry, I was unclear.  Size relative to PAGE_SIZE was my only question,
>> not that we shouldn't require power of 2 sizes.  We're adding device
>> level dirty tracking, where the device page size granularity might be
>> 4K on a host with a CPU 64K page size.  Maybe there's a use case for
>> that.  Given the flexibility claimed by the iova_bitmap support,
>> requiring reported page size less than system PAGE_SIZE seems
>> unjustified.  Thanks,
> +1
>
> Jason

OK

So in V5 we may come with the below as we don't really expect page size 
smaller than 4K.

if (report.page_size < SZ_4K || !is_power_of_2(report.page_size))
         return -EINVAL;

Yishai