| Message ID | 2w6pt2hnemndwmanwhyn3keexa6vtha7rmo6rqoerkmyxhbrh2@ls7lndjpia6z (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] i915/selftest/igt_mmap: let mmap tests run in kthread | expand |
Hi Mikolaj, On 2025-03-04 at 09:43:26 GMT, Mikolaj Wasiak wrote: > When the driver is loaded on the system with numa nodes it might be run in > a kthread, which makes it impossible to use current->mm in the selftest. > This patch allows the selftest to use current->mm by using active_mm. > > Signed-off-by: Mikolaj Wasiak <mikolaj.wasiak@intel.com> > Reviewed-by: Eugene Kobyak <eugene.kobyak@intel.com> > --- > v1 -> v2: Simplify logic of enabling and disabling active_mm > > drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c > index 804f74084bd4..9c3f17e51885 100644 > --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c > +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c > @@ -1837,6 +1837,8 @@ static int igt_mmap_revoke(void *arg) > > int i915_gem_mman_live_selftests(struct drm_i915_private *i915) > { > + int ret; > + bool unuse_mm = false; > static const struct i915_subtest tests[] = { > SUBTEST(igt_partial_tiling), > SUBTEST(igt_smoke_tiling), > @@ -1848,5 +1850,15 @@ int i915_gem_mman_live_selftests(struct drm_i915_private *i915) > SUBTEST(igt_mmap_gpu), > }; > > - return i915_live_subtests(tests, i915); > + if (!current->mm) { > + kthread_use_mm(current->active_mm); Don't we run into the same issue as in V1, meaning we use an unknown current->active_mm (since we run in a kthread, and cannot control it) to use as the current->mm? Maybe a better approach would be to create a new mm for the duration of the test, similarly to how the patch Janusz mentioned does it? (51104c19d857) Thanks Krzysztof
Hi Krzysztof, On 2025-03-05 at 17:31:49 +0100, Krzysztof Niemiec wrote: > Don't we run into the same issue as in V1, meaning we use an unknown > current->active_mm (since we run in a kthread, and cannot control it) to > use as the current->mm? Maybe a better approach would be to create a new > mm for the duration of the test, similarly to how the patch Janusz > mentioned does it? (51104c19d857) As per discussion with Chris, using active_mm is the correct way of enabling current->mm in kthread. On the other hand it may also expose issues with underlying tests because they didn't previously run on such hardware. I think potential fixes to those tests should be addressed in separate patch. Mikołaj
Quoting Mikolaj Wasiak (2025-03-07 08:44:29) > Hi Krzysztof, > > On 2025-03-05 at 17:31:49 +0100, Krzysztof Niemiec wrote: > > Don't we run into the same issue as in V1, meaning we use an unknown > > current->active_mm (since we run in a kthread, and cannot control it) to > > use as the current->mm? Maybe a better approach would be to create a new > > mm for the duration of the test, similarly to how the patch Janusz > > mentioned does it? (51104c19d857) > > As per discussion with Chris, using active_mm is the correct way of > enabling current->mm in kthread. On the other hand it may also expose > issues with underlying tests because they didn't previously run on such > hardware. I think potential fixes to those tests should be addressed in > separate patch. We've looked at the tests, and they should all be finding unused space in the mm and cleaning up after themselves... If we put on our paranoia hats, the biggest problem with borrowing userspace's mm is that it gives them temporary insight into whatever we place into that mm. We don't expose any data, unless by error... Not sure how much effort we want to put on making the selftests paranoia proof, but that (and the surety of cleaning up afterwards) would be a good argument for creating a temporary mm for our use. -Chris
On 2025-03-07 at 12:31:25 +0100, Chris Wilson wrote: > Quoting Mikolaj Wasiak (2025-03-07 08:44:29) > > Hi Krzysztof, > > > > On 2025-03-05 at 17:31:49 +0100, Krzysztof Niemiec wrote: > > > Don't we run into the same issue as in V1, meaning we use an unknown > > > current->active_mm (since we run in a kthread, and cannot control it) to > > > use as the current->mm? Maybe a better approach would be to create a new > > > mm for the duration of the test, similarly to how the patch Janusz > > > mentioned does it? (51104c19d857) > > > > As per discussion with Chris, using active_mm is the correct way of > > enabling current->mm in kthread. On the other hand it may also expose > > issues with underlying tests because they didn't previously run on such > > hardware. I think potential fixes to those tests should be addressed in > > separate patch. > > We've looked at the tests, and they should all be finding unused space > in the mm and cleaning up after themselves... > > If we put on our paranoia hats, the biggest problem with borrowing > userspace's mm is that it gives them temporary insight into whatever > we place into that mm. We don't expose any data, unless by error... > Not sure how much effort we want to put on making the selftests paranoia > proof, but that (and the surety of cleaning up afterwards) would be a > good argument for creating a temporary mm for our use. > -Chris I still don't know if it would be feasible to use methods that are exposed only to kunit to run our selftest. Do you think we should go that way? Mikołaj
On 2025-03-07 at 12:31:25 GMT, Chris Wilson wrote: > Quoting Mikolaj Wasiak (2025-03-07 08:44:29) > > Hi Krzysztof, > > > > On 2025-03-05 at 17:31:49 +0100, Krzysztof Niemiec wrote: > > > Don't we run into the same issue as in V1, meaning we use an unknown > > > current->active_mm (since we run in a kthread, and cannot control it) to > > > use as the current->mm? Maybe a better approach would be to create a new > > > mm for the duration of the test, similarly to how the patch Janusz > > > mentioned does it? (51104c19d857) > > > > As per discussion with Chris, using active_mm is the correct way of > > enabling current->mm in kthread. On the other hand it may also expose > > issues with underlying tests because they didn't previously run on such > > hardware. I think potential fixes to those tests should be addressed in > > separate patch. > > We've looked at the tests, and they should all be finding unused space > in the mm and cleaning up after themselves... > If that's the case, then the patch is alright. I was mostly worried about messing with userspace memory of a random process. > If we put on our paranoia hats, the biggest problem with borrowing > userspace's mm is that it gives them temporary insight into whatever > we place into that mm. We don't expose any data, unless by error... > Not sure how much effort we want to put on making the selftests paranoia > proof, but that (and the surety of cleaning up afterwards) would be a > good argument for creating a temporary mm for our use. I don't think it's really a secret what the selftest puts in that memory anyway (assuming normal test operation). The only problem I can see with using userspace's mm at the moment (paranoia-wise) is that we only lock the mm for the vma_lookup() check [1], meaning there's a time-of-check-time-of-use situation. IFF userspace can somehow unmap the obj from its side after that check is done, this can potentially mess with kernel memory. I have no earthly idea if this can be really abused though, so it might not even be a real issue. Besides, to achieve that, a malicious process would have to win the kthread active_mm lottery so its mm is used for the selftest, then guess the address of the mmapped object (it's technically logged as debug, but the loglevel might not be set as such), and then race with the kthread so the object is unmapped before use. So a lot of stars have to align. If that is not something we consider a problem, then: Reviewed-by: Krzysztof Niemiec <krzysztof.niemiec@intel.com> [1]: gem/selftests/i915_gem_mman.c:923 > -Chris Thanks Krzysztof
Hi, [...] > If that's the case, then the patch is alright. I was mostly worried > about messing with userspace memory of a random process. > > > If we put on our paranoia hats, the biggest problem with borrowing > > userspace's mm is that it gives them temporary insight into whatever > > we place into that mm. We don't expose any data, unless by error... > > Not sure how much effort we want to put on making the selftests paranoia > > proof, but that (and the surety of cleaning up afterwards) would be a > > good argument for creating a temporary mm for our use. > > I don't think it's really a secret what the selftest puts in that memory > anyway (assuming normal test operation). > > The only problem I can see with using userspace's mm at the moment > (paranoia-wise) is that we only lock the mm for the vma_lookup() check > [1], meaning there's a time-of-check-time-of-use situation. IFF > userspace can somehow unmap the obj from its side after that check is > done, this can potentially mess with kernel memory. I have no earthly > idea if this can be really abused though, so it might not even be a real > issue. > > Besides, to achieve that, a malicious process would have to win the > kthread active_mm lottery so its mm is used for the selftest, then guess > the address of the mmapped object (it's technically logged as debug, but > the loglevel might not be set as such), and then race with the kthread > so the object is unmapped before use. So a lot of stars have to align. [...] > [1]: gem/selftests/i915_gem_mman.c:923 Could a malicious process get all necessary information from logs (if available)? Successful guesswork and winning races seem unlikely to happen at the same time, but still possible. That being said, intentionally brute forcing search of these values seems even less likely to complete in the small window when this test runs, so: Reviewed-by: Krzysztof Karas <krzysztof.karas@intel.com> Best Regards, Krzysztof
diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c index 804f74084bd4..9c3f17e51885 100644 --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c @@ -1837,6 +1837,8 @@ static int igt_mmap_revoke(void *arg) int i915_gem_mman_live_selftests(struct drm_i915_private *i915) { + int ret; + bool unuse_mm = false; static const struct i915_subtest tests[] = { SUBTEST(igt_partial_tiling), SUBTEST(igt_smoke_tiling), @@ -1848,5 +1850,15 @@ int i915_gem_mman_live_selftests(struct drm_i915_private *i915) SUBTEST(igt_mmap_gpu), }; - return i915_live_subtests(tests, i915); + if (!current->mm) { + kthread_use_mm(current->active_mm); + unuse_mm = true; + } + + ret = i915_live_subtests(tests, i915); + + if (unuse_mm) + kthread_unuse_mm(current->active_mm); + + return ret; }