* [PATCH 2/2] kmsan: add test_stackdepot_roundtrip
2023-03-06 11:13 [PATCH 1/2] lib/stackdepot: kmsan: mark API outputs as initialized Alexander Potapenko
@ 2023-03-06 11:13 ` Alexander Potapenko
2023-03-06 11:37 ` [PATCH 1/2] lib/stackdepot: kmsan: mark API outputs as initialized Dmitry Vyukov
2023-03-10 23:50 ` Andrey Konovalov
2 siblings, 0 replies; 4+ messages in thread
From: Alexander Potapenko @ 2023-03-06 11:13 UTC (permalink / raw)
To: glider; +Cc: linux-kernel, linux-mm, akpm, elver, dvyukov, kasan-dev
Ensure that KMSAN does not report false positives in instrumented callers
of stack_depot_save(), stack_depot_print(), and stack_depot_fetch().
Signed-off-by: Alexander Potapenko <glider@google.com>
---
mm/kmsan/kmsan_test.c | 31 +++++++++++++++++++++++++++++++
1 file changed, 31 insertions(+)
diff --git a/mm/kmsan/kmsan_test.c b/mm/kmsan/kmsan_test.c
index 7095d3fbb23ac..d9eb141c27aa4 100644
--- a/mm/kmsan/kmsan_test.c
+++ b/mm/kmsan/kmsan_test.c
@@ -551,6 +551,36 @@ static void test_long_origin_chain(struct kunit *test)
KUNIT_EXPECT_TRUE(test, report_matches(&expect));
}
+/*
+ * Test case: ensure that saving/restoring/printing stacks to/from stackdepot
+ * does not trigger errors.
+ *
+ * KMSAN uses stackdepot to store origin stack traces, that's why we do not
+ * instrument lib/stackdepot.c. Yet it must properly mark its outputs as
+ * initialized because other kernel features (e.g. netdev tracker) may also
+ * access stackdepot from instrumented code.
+ */
+static void test_stackdepot_roundtrip(struct kunit *test)
+{
+ unsigned long src_entries[16], *dst_entries;
+ unsigned int src_nentries, dst_nentries;
+ EXPECTATION_NO_REPORT(expect);
+ depot_stack_handle_t handle;
+
+ kunit_info(test, "testing stackdepot roundtrip (no reports)\n");
+
+ src_nentries =
+ stack_trace_save(src_entries, ARRAY_SIZE(src_entries), 1);
+ handle = stack_depot_save(src_entries, src_nentries, GFP_KERNEL);
+ stack_depot_print(handle);
+ dst_nentries = stack_depot_fetch(handle, &dst_entries);
+ KUNIT_EXPECT_TRUE(test, src_nentries == dst_nentries);
+
+ kmsan_check_memory((void *)dst_entries,
+ sizeof(*dst_entries) * dst_nentries);
+ KUNIT_EXPECT_TRUE(test, report_matches(&expect));
+}
+
static struct kunit_case kmsan_test_cases[] = {
KUNIT_CASE(test_uninit_kmalloc),
KUNIT_CASE(test_init_kmalloc),
@@ -573,6 +603,7 @@ static struct kunit_case kmsan_test_cases[] = {
KUNIT_CASE(test_memset32),
KUNIT_CASE(test_memset64),
KUNIT_CASE(test_long_origin_chain),
+ KUNIT_CASE(test_stackdepot_roundtrip),
{},
};
--
2.40.0.rc0.216.gc4246ad0f0-goog
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH 1/2] lib/stackdepot: kmsan: mark API outputs as initialized
2023-03-06 11:13 [PATCH 1/2] lib/stackdepot: kmsan: mark API outputs as initialized Alexander Potapenko
2023-03-06 11:13 ` [PATCH 2/2] kmsan: add test_stackdepot_roundtrip Alexander Potapenko
@ 2023-03-06 11:37 ` Dmitry Vyukov
2023-03-10 23:50 ` Andrey Konovalov
2 siblings, 0 replies; 4+ messages in thread
From: Dmitry Vyukov @ 2023-03-06 11:37 UTC (permalink / raw)
To: Alexander Potapenko
Cc: linux-kernel, linux-mm, akpm, elver, kasan-dev, Andrey Konovalov
On Mon, 6 Mar 2023 at 12:13, Alexander Potapenko <glider@google.com> wrote:
>
> KMSAN does not instrument stackdepot and may treat memory allocated by
> it as uninitialized. This is not a problem for KMSAN itself, because its
> functions calling stackdepot API are also not instrumented.
> But other kernel features (e.g. netdev tracker) may access stack depot
> from instrumented code, which will lead to false positives, unless we
> explicitly mark stackdepot outputs as initialized.
>
> Cc: Andrey Konovalov <andreyknvl@gmail.com>
> Cc: Marco Elver <elver@google.com>
> Suggested-by: Dmitry Vyukov <dvyukov@google.com>
> Signed-off-by: Alexander Potapenko <glider@google.com>
Add:
Reported-by: syzbot <syzkaller@googlegroups.com>
Otherwise:
Reviewed-by: Dmitry Vyukov <dvyukov@google.com>
> ---
> lib/stackdepot.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/lib/stackdepot.c b/lib/stackdepot.c
> index 036da8e295d19..2f5aa851834eb 100644
> --- a/lib/stackdepot.c
> +++ b/lib/stackdepot.c
> @@ -17,6 +17,7 @@
> #include <linux/gfp.h>
> #include <linux/jhash.h>
> #include <linux/kernel.h>
> +#include <linux/kmsan.h>
> #include <linux/mm.h>
> #include <linux/mutex.h>
> #include <linux/percpu.h>
> @@ -306,6 +307,11 @@ depot_alloc_stack(unsigned long *entries, int size, u32 hash, void **prealloc)
> stack->handle.extra = 0;
> memcpy(stack->entries, entries, flex_array_size(stack, entries, size));
> pool_offset += required_size;
> + /*
> + * Let KMSAN know the stored stack record is initialized. This shall
> + * prevent false positive reports if instrumented code accesses it.
> + */
> + kmsan_unpoison_memory(stack, required_size);
>
> return stack;
> }
> @@ -465,6 +471,12 @@ unsigned int stack_depot_fetch(depot_stack_handle_t handle,
> struct stack_record *stack;
>
> *entries = NULL;
> + /*
> + * Let KMSAN know *entries is initialized. This shall prevent false
> + * positive reports if instrumented code accesses it.
> + */
> + kmsan_unpoison_memory(entries, sizeof(*entries));
> +
> if (!handle)
> return 0;
>
> --
> 2.40.0.rc0.216.gc4246ad0f0-goog
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/2] lib/stackdepot: kmsan: mark API outputs as initialized
2023-03-06 11:13 [PATCH 1/2] lib/stackdepot: kmsan: mark API outputs as initialized Alexander Potapenko
2023-03-06 11:13 ` [PATCH 2/2] kmsan: add test_stackdepot_roundtrip Alexander Potapenko
2023-03-06 11:37 ` [PATCH 1/2] lib/stackdepot: kmsan: mark API outputs as initialized Dmitry Vyukov
@ 2023-03-10 23:50 ` Andrey Konovalov
2 siblings, 0 replies; 4+ messages in thread
From: Andrey Konovalov @ 2023-03-10 23:50 UTC (permalink / raw)
To: Alexander Potapenko
Cc: linux-kernel, linux-mm, akpm, elver, dvyukov, kasan-dev
On Mon, Mar 6, 2023 at 12:13 PM Alexander Potapenko <glider@google.com> wrote:
>
> KMSAN does not instrument stackdepot and may treat memory allocated by
> it as uninitialized. This is not a problem for KMSAN itself, because its
> functions calling stackdepot API are also not instrumented.
> But other kernel features (e.g. netdev tracker) may access stack depot
> from instrumented code, which will lead to false positives, unless we
> explicitly mark stackdepot outputs as initialized.
>
> Cc: Andrey Konovalov <andreyknvl@gmail.com>
> Cc: Marco Elver <elver@google.com>
> Suggested-by: Dmitry Vyukov <dvyukov@google.com>
> Signed-off-by: Alexander Potapenko <glider@google.com>
> ---
> lib/stackdepot.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/lib/stackdepot.c b/lib/stackdepot.c
> index 036da8e295d19..2f5aa851834eb 100644
> --- a/lib/stackdepot.c
> +++ b/lib/stackdepot.c
> @@ -17,6 +17,7 @@
> #include <linux/gfp.h>
> #include <linux/jhash.h>
> #include <linux/kernel.h>
> +#include <linux/kmsan.h>
> #include <linux/mm.h>
> #include <linux/mutex.h>
> #include <linux/percpu.h>
> @@ -306,6 +307,11 @@ depot_alloc_stack(unsigned long *entries, int size, u32 hash, void **prealloc)
> stack->handle.extra = 0;
> memcpy(stack->entries, entries, flex_array_size(stack, entries, size));
> pool_offset += required_size;
> + /*
> + * Let KMSAN know the stored stack record is initialized. This shall
> + * prevent false positive reports if instrumented code accesses it.
> + */
> + kmsan_unpoison_memory(stack, required_size);
>
> return stack;
> }
> @@ -465,6 +471,12 @@ unsigned int stack_depot_fetch(depot_stack_handle_t handle,
> struct stack_record *stack;
>
> *entries = NULL;
> + /*
> + * Let KMSAN know *entries is initialized. This shall prevent false
> + * positive reports if instrumented code accesses it.
> + */
> + kmsan_unpoison_memory(entries, sizeof(*entries));
> +
> if (!handle)
> return 0;
>
> --
> 2.40.0.rc0.216.gc4246ad0f0-goog
>
Reviewed-by: Andrey Konovalov <andreyknvl@gmail.com>
^ permalink raw reply [flat|nested] 4+ messages in thread