Re: [PATCH v5 07/18] iommufd: Add IOMMU_HWPT_GET_DIRTY_BITMAP

[Jason Gunthorpe <jgg@nvidia.com>]

On Mon, Oct 23, 2023 at 06:55:56PM +0100, Joao Martins wrote:
> On 23/10/2023 17:34, Jason Gunthorpe wrote:
> > On Mon, Oct 23, 2023 at 05:31:22PM +0100, Joao Martins wrote:
> >>> Write it like this:
> >>>
> >>> int iommufd_check_iova_range(struct iommufd_ioas *ioas,
> >>> 			     struct iommu_hwpt_get_dirty_bitmap *bitmap)
> >>> {
> >>> 	size_t iommu_pgsize = ioas->iopt.iova_alignment;
> >>> 	u64 last_iova;
> >>>
> >>> 	if (check_add_overflow(bitmap->iova, bitmap->length - 1, &last_iova))
> >>> 		return -EOVERFLOW;
> >>>
> >>> 	if (bitmap->iova > ULONG_MAX || last_iova > ULONG_MAX)
> >>> 		return -EOVERFLOW;
> >>>
> >>> 	if ((bitmap->iova & (iommu_pgsize - 1)) ||
> >>> 	    ((last_iova + 1) & (iommu_pgsize - 1)))
> >>> 		return -EINVAL;
> >>> 	return 0;
> >>> }
> >>>
> >>> And if 0 should really be rejected then check iova == last_iova
> >>
> >> It should; Perhaps extending the above and replicate that second the ::page_size
> >> alignment check is important as it's what's used by the bitmap e.g.
> > 
> > That makes sense, much clearer what it is trying to do that way
> 
> In regards to clearity, I am still checking if bitmap::page_size being 0 or not,
> to avoid being so implicitly in the last check i.e. (bitmap->iova &
> (bitmap->page_size - 1)) being an and to 0xfffffffff.

I think you have to check explicitly, iova/last_iova could also be
zero..

Jason