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;
