Similarly to what's been done in commit ff444efbbb9be ("kmsan: allow
using __msan_instrument_asm_store() inside runtime"), it should be safe
to call kmsan_unpoison_memory() from within the runtime, as it does not
allocate memory or take locks. Remove the redundant runtime checks.
This should fix false positives seen with CONFIG_DEBUG_LIST=y when
the non-instrumented lib/stackdepot.c failed to unpoison the memory
chunks later checked by the instrumented lib/list_debug.c
Also replace the implementation of kmsan_unpoison_entry_regs() with
a call to kmsan_unpoison_memory().
Signed-off-by: Alexander Potapenko <[email protected]>
Cc: Marco Elver <[email protected]>
Cc: Dmitry Vyukov <[email protected]>
Cc: Ilya Leoshkevich <[email protected]>
Cc: Nicholas Miehlbradt <[email protected]>
---
mm/kmsan/hooks.c | 36 +++++++++++++-----------------------
1 file changed, 13 insertions(+), 23 deletions(-)
diff --git a/mm/kmsan/hooks.c b/mm/kmsan/hooks.c
index 5d6e2dee5692a..8a990cbf6d670 100644
--- a/mm/kmsan/hooks.c
+++ b/mm/kmsan/hooks.c
@@ -359,6 +359,12 @@ void kmsan_handle_dma_sg(struct scatterlist *sg, int nents,
}
/* Functions from kmsan-checks.h follow. */
+
+/*
+ * To create an origin, kmsan_poison_memory() unwinds the stacks and stores it
+ * into the stack depot. This may cause deadlocks if done from within KMSAN
+ * runtime, therefore we bail out if kmsan_in_runtime().
+ */
void kmsan_poison_memory(const void *address, size_t size, gfp_t flags)
{
if (!kmsan_enabled || kmsan_in_runtime())
@@ -371,47 +377,31 @@ void kmsan_poison_memory(const void *address, size_t size, gfp_t flags)
}
EXPORT_SYMBOL(kmsan_poison_memory);
+/*
+ * Unlike kmsan_poison_memory(), this function can be used from within KMSAN
+ * runtime, because it does not trigger allocations or call instrumented code.
+ */
void kmsan_unpoison_memory(const void *address, size_t size)
{
unsigned long ua_flags;
- if (!kmsan_enabled || kmsan_in_runtime())
+ if (!kmsan_enabled)
return;
ua_flags = user_access_save();
- kmsan_enter_runtime();
/* The users may want to poison/unpoison random memory. */
kmsan_internal_unpoison_memory((void *)address, size,
KMSAN_POISON_NOCHECK);
- kmsan_leave_runtime();
user_access_restore(ua_flags);
}
EXPORT_SYMBOL(kmsan_unpoison_memory);
/*
- * Version of kmsan_unpoison_memory() that can be called from within the KMSAN
- * runtime.
- *
- * Non-instrumented IRQ entry functions receive struct pt_regs from assembly
- * code. Those regs need to be unpoisoned, otherwise using them will result in
- * false positives.
- * Using kmsan_unpoison_memory() is not an option in entry code, because the
- * return value of in_task() is inconsistent - as a result, certain calls to
- * kmsan_unpoison_memory() are ignored. kmsan_unpoison_entry_regs() ensures that
- * the registers are unpoisoned even if kmsan_in_runtime() is true in the early
- * entry code.
+ * Version of kmsan_unpoison_memory() called from IRQ entry functions.
*/
void kmsan_unpoison_entry_regs(const struct pt_regs *regs)
{
- unsigned long ua_flags;
-
- if (!kmsan_enabled)
- return;
-
- ua_flags = user_access_save();
- kmsan_internal_unpoison_memory((void *)regs, sizeof(*regs),
- KMSAN_POISON_NOCHECK);
- user_access_restore(ua_flags);
+ kmsan_unpoison_memory((void *)regs, sizeof(*regs);
}
void kmsan_check_memory(const void *addr, size_t size)
--
2.43.0.429.g432eaa2c6b-goog
On Wed, 24 Jan 2024 at 17:42, 'Alexander Potapenko' via kasan-dev
<[email protected]> wrote:
>
> Similarly to what's been done in commit ff444efbbb9be ("kmsan: allow
> using __msan_instrument_asm_store() inside runtime"), it should be safe
> to call kmsan_unpoison_memory() from within the runtime, as it does not
> allocate memory or take locks. Remove the redundant runtime checks.
>
> This should fix false positives seen with CONFIG_DEBUG_LIST=y when
> the non-instrumented lib/stackdepot.c failed to unpoison the memory
> chunks later checked by the instrumented lib/list_debug.c
>
> Also replace the implementation of kmsan_unpoison_entry_regs() with
> a call to kmsan_unpoison_memory().
>
> Signed-off-by: Alexander Potapenko <[email protected]>
> Cc: Marco Elver <[email protected]>
> Cc: Dmitry Vyukov <[email protected]>
> Cc: Ilya Leoshkevich <[email protected]>
> Cc: Nicholas Miehlbradt <[email protected]>
Tested-by: Marco Elver <[email protected]>
Nice - this fixes the false positives I've seen in testing the new
stack depot changes.
But I think this version of the patch wasn't compile-tested, see below.
> ---
> mm/kmsan/hooks.c | 36 +++++++++++++-----------------------
> 1 file changed, 13 insertions(+), 23 deletions(-)
>
> diff --git a/mm/kmsan/hooks.c b/mm/kmsan/hooks.c
> index 5d6e2dee5692a..8a990cbf6d670 100644
> --- a/mm/kmsan/hooks.c
> +++ b/mm/kmsan/hooks.c
> @@ -359,6 +359,12 @@ void kmsan_handle_dma_sg(struct scatterlist *sg, int nents,
> }
>
> /* Functions from kmsan-checks.h follow. */
> +
> +/*
> + * To create an origin, kmsan_poison_memory() unwinds the stacks and stores it
> + * into the stack depot. This may cause deadlocks if done from within KMSAN
> + * runtime, therefore we bail out if kmsan_in_runtime().
> + */
> void kmsan_poison_memory(const void *address, size_t size, gfp_t flags)
> {
> if (!kmsan_enabled || kmsan_in_runtime())
> @@ -371,47 +377,31 @@ void kmsan_poison_memory(const void *address, size_t size, gfp_t flags)
> }
> EXPORT_SYMBOL(kmsan_poison_memory);
>
> +/*
> + * Unlike kmsan_poison_memory(), this function can be used from within KMSAN
> + * runtime, because it does not trigger allocations or call instrumented code.
> + */
> void kmsan_unpoison_memory(const void *address, size_t size)
> {
> unsigned long ua_flags;
>
> - if (!kmsan_enabled || kmsan_in_runtime())
> + if (!kmsan_enabled)
> return;
>
> ua_flags = user_access_save();
> - kmsan_enter_runtime();
> /* The users may want to poison/unpoison random memory. */
> kmsan_internal_unpoison_memory((void *)address, size,
> KMSAN_POISON_NOCHECK);
> - kmsan_leave_runtime();
> user_access_restore(ua_flags);
> }
> EXPORT_SYMBOL(kmsan_unpoison_memory);
>
> /*
> - * Version of kmsan_unpoison_memory() that can be called from within the KMSAN
> - * runtime.
> - *
> - * Non-instrumented IRQ entry functions receive struct pt_regs from assembly
> - * code. Those regs need to be unpoisoned, otherwise using them will result in
> - * false positives.
> - * Using kmsan_unpoison_memory() is not an option in entry code, because the
> - * return value of in_task() is inconsistent - as a result, certain calls to
> - * kmsan_unpoison_memory() are ignored. kmsan_unpoison_entry_regs() ensures that
> - * the registers are unpoisoned even if kmsan_in_runtime() is true in the early
> - * entry code.
> + * Version of kmsan_unpoison_memory() called from IRQ entry functions.
> */
> void kmsan_unpoison_entry_regs(const struct pt_regs *regs)
> {
> - unsigned long ua_flags;
> -
> - if (!kmsan_enabled)
> - return;
> -
> - ua_flags = user_access_save();
> - kmsan_internal_unpoison_memory((void *)regs, sizeof(*regs),
> - KMSAN_POISON_NOCHECK);
> - user_access_restore(ua_flags);
> + kmsan_unpoison_memory((void *)regs, sizeof(*regs);
missing ')', probably:
+ kmsan_unpoison_memory((void *)regs, sizeof(*regs));
On Wed, Jan 24, 2024 at 6:15 PM Marco Elver <[email protected]> wrote:
>
> On Wed, 24 Jan 2024 at 17:42, 'Alexander Potapenko' via kasan-dev
> <[email protected]> wrote:
> >
> > Similarly to what's been done in commit ff444efbbb9be ("kmsan: allow
> > using __msan_instrument_asm_store() inside runtime"), it should be safe
> > to call kmsan_unpoison_memory() from within the runtime, as it does not
> > allocate memory or take locks. Remove the redundant runtime checks.
> >
> > This should fix false positives seen with CONFIG_DEBUG_LIST=y when
> > the non-instrumented lib/stackdepot.c failed to unpoison the memory
> > chunks later checked by the instrumented lib/list_debug.c
> >
> > Also replace the implementation of kmsan_unpoison_entry_regs() with
> > a call to kmsan_unpoison_memory().
> >
> > Signed-off-by: Alexander Potapenko <[email protected]>
> > Cc: Marco Elver <[email protected]>
> > Cc: Dmitry Vyukov <[email protected]>
> > Cc: Ilya Leoshkevich <[email protected]>
> > Cc: Nicholas Miehlbradt <[email protected]>
>
> Tested-by: Marco Elver <[email protected]>
>
> missing ')', probably:
>
> + kmsan_unpoison_memory((void *)regs, sizeof(*regs));
My bad - you are right. Thanks for catching!