Re: [PATCH v5 16/18] iommufd/selftest: Test IOMMU_HWPT_GET_DIRTY_BITMAP

[Nicolin Chen <nicolinc@nvidia.com>]

On Mon, Oct 23, 2023 at 10:46:19PM +0100, Joao Martins wrote:

> > You are probably just not copying it right.
> >
> > The bitmap APIs treat the pointer as one big array of ulongs and set the right
> > word of it, so your copy_from_user needs to make sure it is copying from the
> > right offset.
> >
> > Given that the tests (of different sizes) exercise the boundaries of the bitmap
> > it eventually exposes. The 256M specifically it could be that I an testing the 2
> > PAGE_SIZE bitmap, that I offset on purpose (as part of the test).
> >
> > Let me play with it in the meantime and I will paste an diff based on yours.
> 
> This is based on your snippet, except that we just copy the whole thing instead
> of per chunk.  Should make it less error-prone than to calculate offsets. Could
> you try it out and see if it works for you? Meanwhile I found out that I was
> checking the uptr (bitmap pointer) alignment against page_size which didn't make
> sense.

This fixes everything, per my test results. Let's have all of
these incremental fixes in v6.

Cheers
Nicolin

> diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c
> index 8a2c7df85441..d8551c9d5b6c 100644
> --- a/drivers/iommu/iommufd/selftest.c
> +++ b/drivers/iommu/iommufd/selftest.c
> @@ -1098,14 +1098,14 @@ static int iommufd_test_dirty(struct iommufd_ucmd *ucmd,
> unsigned int mockpt_id,
>                               unsigned long page_size, void __user *uptr,
>                               u32 flags)
>  {
> -       unsigned long i, max = length / page_size;
> +       unsigned long bitmap_size, i, max = length / page_size;
>         struct iommu_test_cmd *cmd = ucmd->cmd;
>         struct iommufd_hw_pagetable *hwpt;
>         struct mock_iommu_domain *mock;
>         int rc, count = 0;
> +       void *tmp;
> 
> -       if (iova % page_size || length % page_size ||
> -           (uintptr_t)uptr % page_size)
> +       if (iova % page_size || length % page_size || !uptr)
>                 return -EINVAL;
> 
>         hwpt = get_md_pagetable(ucmd, mockpt_id, &mock);
> @@ -1117,11 +1117,24 @@ static int iommufd_test_dirty(struct iommufd_ucmd *ucmd,
> unsigned int mockpt_id,
>                 goto out_put;
>         }
> 
> +       bitmap_size = max / BITS_PER_BYTE;
> +
> +       tmp = kvzalloc(bitmap_size, GFP_KERNEL_ACCOUNT);
> +       if (!tmp) {
> +               rc = -ENOMEM;
> +               goto out_put;
> +       }
> +
> +       if (copy_from_user(tmp, uptr, bitmap_size)) {
> +               rc = -EFAULT;
> +               goto out_free;
> +       }
> +
>         for (i = 0; i < max; i++) {
>                 unsigned long cur = iova + i * page_size;
>                 void *ent, *old;
> 
> -               if (!test_bit(i, (unsigned long *)uptr))
> +               if (!test_bit(i, (unsigned long *)tmp))
>                         continue;
> 
>                 ent = xa_load(&mock->pfns, cur / page_size);
> @@ -1138,6 +1151,8 @@ static int iommufd_test_dirty(struct iommufd_ucmd *ucmd,
> unsigned int mockpt_id,
> 
>         cmd->dirty.out_nr_dirty = count;
>         rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
> +out_free:
> +       kvfree(tmp);
>  out_put:
>         iommufd_put_object(&hwpt->obj);
>         return rc;