On 23/10/2023 21:50, Joao Martins wrote:
> On 23/10/2023 21:37, Nicolin Chen wrote:
>> On Mon, Oct 23, 2023 at 09:15:32PM +0100, Joao Martins wrote:
>>> 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?
>>
>
> 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.
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;
