diff mbox series

kasan: Enable KUnit integration whenever CONFIG_KUNIT is enabled

Message ID 20221019085747.3810920-1-davidgow@google.com
State New
Headers
Series kasan: Enable KUnit integration whenever CONFIG_KUNIT is enabled |

Commit Message

David Gow Oct. 19, 2022, 8:57 a.m. UTC 
  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(-)

Comments

Andrey Konovalov Oct. 19, 2022, 2:17 p.m. UTC | #1 
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/
David Gow Oct. 19, 2022, 3:06 p.m. UTC | #2 
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
Andrey Konovalov Oct. 19, 2022, 7:48 p.m. UTC | #3 
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.
David Gow Oct. 20, 2022, 6:38 a.m. UTC | #4 
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 mbox series

Patch

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");