It might not be obvious to the compiler that the expression must be
executed between writing and reading to fail_data. In this case, the
compiler might reorder or optimize away some of the accesses, and
the tests will fail.
Add compiler barriers around the expression in KUNIT_EXPECT_KASAN_FAIL.
Signed-off-by: Andrey Konovalov <[email protected]>
Link: https://linux-review.googlesource.com/id/I046079f48641a1d36fe627fc8827a9249102fd50
---
lib/test_kasan.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/lib/test_kasan.c b/lib/test_kasan.c
index dd3d2f95c24e..b5077a47b95a 100644
--- a/lib/test_kasan.c
+++ b/lib/test_kasan.c
@@ -79,7 +79,9 @@ static void kasan_test_exit(struct kunit *test)
NULL, \
&resource, \
"kasan_data", &fail_data); \
+ barrier(); \
expression; \
+ barrier(); \
KUNIT_EXPECT_EQ(test, \
fail_data.report_expected, \
fail_data.report_found); \
--
2.29.2.729.g45daf8777d-goog
On Tue, Jan 5, 2021 at 7:28 PM Andrey Konovalov <[email protected]> wrote:
>
> It might not be obvious to the compiler that the expression must be
> executed between writing and reading to fail_data. In this case, the
> compiler might reorder or optimize away some of the accesses, and
> the tests will fail.
Have you seen this happen in practice?
Are these accesses to fail_data that are optimized (in which case we
could make it volatile), or some part of the expression?
Note that compiler barriers won't probably help against removing
memory accesses, they only prevent reordering.
> + barrier(); \
> expression; \
> + barrier(); \
The need for barriers is not obvious to the reader, so a comment in
the code clarifying that would be nice.
On Tue, Jan 05, 2021 at 07:27PM +0100, Andrey Konovalov wrote:
> It might not be obvious to the compiler that the expression must be
> executed between writing and reading to fail_data. In this case, the
> compiler might reorder or optimize away some of the accesses, and
> the tests will fail.
>
> Add compiler barriers around the expression in KUNIT_EXPECT_KASAN_FAIL.
>
> Signed-off-by: Andrey Konovalov <[email protected]>
> Link: https://linux-review.googlesource.com/id/I046079f48641a1d36fe627fc8827a9249102fd50
Reviewed-by: Marco Elver <[email protected]>
> ---
> lib/test_kasan.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/lib/test_kasan.c b/lib/test_kasan.c
> index dd3d2f95c24e..b5077a47b95a 100644
> --- a/lib/test_kasan.c
> +++ b/lib/test_kasan.c
> @@ -79,7 +79,9 @@ static void kasan_test_exit(struct kunit *test)
> NULL, \
> &resource, \
> "kasan_data", &fail_data); \
> + barrier(); \
> expression; \
> + barrier(); \
> KUNIT_EXPECT_EQ(test, \
> fail_data.report_expected, \
> fail_data.report_found); \
> --
> 2.29.2.729.g45daf8777d-goog
>
On Tue, Jan 12, 2021 at 9:18 AM Alexander Potapenko <[email protected]> wrote:
>
> On Tue, Jan 5, 2021 at 7:28 PM Andrey Konovalov <[email protected]> wrote:
> >
> > It might not be obvious to the compiler that the expression must be
> > executed between writing and reading to fail_data. In this case, the
> > compiler might reorder or optimize away some of the accesses, and
> > the tests will fail.
>
> Have you seen this happen in practice?
Yes.
> Are these accesses to fail_data that are optimized (in which case we
> could make it volatile)?
Yes. AFAIU compiler doesn't expect expression to change fail_data
fields, no those accesses and checks are optimized away.
> Note that compiler barriers won't probably help against removing
> memory accesses, they only prevent reordering.
>
> > + barrier(); \
> > expression; \
> > + barrier(); \
>
> The need for barriers is not obvious to the reader, so a comment in
> the code clarifying that would be nice.
Will add a comment in v2, thanks!
On Tue, Jan 12, 2021 at 8:50 PM Andrey Konovalov <[email protected]> wrote:
>
> On Tue, Jan 12, 2021 at 9:18 AM Alexander Potapenko <[email protected]> wrote:
> >
> > On Tue, Jan 5, 2021 at 7:28 PM Andrey Konovalov <[email protected]> wrote:
> > >
> > > It might not be obvious to the compiler that the expression must be
> > > executed between writing and reading to fail_data. In this case, the
> > > compiler might reorder or optimize away some of the accesses, and
> > > the tests will fail.
> >
> > Have you seen this happen in practice?
>
> Yes.
>
> > Are these accesses to fail_data that are optimized (in which case we
> > could make it volatile)?
>
> Yes. AFAIU compiler doesn't expect expression to change fail_data
> fields, no those accesses and checks are optimized away.
Ah, actually no, it reorders the expression and puts it after
fail_data fields checks. That's why I put the barriers.
> > Note that compiler barriers won't probably help against removing
> > memory accesses, they only prevent reordering.
But using WRITE/READ_ONCE() might also be a good idea, as technically
the compiler can optimize away the accesses.