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

[Joao Martins <joao.m.martins@xxxxxxxxxx>]

On 23/10/2023 13:41, Arnd Bergmann wrote:
> On Mon, Oct 23, 2023, at 14:10, Jason Gunthorpe wrote:
>> On Mon, Oct 23, 2023 at 10:28:13AM +0100, Joao Martins wrote:
>>>> so it's probably
>>>> best to add a range check plus type cast, rather than an
>>>> expensive div_u64() here.
>>>
>>> OK
>>
>> Just keep it simple, we don't need to optimize for 32 bit. div_u64
>> will make the compiler happy.
> 
> Fair enough. FWIW, I tried adding just the range check to see
> if that would make the compiler turn it into a 32-bit division,
> but that didn't work.
> 
> Some type of range check might still be good to have for
> unrelated reasons.

I can reproduce the arm32 build problem and I'm applying this diff below to this
patch to fix it. It essentially moves all the checks to
iommufd_check_iova_range(), including range-check and adding div_u64.

Additionally, perhaps should also move the iommufd_check_iova_range() invocation
via io_pagetable.c code rather than hw-pagetable code? It seems to make more
sense as there's nothing hw-pagetable specific that needs to be in here.

diff --git a/drivers/iommu/iommufd/hw_pagetable.c
b/drivers/iommu/iommufd/hw_pagetable.c
index a25042b0d3ba..4705954c51fe 100644
--- a/drivers/iommu/iommufd/hw_pagetable.c
+++ b/drivers/iommu/iommufd/hw_pagetable.c
@@ -224,23 +224,27 @@ int iommufd_hwpt_set_dirty_tracking(struct iommufd_ucmd *ucmd)
 int iommufd_check_iova_range(struct iommufd_ioas *ioas,
                             struct iommu_hwpt_get_dirty_bitmap *bitmap)
 {
-       unsigned long npages;
+       unsigned long npages, last_iova, iova = bitmap->iova;
+       unsigned long length = bitmap->length;
        size_t iommu_pgsize;
        int rc = -EINVAL;

        if (!bitmap->page_size)
                return rc;

-       npages = bitmap->length / bitmap->page_size;
+       if (check_add_overflow(iova, length - 1, &last_iova))
+               return -EOVERFLOW;
+
+       npages = div_u64(bitmap->length, bitmap->page_size);
        if (!npages || (npages > ULONG_MAX))
                return rc;

        iommu_pgsize = ioas->iopt.iova_alignment;

-       if (bitmap->iova & (iommu_pgsize - 1))
+       if (iova & (iommu_pgsize - 1))
                return rc;

-       if (!bitmap->length || bitmap->length & (iommu_pgsize - 1))
+       if (length || length & (iommu_pgsize - 1))
                return rc;

        return 0;
diff --git a/drivers/iommu/iommufd/io_pagetable.c
b/drivers/iommu/iommufd/io_pagetable.c
index 9955797862eb..00f5f60dc27e 100644
--- a/drivers/iommu/iommufd/io_pagetable.c
+++ b/drivers/iommu/iommufd/io_pagetable.c
@@ -486,15 +486,7 @@ int iopt_read_and_clear_dirty_data(struct io_pagetable *iopt,
                                   unsigned long flags,
                                   struct iommu_hwpt_get_dirty_bitmap *bitmap)
 {
-       unsigned long last_iova, iova = bitmap->iova;
-       unsigned long length = bitmap->length;
-       int ret = -EINVAL;
-
-       if ((iova & (iopt->iova_alignment - 1)))
-               return -EINVAL;
-
-       if (check_add_overflow(iova, length - 1, &last_iova))
-               return -EOVERFLOW;
+       int ret;

        down_read(&iopt->iova_rwsem);
        ret = iommu_read_and_clear_dirty(domain, iopt, flags, bitmap);