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

[Nicolin Chen <nicolinc@nvidia.com>]

On Mon, Oct 23, 2023 at 09:15:32PM +0100, Joao Martins wrote:
> External email: Use caution opening links or attachments
> 
> 
> On 23/10/2023 21:08, Nicolin Chen wrote:
> > On Fri, Oct 20, 2023 at 11:28:02PM +0100, Joao Martins wrote:
> >
> >> +static int iommufd_test_dirty(struct iommufd_ucmd *ucmd,
> >> +                             unsigned int mockpt_id, unsigned long iova,
> >> +                             size_t length, unsigned long page_size,
> >> +                             void __user *uptr, u32 flags)
> >> +{
> >> +       unsigned long 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;
> >> +
> >> +       if (iova % page_size || length % page_size ||
> >> +           (uintptr_t)uptr % page_size)
> >> +               return -EINVAL;
> >> +
> >> +       hwpt = get_md_pagetable(ucmd, mockpt_id, &mock);
> >> +       if (IS_ERR(hwpt))
> >> +               return PTR_ERR(hwpt);
> >> +
> >> +       if (!(mock->flags & MOCK_DIRTY_TRACK)) {
> >> +               rc = -EINVAL;
> >> +               goto out_put;
> >> +       }
> >> +
> >> +       for (i = 0; i < max; i++) {
> >> +               unsigned long cur = iova + i * page_size;
> >> +               void *ent, *old;
> >> +
> >> +               if (!test_bit(i, (unsigned long *) uptr))
> >> +                       continue;
> >
> > Is it okay to test_bit on a user pointer/page? Should we call
> > get_user_pages or so?
> >
> Arggh, let me fix that.
> 
> This is where it is failing the selftest for you?
> 
> If so, I should paste a snippet for you to test.

Yea, the crash seems to be caused by this. Possibly some memory
debugging feature that I turned on caught this?

I tried a test fix and the crash is gone (attaching at EOM).

However, I still see other failures:
# #  RUN           iommufd_dirty_tracking.domain_dirty128M.get_dirty_bitmap ...
# # iommufd_utils.h:292:get_dirty_bitmap:Expected nr (32768) == out_dirty (13648)
# # get_dirty_bitmap: Test terminated by assertion
# #          FAIL  iommufd_dirty_tracking.domain_dirty128M.get_dirty_bitmap
# not ok 147 iommufd_dirty_tracking.domain_dirty128M.get_dirty_bitmap
# #  RUN           iommufd_dirty_tracking.domain_dirty256M.enforce_dirty ...
# #            OK  iommufd_dirty_tracking.domain_dirty256M.enforce_dirty
# ok 148 iommufd_dirty_tracking.domain_dirty256M.enforce_dirty
# #  RUN           iommufd_dirty_tracking.domain_dirty256M.set_dirty_tracking ...
# #            OK  iommufd_dirty_tracking.domain_dirty256M.set_dirty_tracking
# ok 149 iommufd_dirty_tracking.domain_dirty256M.set_dirty_tracking
# #  RUN           iommufd_dirty_tracking.domain_dirty256M.device_dirty_capability ...
# #            OK  iommufd_dirty_tracking.domain_dirty256M.device_dirty_capability
# ok 150 iommufd_dirty_tracking.domain_dirty256M.device_dirty_capability
# #  RUN           iommufd_dirty_tracking.domain_dirty256M.get_dirty_bitmap ...
# # iommufd_utils.h:292:get_dirty_bitmap:Expected nr (65536) == out_dirty (8923)


Maybe page_size isn't the right size?

-------------attaching copy_from_user------------
diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c
index 8a2c7df85441..daa198809d61 100644
--- a/drivers/iommu/iommufd/selftest.c
+++ b/drivers/iommu/iommufd/selftest.c
@@ -1103,8 +1103,9 @@ static int iommufd_test_dirty(struct iommufd_ucmd *ucmd, unsigned int mockpt_id,
 	struct iommufd_hw_pagetable *hwpt;
 	struct mock_iommu_domain *mock;
 	int rc, count = 0;
+	void *tmp;
 
-	if (iova % page_size || length % page_size ||
+	if (iova % page_size || length % page_size || !uptr ||
 	    (uintptr_t)uptr % page_size)
 		return -EINVAL;
 
@@ -1117,11 +1118,22 @@ static int iommufd_test_dirty(struct iommufd_ucmd *ucmd, unsigned int mockpt_id,
 		goto out_put;
 	}
 
+	tmp = kvzalloc(page_size, GFP_KERNEL_ACCOUNT);
+	if (!tmp) {
+		rc = -ENOMEM;
+		goto out_put;
+	}
+
 	for (i = 0; i < max; i++) {
 		unsigned long cur = iova + i * page_size;
 		void *ent, *old;
 
-		if (!test_bit(i, (unsigned long *)uptr))
+		if (copy_from_user(tmp, uptr, page_size)) {
+			rc = -EFAULT;
+			goto out_free;
+		}
+
+		if (!test_bit(i, (unsigned long *)tmp))
 			continue;
 
 		ent = xa_load(&mock->pfns, cur / page_size);
@@ -1138,6 +1150,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;