| Message ID | 20221019085747.3810920-1-davidgow@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | kasan: Enable KUnit integration whenever CONFIG_KUNIT is enabled | expand |
On Wed, Oct 19, 2022 at 10:58 AM 'David Gow' via kasan-dev <kasan-dev@googlegroups.com> wrote: > > Enable the KASAN/KUnit integration even when the KASAN tests are > disabled, as it's useful for testing other things under KASAN. > Essentially, this reverts commit 49d9977ac909 ("kasan: check CONFIG_KASAN_KUNIT_TEST instead of CONFIG_KUNIT"). > > To mitigate the performance impact slightly, add a likely() to the check > for a currently running test. > > There's more we can do for performance if/when it becomes more of a > problem, such as only enabling the "expect a KASAN failure" support wif > the KASAN tests are enabled, or putting the whole thing behind a "kunit > tests are running" static branch (which I do plan to do eventually). > > Fixes: 49d9977ac909 ("kasan: check CONFIG_KASAN_KUNIT_TEST instead of CONFIG_KUNIT") > Signed-off-by: David Gow <davidgow@google.com> > --- > > Basically, hiding the KASAN/KUnit integration broke being able to just > pass --kconfig_add CONFIG_KASAN=y to kunit_tool to enable KASAN > integration. We didn't notice this, because usually > CONFIG_KUNIT_ALL_TESTS is enabled, which in turn enables > CONFIG_KASAN_KUNIT_TEST. However, using a separate .kunitconfig might > result in failures being missed. > > Take, for example: > ./tools/testing/kunit/kunit.py run --kconfig_add CONFIG_KASAN=y \ > --kunitconfig drivers/gpu/drm/tests > > This should run the drm tests with KASAN enabled, but even if there's a > KASAN failure (such as the one fixed by [1]), kunit_tool will report > success. Hi David, How does KUnit detect a KASAN failure for other tests than the KASAN ones? I thought this was only implemented for KASAN tests. At least, I don't see any code querying kunit_kasan_status outside of KASAN tests. I'm currently switching KASAN tests from using KUnit resources to console tracepoints [1], and those patches will be in conflict with yours. Thanks! [1] https://lore.kernel.org/linux-mm/ebf96ea600050f00ed567e80505ae8f242633640.1666113393.git.andreyknvl@google.com/
On Wed, Oct 19, 2022 at 10:18 PM Andrey Konovalov <andreyknvl@gmail.com> wrote: > > On Wed, Oct 19, 2022 at 10:58 AM 'David Gow' via kasan-dev > <kasan-dev@googlegroups.com> wrote: > > > > Enable the KASAN/KUnit integration even when the KASAN tests are > > disabled, as it's useful for testing other things under KASAN. > > Essentially, this reverts commit 49d9977ac909 ("kasan: check CONFIG_KASAN_KUNIT_TEST instead of CONFIG_KUNIT"). > > > > To mitigate the performance impact slightly, add a likely() to the check > > for a currently running test. > > > > There's more we can do for performance if/when it becomes more of a > > problem, such as only enabling the "expect a KASAN failure" support wif > > the KASAN tests are enabled, or putting the whole thing behind a "kunit > > tests are running" static branch (which I do plan to do eventually). > > > > Fixes: 49d9977ac909 ("kasan: check CONFIG_KASAN_KUNIT_TEST instead of CONFIG_KUNIT") > > Signed-off-by: David Gow <davidgow@google.com> > > --- > > > > Basically, hiding the KASAN/KUnit integration broke being able to just > > pass --kconfig_add CONFIG_KASAN=y to kunit_tool to enable KASAN > > integration. We didn't notice this, because usually > > CONFIG_KUNIT_ALL_TESTS is enabled, which in turn enables > > CONFIG_KASAN_KUNIT_TEST. However, using a separate .kunitconfig might > > result in failures being missed. > > > > Take, for example: > > ./tools/testing/kunit/kunit.py run --kconfig_add CONFIG_KASAN=y \ > > --kunitconfig drivers/gpu/drm/tests > > > > This should run the drm tests with KASAN enabled, but even if there's a > > KASAN failure (such as the one fixed by [1]), kunit_tool will report > > success. > > Hi David, > > How does KUnit detect a KASAN failure for other tests than the KASAN > ones? I thought this was only implemented for KASAN tests. At least, I > don't see any code querying kunit_kasan_status outside of KASAN tests. Yeah, there aren't any other tests which set up a "kasan_status" resource to expect specific failures, but we still want the fallback call to kunit_set_failure() so that any test which causes a KASAN report will fail: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/mm/kasan/report.c#n130 > I'm currently switching KASAN tests from using KUnit resources to > console tracepoints [1], and those patches will be in conflict with > yours. Ah, sorry -- I'd seen these go past, and totally forgot about them! I think all we really want to keep is the ability to fail tests if a KASAN report occurs. The tricky bit is then disabling that for the KASAN tests, so that they can have "expected" failures. -- David
On Wed, Oct 19, 2022 at 5:06 PM David Gow <davidgow@google.com> wrote: > > > How does KUnit detect a KASAN failure for other tests than the KASAN > > ones? I thought this was only implemented for KASAN tests. At least, I > > don't see any code querying kunit_kasan_status outside of KASAN tests. > > Yeah, there aren't any other tests which set up a "kasan_status" > resource to expect specific failures, but we still want the fallback > call to kunit_set_failure() so that any test which causes a KASAN > report will fail: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/mm/kasan/report.c#n130 Ah, right. Thanks for the explanation! > > I'm currently switching KASAN tests from using KUnit resources to > > console tracepoints [1], and those patches will be in conflict with > > yours. > > Ah, sorry -- I'd seen these go past, and totally forgot about them! I > think all we really want to keep is the ability to fail tests if a > KASAN report occurs. The tricky bit is then disabling that for the > KASAN tests, so that they can have "expected" failures. I wonder what's the best solution to support this, assuming KASAN tests are switched to using tracepoints... I guess we could still keep the per-task KUnit flag, and only use it for non-KASAN tests. However, they will still suffer from the same issue tracepoints solve for KASAN tests: if a bug is triggered in a context other than the current task, the test will succeed.
On Thu, Oct 20, 2022 at 3:48 AM Andrey Konovalov <andreyknvl@gmail.com> wrote: > > On Wed, Oct 19, 2022 at 5:06 PM David Gow <davidgow@google.com> wrote: > > > > > How does KUnit detect a KASAN failure for other tests than the KASAN > > > ones? I thought this was only implemented for KASAN tests. At least, I > > > don't see any code querying kunit_kasan_status outside of KASAN tests. > > > > Yeah, there aren't any other tests which set up a "kasan_status" > > resource to expect specific failures, but we still want the fallback > > call to kunit_set_failure() so that any test which causes a KASAN > > report will fail: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/mm/kasan/report.c#n130 > > Ah, right. Thanks for the explanation! > > > > I'm currently switching KASAN tests from using KUnit resources to > > > console tracepoints [1], and those patches will be in conflict with > > > yours. > > > > Ah, sorry -- I'd seen these go past, and totally forgot about them! I > > think all we really want to keep is the ability to fail tests if a > > KASAN report occurs. The tricky bit is then disabling that for the > > KASAN tests, so that they can have "expected" failures. > > I wonder what's the best solution to support this, assuming KASAN > tests are switched to using tracepoints... I guess we could still keep > the per-task KUnit flag, and only use it for non-KASAN tests. However, > they will still suffer from the same issue tracepoints solve for KASAN > tests: if a bug is triggered in a context other than the current task, > the test will succeed. Yeah: I'm not sure what the perfect solution here is. Ideally, we'd have some good way to get the current test, which would work even in workqueues, rcu, etc. This affects more than just KASAN: there are quite a few different places where getting "the current test" is important. One option is just to use a global: we don't support running multiple simultaneous KUnit tests at all, at the moment. But, equally, it increases the possibility of false-positives if something non-test related needs to access the test structure. This is probably not too much of a problem for KASAN, but the function redirection features we're working on benefit quite a bit from those redirections not being enabled outside of the test. Thus far, we've just sort-of accepted that these don't work with tests which push work to other tasks, but it is sub-optimal. And even if KASAN moves to tracepoints, this problem doesn't totally go away, as you still need some way to know you're in the KASAN test to disable the "fail-test-on-KASAN-report" behaviour. I guess that could be some global flag triggered from the suite_init / suite_exit for the KASAN test, though. Cheers, -- David
diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h index abbcc1b0eec5..afacef14c7f4 100644 --- a/mm/kasan/kasan.h +++ b/mm/kasan/kasan.h @@ -261,7 +261,7 @@ struct kasan_stack_ring { #endif /* CONFIG_KASAN_SW_TAGS || CONFIG_KASAN_HW_TAGS */ -#if IS_ENABLED(CONFIG_KASAN_KUNIT_TEST) +#if IS_ENABLED(CONFIG_KUNIT) /* Used in KUnit-compatible KASAN tests. */ struct kunit_kasan_status { bool report_found; diff --git a/mm/kasan/report.c b/mm/kasan/report.c index df3602062bfd..efa063b9d093 100644 --- a/mm/kasan/report.c +++ b/mm/kasan/report.c @@ -114,7 +114,7 @@ EXPORT_SYMBOL_GPL(kasan_restore_multi_shot); #endif -#if IS_ENABLED(CONFIG_KASAN_KUNIT_TEST) +#if IS_ENABLED(CONFIG_KUNIT) static void update_kunit_status(bool sync) { struct kunit *test; @@ -122,7 +122,7 @@ static void update_kunit_status(bool sync) struct kunit_kasan_status *status; test = current->kunit_test; - if (!test) + if (likely(!test)) return; resource = kunit_find_named_resource(test, "kasan_status");
Enable the KASAN/KUnit integration even when the KASAN tests are disabled, as it's useful for testing other things under KASAN. Essentially, this reverts commit 49d9977ac909 ("kasan: check CONFIG_KASAN_KUNIT_TEST instead of CONFIG_KUNIT"). To mitigate the performance impact slightly, add a likely() to the check for a currently running test. There's more we can do for performance if/when it becomes more of a problem, such as only enabling the "expect a KASAN failure" support wif the KASAN tests are enabled, or putting the whole thing behind a "kunit tests are running" static branch (which I do plan to do eventually). Fixes: 49d9977ac909 ("kasan: check CONFIG_KASAN_KUNIT_TEST instead of CONFIG_KUNIT") Signed-off-by: David Gow <davidgow@google.com> --- Basically, hiding the KASAN/KUnit integration broke being able to just pass --kconfig_add CONFIG_KASAN=y to kunit_tool to enable KASAN integration. We didn't notice this, because usually CONFIG_KUNIT_ALL_TESTS is enabled, which in turn enables CONFIG_KASAN_KUNIT_TEST. However, using a separate .kunitconfig might result in failures being missed. Take, for example: ./tools/testing/kunit/kunit.py run --kconfig_add CONFIG_KASAN=y \ --kunitconfig drivers/gpu/drm/tests This should run the drm tests with KASAN enabled, but even if there's a KASAN failure (such as the one fixed by [1]), kunit_tool will report success. [1]: https://lore.kernel.org/dri-devel/20221019073239.3779180-1-davidgow@google.com/ --- mm/kasan/kasan.h | 2 +- mm/kasan/report.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)