Received-SPF: pass (google.com: domain of linux-kernel+bounces-37410-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <20240124164211.1141742-1-glider@google.com>
In-Reply-To: <20240124164211.1141742-1-glider@google.com>
From: Marco Elver <elver@google.com>
Date: Wed, 24 Jan 2024 18:15:04 +0100
Message-ID: <CANpmjNP-9hV_d3zEHhUSpdUYpM1BAFKmTTzWwe5o5ubtwTvQAQ@mail.gmail.com>
Subject: Re: [PATCH] mm: kmsan: remove runtime checks from kmsan_unpoison_memory()
To: Alexander Potapenko <glider@google.com>
Cc: akpm@linux-foundation.org, linux-kernel@vger.kernel.org, 
	linux-mm@kvack.org, kasan-dev@googlegroups.com, 
	Dmitry Vyukov <dvyukov@google.com>, Ilya Leoshkevich <iii@linux.ibm.com>, 
	Nicholas Miehlbradt <nicholas@linux.ibm.com>
Content-Type: text/plain; charset="UTF-8"

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));