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@xxxxxxxxx>
> > ---
> >
> > 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
>
