[PATCH] i915/selftest/igt_mmap: let mmap tests run in kthread

[Janusz Krzysztofik]

Hi,

On Wednesday, 26 February 2025 13:41:34 CET Krzysztof Niemiec wrote:
> On 2025-02-25 at 11:41:56 GMT, Mikolaj Wasiak wrote:
> > When driver is loaded on system with numa nodes it might be run in 
kthread.
> > This makes it impossible to use current->mm in selftests which currently
> > creates null pointer exception.
> > This patch allows selftest to use current->mm by using active_mm.
> > 
> > Signed-off-by: Mikolaj Wasiak <mikolaj.wasiak at intel.com>
> > ---
> > 
> > This patch is mostly damage control. By using active_mm we expose our
> > test to foreign memory mapping, which sometimes makes the test fail.
> > That is still better than just having null pointer exception in driver
> > code.
> > 
> 
> We talked about this offline a bit, I'll recount what we determined here
> just for the record.
> 
> On NUMA systems, the driver might be probed via a kthread consuming a
> workqueue. This shouldn't be a problem as long as we don't rely on
> current->mm (i915 usually doesn't, unless it's ioctls where it's fair
> game) - but this test wasn't written with that in mind, hence the
> derefs.
> 
> We can't just use current->active_mm in place of current->mm inside of
> the test code, because one of the functions that the test uses
> (vma_lookup to be exact) ends up using current->mm. That's too deep
> into the kernel to ever touch it with such a patch.
> 
> The problem is, we also can't just set current->mm of the kthread
> to current->active_mm, because God knows what that value can be (again,
> we are executing in some random kthread that just happened to work on
> the probe).
> 
> So I don't think this patch is a good idea; it IS better than just
> having a NULL deref, but since active_mm is being so unreliable here,
> that kind of defeats the point of running the test. Also, since this is
> an mmap test and it does stuff like user pointers, it does not really
> make much sense to run it in a kthread (unless it's explicitly forked
> from a context, current->mm of which we have control over, but that's
> not what's happening here).
> 
> I have two suggestions for a different fix:
> 
> 1. Disable the test on systems with NUMA and/or if it's running in a
>    kthread, on the premise that it doesn't make sense to run this
>    specific test in a kthread. This is the easier option.
> 
> 2. Find a way to pass an argument to the selftest, so we can pass a
>    known mm to the test thread. Then set it as current->mm for the
>    duration of the test like you're doing in your patch. We could pass
>    the IGT process's mm to "emulate" having it being the initializer of
>    the test task; this way we're being a good citizen and we don't mess
>    with some other process memory. I figure this is the harder option.
> 
> I'd try 2 and if it doesn't work then just resort to 1 if there's no
> better idea floating around.

I agree with both Andi and Krzysztof comments.

If the issue is tracked in our bug tracker then please provide a link to its 
record in a Link: or even Closes: tag.  Do you have call traces on hand?  
Probably yes, so please consider adding a concise excerpt to your description.

While looking for similar cases, I've found commit 51104c19d857 ("kunit: test: 
Add vm_mmap() allocation resource manager") that seems to have resolved a 
similar issue for then newly added kunit tests accessing current->mm.  Maybe 
the approach used there is worth of reusing it for i915 selftests.

Thanks,
Janusz


> 
> Thanks
> Krzysztof
>