Re: [PATCH v2 2/2] kasan: apply store-only mode in kasan kunit testcases

[Yeoreum Yun <yeoreum.yun@arm.com>]

Hi Andrey,

> > > > +/*
> > > > + * KUNIT_EXPECT_KASAN_SUCCESS - check that the executed expression doesn't
> > > > + * produces a KASAN report; causes a KUnit test failure otherwise.
> > >
> > > Should be no need for this, the existing functionality already checks
> > > that there are no reports outside of KUNIT_EXPECT_KASAN_FAIL().
> >
> > This is function's purpose is to print failure situtations:
> >   - KASAN should reports but no report is found.
> >   - KASAN shouldn't report but there report is found.
> >
> > To print the second error, the "TEMPLATE" macro is added.
> > not just checking the no report but to check whether report was
> > generated as expected.
>
> There's no need to an explicit wrapper for detecting the second case.
> If there's a KASAN report printed outside of
> KUNIT_EXPECT_KASAN_FAIL(), either the next KUNIT_EXPECT_KASAN_FAIL()
> or kasan_test_exit() will detect this.

Sorry for bothering you, But I'm not sure whether
I understood your suggetion but that's sound of implentation like:

+#ifdef CONFIG_KASAN_HW_TAGS
+#define KUNIT_EXPECT_KASAN_FAIL_READ(test, expression) do {            \
+       if (!kasan_enabled_store_only()) {                              \
+               KUNIT_EXPECT_KASAN_FAIL(test, expression);              \
+               goto ____skip;                                          \
+       }                                                               \
+       if (kasan_sync_fault_possible())                                \
+               migrate_disable();                                      \
+       KUNIT_EXPECT_FALSE(test, READ_ONCE(test_status.report_found));  \
+       barrier();                                                      \
+       expression;                                                     \
+       barrier();                                                      \
+       if (kasan_sync_fault_possible())                                \
+               migrate_enable();                                       \
+___skip:                                                               \
+} while (0)
+#else
+#define KUNIT_EXPECT_KASAN_FAIL_READ(test, expression) \
+       KUNIT_EXPECT_KASAN_FAIL(test, expression)
+#endif

and you expect the "Error print" on the next KUNIT_EXPECT_KASAN_FAIL's
  KUNIT_EXPECT_FALSE(test, READ_ONCE(test_status.report_found));
or kasan_test_exit().

this maybe work, but it wouldn't print the proper "expression" and
seems like reporting the problem in another place from it happens
(at least source line printing is different from
where it happens -- KUNIT_EXPECT_KASAN_FAIL_READ() and
where it reports -- KUNIT_EXPECT_FALSE()).

Also, some of test case using atomic, kasan_enabled_store_only() can
use for KUNIT_EXPECT_KASAN_FAIL()
i.e) atomic_set() which allocated with the sizeof 42 (writing on
redzone).

That's why I think it would be better to use like with
sustaining _KUNIT_EXPECT_KASAN_TEMPLATE:

+/*
+ * KUNIT_EXPECT_KASAN_FAIL - check that the executed expression produces a
+ * KASAN report; causes a KUnit test failure otherwise.
+ *
+ * @test: Currently executing KUnit test.
+ * @expr: Expression produce a KASAN report.
+ */
+#define KUNIT_EXPECT_KASAN_FAIL(test, expr)                    \
+       _KUNIT_EXPECT_KASAN_TEMPLATE(test, expr, #expr, true)
+
+/*
+ * KUNIT_EXPECT_KASAN_FAIL_READ - check that the executed expression produces
+ * a KASAN report for read access.
+ * It causes a KUnit test failure. if KASAN report isn't produced for read access.
+ * For write access, it cause a KUnit test failure if a KASAN report is produced
+ *
+ * @test: Currently executing KUnit test.
+ * @expr: Expression doesn't produce a KASAN report.
+ */
+#define KUNIT_EXPECT_KASAN_FAIL_READ(test, expr)                       \
+       _KUNIT_EXPECT_KASAN_TEMPLATE(test, expr, #expr,                 \
+                       !kasan_store_only_enabled())                    \

Am I misunderstading?

Thanks.

--
Sincerely,
Yeoreum Yun