| Message ID | 20240124164211.1141742-1-glider@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: kmsan: remove runtime checks from kmsan_unpoison_memory() | expand |
On Wed, 24 Jan 2024 at 17:42, 'Alexander Potapenko' via kasan-dev <kasan-dev@googlegroups.com> 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 <glider@google.com> > Cc: 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> Tested-by: Marco Elver <elver@google.com> 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 <elver@google.com> wrote: > > On Wed, 24 Jan 2024 at 17:42, 'Alexander Potapenko' via kasan-dev > <kasan-dev@googlegroups.com> 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 <glider@google.com> > > Cc: 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> > > Tested-by: Marco Elver <elver@google.com> > > missing ')', probably: > > + kmsan_unpoison_memory((void *)regs, sizeof(*regs)); My bad - you are right. Thanks for catching!
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)
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> Cc: 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(-)