diff mbox series

[v2] mm: kmsan: remove runtime checks from kmsan_unpoison_memory()

Message ID 20240124173134.1165747-1-glider@google.com
State New
Headers
Series [v2] mm: kmsan: remove runtime checks from kmsan_unpoison_memory() |

Commit Message

Alexander Potapenko Jan. 24, 2024, 5:31 p.m. UTC 
  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 <glider@google.com>
Tested-by: Marco Elver <elver@google.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Ilya Leoshkevich <iii@linux.ibm.com>
Cc: Nicholas Miehlbradt <nicholas@linux.ibm.com>
---
 mm/kmsan/hooks.c | 36 +++++++++++++-----------------------
 1 file changed, 13 insertions(+), 23 deletions(-)

Comments

Andrew Morton Jan. 26, 2024, 1:34 a.m. UTC | #1 
On Wed, 24 Jan 2024 18:31:34 +0100 Alexander Potapenko <glider@google.com> wrote:

> Similarly to what's been done in commit ff444efbbb9be ("kmsan: allow

I make that 85716a80c16d.

> 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().
> 

"false positives" sound unpleasant.  Should this fix be backported into
earlier kernels?  And can we identify a suitable Fixes: target?
Alexander Potapenko Jan. 26, 2024, 4:57 p.m. UTC | #2 
On Fri, Jan 26, 2024 at 2:34 AM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Wed, 24 Jan 2024 18:31:34 +0100 Alexander Potapenko <glider@google.com> wrote:
>
> > Similarly to what's been done in commit ff444efbbb9be ("kmsan: allow
>
> I make that 85716a80c16d.
>
> > 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().
> >
>
> "false positives" sound unpleasant.  Should this fix be backported into
> earlier kernels?  And can we identify a suitable Fixes: target?
>

Surprisingly, I haven't seen these false reports before, but the bug
has been there since KMSAN's early downstream days (at the time we
might have needed to have those checks).
So it should probably be:

Fixes: f80be4571b19b9 ("kmsan: add KMSAN runtime core")
diff mbox series

Patch

diff --git a/mm/kmsan/hooks.c b/mm/kmsan/hooks.c
index 5d6e2dee5692a..0b09daa188ef6 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)