[PATCH 1/3] x86: mm: disable KMSAN instrumentation for physaddr.c

[PATCH 1/3] x86: mm: disable KMSAN instrumentation for physaddr.c

[Alexander Potapenko]

Enabling CONFIG_DEBUG_VIRTUAL=y together with KMSAN led to infinite
recursion, because kmsan_get_metadata() ended up calling instrumented
__pfn_valid() from arch/x86/mm/physaddr.c.

Prevent it by disabling instrumentation of the whole file.

Reported-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Closes: https://github.com/google/kmsan/issues/95
Signed-off-by: Alexander Potapenko <glider@google.com>
---
 arch/x86/mm/Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/mm/Makefile b/arch/x86/mm/Makefile
index 8d3a00e5c528e..d3b27a383127d 100644
--- a/arch/x86/mm/Makefile
+++ b/arch/x86/mm/Makefile
@@ -17,6 +17,7 @@ KCSAN_SANITIZE := n
 # Avoid recursion by not calling KMSAN hooks for CEA code.
 KMSAN_SANITIZE_cpu_entry_area.o := n
 KMSAN_SANITIZE_mem_encrypt_identity.o := n
+KMSAN_SANITIZE_physaddr.o := n
 
 ifdef CONFIG_FUNCTION_TRACER
 CFLAGS_REMOVE_mem_encrypt.o		= -pg
-- 
2.45.2.741.gdbec12cfda-goog

[PATCH 2/3] lib/Kconfig.debug: disable LOCK_DEBUGGING_SUPPORT under KMSAN

[Alexander Potapenko]

At least on x86 KMSAN is seriously slown down by lockdep, as every
pfn_valid() call (which is done on every instrumented memory access
in the kernel) performs several lockdep checks, all of which, in turn,
perform additional memory accesses and call KMSAN instrumentation.

Right now lockdep overflows the stack under KMSAN, but even if we use
reentrancy counters to avoid the recursion on the KMSAN side, the slowdown
from lockdep remains big enough for the kernel to become unusable.

Reported-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Closes: https://github.com/google/kmsan/issues/94
Link: https://groups.google.com/g/kasan-dev/c/ZBiGzZL36-I/m/WtNuKqP9EQAJ
Signed-off-by: Alexander Potapenko <glider@google.com>
---
 lib/Kconfig.debug | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 59b6765d86b8f..036905cf1dbe9 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1339,7 +1339,7 @@ menu "Lock Debugging (spinlocks, mutexes, etc...)"
 
 config LOCK_DEBUGGING_SUPPORT
 	bool
-	depends on TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT && LOCKDEP_SUPPORT
+	depends on TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT && LOCKDEP_SUPPORT && !KMSAN
 	default y
 
 config PROVE_LOCKING
-- 
2.45.2.741.gdbec12cfda-goog

Re: [PATCH 2/3] lib/Kconfig.debug: disable LOCK_DEBUGGING_SUPPORT under KMSAN

[Kirill A . Shutemov]

On Fri, Jun 21, 2024 at 11:49:00AM +0200, Alexander Potapenko wrote:
> At least on x86 KMSAN is seriously slown down by lockdep, as every
> pfn_valid() call (which is done on every instrumented memory access
> in the kernel) performs several lockdep checks, all of which, in turn,
> perform additional memory accesses and call KMSAN instrumentation.
> 
> Right now lockdep overflows the stack under KMSAN, but even if we use
> reentrancy counters to avoid the recursion on the KMSAN side, the slowdown
> from lockdep remains big enough for the kernel to become unusable.
> 
> Reported-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Closes: https://github.com/google/kmsan/issues/94
> Link: https://groups.google.com/g/kasan-dev/c/ZBiGzZL36-I/m/WtNuKqP9EQAJ
> Signed-off-by: Alexander Potapenko <glider@google.com>
> ---
>  lib/Kconfig.debug | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 59b6765d86b8f..036905cf1dbe9 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -1339,7 +1339,7 @@ menu "Lock Debugging (spinlocks, mutexes, etc...)"
>  
>  config LOCK_DEBUGGING_SUPPORT
>  	bool
> -	depends on TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT && LOCKDEP_SUPPORT
> +	depends on TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT && LOCKDEP_SUPPORT && !KMSAN

Nit: no need to pile everything in one line. Add "depends on !KMSAN" on a
separate line.

Otherwise, looks sane.

Reviewed-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCH 2/3] lib/Kconfig.debug: disable LOCK_DEBUGGING_SUPPORT under KMSAN

[Dave Hansen]

On 6/21/24 02:49, Alexander Potapenko wrote:
>  config LOCK_DEBUGGING_SUPPORT
>  	bool
> -	depends on TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT && LOCKDEP_SUPPORT
> +	depends on TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT && LOCKDEP_SUPPORT && !KMSAN
>  	default y

This kinda stinks.  It ends up doubling the amount of work that ks

Re: [PATCH 2/3] lib/Kconfig.debug: disable LOCK_DEBUGGING_SUPPORT under KMSAN

[Dave Hansen]

On 6/21/24 02:49, Alexander Potapenko wrote:
>  config LOCK_DEBUGGING_SUPPORT
>  	bool
> -	depends on TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT && LOCKDEP_SUPPORT
> +	depends on TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT && LOCKDEP_SUPPORT && !KMSAN
>  	default y

This kinda stinks.  Practically, it'll mean that anyone turning on KMSAN
will accidentally turn off lockdep.  That's really nasty, especially for
folks who are turning on debug options left and right to track down
nasty bugs.

I'd *MUCH* rather hide KMSAN:

config KMSAN
        bool "KMSAN: detector of uninitialized values use"
        depends on HAVE_ARCH_KMSAN && HAVE_KMSAN_COMPILER
        depends on DEBUG_KERNEL && !KASAN && !KCSAN
        depends on !PREEMPT_RT
+	depends on !LOCKDEP

Because, frankly, lockdep is way more important than KMSAN.

But ideally, we'd allow them to coexist somehow.  Have we even discussed
the problem with the lockdep folks?  For instance, I'd much rather have
a relaxed lockdep with no checking in pfn_valid() than no lockdep at all.

Re: [PATCH 2/3] lib/Kconfig.debug: disable LOCK_DEBUGGING_SUPPORT under KMSAN

[Boqun Feng]

On Fri, Jun 21, 2024 at 09:23:25AM -0700, Dave Hansen wrote:
> On 6/21/24 02:49, Alexander Potapenko wrote:
> >  config LOCK_DEBUGGING_SUPPORT
> >  	bool
> > -	depends on TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT && LOCKDEP_SUPPORT
> > +	depends on TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT && LOCKDEP_SUPPORT && !KMSAN
> >  	default y
> 
> This kinda stinks.  Practically, it'll mean that anyone turning on KMSAN
> will accidentally turn off lockdep.  That's really nasty, especially for
> folks who are turning on debug options left and right to track down
> nasty bugs.
> 
> I'd *MUCH* rather hide KMSAN:
> 
> config KMSAN
>         bool "KMSAN: detector of uninitialized values use"
>         depends on HAVE_ARCH_KMSAN && HAVE_KMSAN_COMPILER
>         depends on DEBUG_KERNEL && !KASAN && !KCSAN
>         depends on !PREEMPT_RT
> +	depends on !LOCKDEP
> 
> Because, frankly, lockdep is way more important than KMSAN.
> 
> But ideally, we'd allow them to coexist somehow.  Have we even discussed
> the problem with the lockdep folks?  For instance, I'd much rather have
> a relaxed lockdep with no checking in pfn_valid() than no lockdep at all.

The only locks used in pfn_valid() are rcu_read_lock_sched(), right? If
so, could you try (don't tell Paul ;-)) replace rcu_read_lock_sched()
with preempt_disable() and rcu_read_unlock_sched() with
preempt_enable()? That would avoid calling into lockdep. If that works
for KMSAN, we can either have a special rcu_read_lock_sched() or call
lockdep_recursion_inc() in instrumented pfn_valid() to disable lockdep
temporarily.

[Cc Paul]

Regards,
Boqun

Re: [PATCH 2/3] lib/Kconfig.debug: disable LOCK_DEBUGGING_SUPPORT under KMSAN

[Paul E. McKenney]

On Tue, Jun 25, 2024 at 11:51:23AM -0700, Boqun Feng wrote:
> On Fri, Jun 21, 2024 at 09:23:25AM -0700, Dave Hansen wrote:
> > On 6/21/24 02:49, Alexander Potapenko wrote:
> > >  config LOCK_DEBUGGING_SUPPORT
> > >  	bool
> > > -	depends on TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT && LOCKDEP_SUPPORT
> > > +	depends on TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT && LOCKDEP_SUPPORT && !KMSAN
> > >  	default y
> > 
> > This kinda stinks.  Practically, it'll mean that anyone turning on KMSAN
> > will accidentally turn off lockdep.  That's really nasty, especially for
> > folks who are turning on debug options left and right to track down
> > nasty bugs.
> > 
> > I'd *MUCH* rather hide KMSAN:
> > 
> > config KMSAN
> >         bool "KMSAN: detector of uninitialized values use"
> >         depends on HAVE_ARCH_KMSAN && HAVE_KMSAN_COMPILER
> >         depends on DEBUG_KERNEL && !KASAN && !KCSAN
> >         depends on !PREEMPT_RT
> > +	depends on !LOCKDEP
> > 
> > Because, frankly, lockdep is way more important than KMSAN.
> > 
> > But ideally, we'd allow them to coexist somehow.  Have we even discussed
> > the problem with the lockdep folks?  For instance, I'd much rather have
> > a relaxed lockdep with no checking in pfn_valid() than no lockdep at all.
> 
> The only locks used in pfn_valid() are rcu_read_lock_sched(), right? If
> so, could you try (don't tell Paul ;-)) replace rcu_read_lock_sched()
> with preempt_disable() and rcu_read_unlock_sched() with
> preempt_enable()? That would avoid calling into lockdep. If that works
> for KMSAN, we can either have a special rcu_read_lock_sched() or call
> lockdep_recursion_inc() in instrumented pfn_valid() to disable lockdep
> temporarily.
> 
> [Cc Paul]

Don't tell me what?  ;-)

An alternative is to use rcu_read_lock_sched_notrace() and
rcu_read_unlock_sched_notrace().  If you really want to use
preempt_disable() and preempt_enable() instead, you will likely want
the _notrace() variants.

							Thanx, Paul

Re: [PATCH 2/3] lib/Kconfig.debug: disable LOCK_DEBUGGING_SUPPORT under KMSAN

[Boqun Feng]

On Tue, Jun 25, 2024 at 12:06:52PM -0700, Paul E. McKenney wrote:
> On Tue, Jun 25, 2024 at 11:51:23AM -0700, Boqun Feng wrote:
> > On Fri, Jun 21, 2024 at 09:23:25AM -0700, Dave Hansen wrote:
> > > On 6/21/24 02:49, Alexander Potapenko wrote:
> > > >  config LOCK_DEBUGGING_SUPPORT
> > > >  	bool
> > > > -	depends on TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT && LOCKDEP_SUPPORT
> > > > +	depends on TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT && LOCKDEP_SUPPORT && !KMSAN
> > > >  	default y
> > > 
> > > This kinda stinks.  Practically, it'll mean that anyone turning on KMSAN
> > > will accidentally turn off lockdep.  That's really nasty, especially for
> > > folks who are turning on debug options left and right to track down
> > > nasty bugs.
> > > 
> > > I'd *MUCH* rather hide KMSAN:
> > > 
> > > config KMSAN
> > >         bool "KMSAN: detector of uninitialized values use"
> > >         depends on HAVE_ARCH_KMSAN && HAVE_KMSAN_COMPILER
> > >         depends on DEBUG_KERNEL && !KASAN && !KCSAN
> > >         depends on !PREEMPT_RT
> > > +	depends on !LOCKDEP
> > > 
> > > Because, frankly, lockdep is way more important than KMSAN.
> > > 
> > > But ideally, we'd allow them to coexist somehow.  Have we even discussed
> > > the problem with the lockdep folks?  For instance, I'd much rather have
> > > a relaxed lockdep with no checking in pfn_valid() than no lockdep at all.
> > 
> > The only locks used in pfn_valid() are rcu_read_lock_sched(), right? If
> > so, could you try (don't tell Paul ;-)) replace rcu_read_lock_sched()
> > with preempt_disable() and rcu_read_unlock_sched() with
> > preempt_enable()? That would avoid calling into lockdep. If that works
> > for KMSAN, we can either have a special rcu_read_lock_sched() or call
> > lockdep_recursion_inc() in instrumented pfn_valid() to disable lockdep
> > temporarily.
> > 
> > [Cc Paul]
> 
> Don't tell me what?  ;-)
> 

Turn out that telling you is a good idea ;-)

> An alternative is to use rcu_read_lock_sched_notrace() and
> rcu_read_unlock_sched_notrace().  If you really want to use

Yes, I think this is better than what I proposed.

Regards,
Boqun

> preempt_disable() and preempt_enable() instead, you will likely want
> the _notrace() variants.
> 
> 							Thanx, Paul

Re: [PATCH 2/3] lib/Kconfig.debug: disable LOCK_DEBUGGING_SUPPORT under KMSAN

[Alexander Potapenko]

On Tue, Jun 25, 2024 at 9:38 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> On Tue, Jun 25, 2024 at 12:06:52PM -0700, Paul E. McKenney wrote:
> > On Tue, Jun 25, 2024 at 11:51:23AM -0700, Boqun Feng wrote:
> > > On Fri, Jun 21, 2024 at 09:23:25AM -0700, Dave Hansen wrote:
> > > > On 6/21/24 02:49, Alexander Potapenko wrote:
> > > > >  config LOCK_DEBUGGING_SUPPORT
> > > > >         bool
> > > > > -       depends on TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT && LOCKDEP_SUPPORT
> > > > > +       depends on TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT && LOCKDEP_SUPPORT && !KMSAN
> > > > >         default y
> > > >
> > > > This kinda stinks.  Practically, it'll mean that anyone turning on KMSAN
> > > > will accidentally turn off lockdep.  That's really nasty, especially for
> > > > folks who are turning on debug options left and right to track down
> > > > nasty bugs.
> > > >
> > > > I'd *MUCH* rather hide KMSAN:
> > > >
> > > > config KMSAN
> > > >         bool "KMSAN: detector of uninitialized values use"
> > > >         depends on HAVE_ARCH_KMSAN && HAVE_KMSAN_COMPILER
> > > >         depends on DEBUG_KERNEL && !KASAN && !KCSAN
> > > >         depends on !PREEMPT_RT
> > > > + depends on !LOCKDEP
> > > >
> > > > Because, frankly, lockdep is way more important than KMSAN.
> > > >
> > > > But ideally, we'd allow them to coexist somehow.  Have we even discussed
> > > > the problem with the lockdep folks?  For instance, I'd much rather have
> > > > a relaxed lockdep with no checking in pfn_valid() than no lockdep at all.
> > >
> > > The only locks used in pfn_valid() are rcu_read_lock_sched(), right? If
> > > so, could you try (don't tell Paul ;-)) replace rcu_read_lock_sched()
> > > with preempt_disable() and rcu_read_unlock_sched() with
> > > preempt_enable()? That would avoid calling into lockdep. If that works
> > > for KMSAN, we can either have a special rcu_read_lock_sched() or call
> > > lockdep_recursion_inc() in instrumented pfn_valid() to disable lockdep
> > > temporarily.
> > >
> > > [Cc Paul]
> >
> > Don't tell me what?  ;-)
> >
>
> Turn out that telling you is a good idea ;-)
>
> > An alternative is to use rcu_read_lock_sched_notrace() and
> > rcu_read_unlock_sched_notrace().  If you really want to use
>
> Yes, I think this is better than what I proposed.

Thanks for your comments!
Yes, that's what I was actually looking into after Dave's answer on
the other thread
(https://groups.google.com/g/kasan-dev/c/ZBiGzZL36-I/m/WtNuKqP9EQAJ)
I'll still need to rework the code calling virt_to_page() to avoid
deadlocks from there though.

[PATCH 3/3] x86/traps: fix an objtool warning in handle_bug()

[Alexander Potapenko]

Because handle_bug() is a noinstr function, call to
kmsan_unpoison_entry_regs() should be happening within the
instrumentation_begin()/instrumentation_end() region.

Fortunately, the same noinstr annotation lets us dereference @regs
in handle_bug() without unpoisoning them, so we don't have to move the
`is_valid_bugaddr(regs->ip)` check below instrumentation_begin().

Reported-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Link: https://groups.google.com/g/kasan-dev/c/ZBiGzZL36-I/m/WtNuKqP9EQAJ
Signed-off-by: Alexander Potapenko <glider@google.com>
---
 arch/x86/kernel/traps.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 4fa0b17e5043a..e8f330d9ba5d4 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -217,12 +217,6 @@ static noinstr bool handle_bug(struct pt_regs *regs)
 {
 	bool handled = false;
 
-	/*
-	 * Normally @regs are unpoisoned by irqentry_enter(), but handle_bug()
-	 * is a rare case that uses @regs without passing them to
-	 * irqentry_enter().
-	 */
-	kmsan_unpoison_entry_regs(regs);
 	if (!is_valid_bugaddr(regs->ip))
 		return handled;
 
@@ -230,6 +224,15 @@ static noinstr bool handle_bug(struct pt_regs *regs)
 	 * All lies, just get the WARN/BUG out.
 	 */
 	instrumentation_begin();
+	/*
+	 * Normally @regs are unpoisoned by irqentry_enter(), but handle_bug()
+	 * is a rare case that uses @regs without passing them to
+	 * irqentry_enter().
+	 * Unpoisoning of @regs should be done before the first access to it,
+	 * but because this is a noinstr function it is fine to postpone
+	 * unpoisoning until the call of instrumentation_begin().
+	 */
+	kmsan_unpoison_entry_regs(regs);
 	/*
 	 * Since we're emulating a CALL with exceptions, restore the interrupt
 	 * state to what it was at the exception site.
-- 
2.45.2.741.gdbec12cfda-goog

Re: [PATCH 3/3] x86/traps: fix an objtool warning in handle_bug()

[Kirill A . Shutemov]

On Fri, Jun 21, 2024 at 11:49:01AM +0200, Alexander Potapenko wrote:
> Because handle_bug() is a noinstr function, call to
> kmsan_unpoison_entry_regs() should be happening within the
> instrumentation_begin()/instrumentation_end() region.
> 
> Fortunately, the same noinstr annotation lets us dereference @regs
> in handle_bug() without unpoisoning them, so we don't have to move the
> `is_valid_bugaddr(regs->ip)` check below instrumentation_begin().

Imperative mood, please. And capitalize "fix" in the subject.

https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/tree/Documentation/process/maintainer-tip.rst#n134
> 
> Reported-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Link: https://groups.google.com/g/kasan-dev/c/ZBiGzZL36-I/m/WtNuKqP9EQAJ
> Signed-off-by: Alexander Potapenko <glider@google.com>

Otherwise, looks good.

Reviewed-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCH 1/3] x86: mm: disable KMSAN instrumentation for physaddr.c

[Kirill A . Shutemov]

On Fri, Jun 21, 2024 at 11:48:59AM +0200, Alexander Potapenko wrote:
> Enabling CONFIG_DEBUG_VIRTUAL=y together with KMSAN led to infinite
> recursion, because kmsan_get_metadata() ended up calling instrumented
> __pfn_valid() from arch/x86/mm/physaddr.c.
> 
> Prevent it by disabling instrumentation of the whole file.
> 
> Reported-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Closes: https://github.com/google/kmsan/issues/95
> Signed-off-by: Alexander Potapenko <glider@google.com>

Reviewed-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCH 1/3] x86: mm: disable KMSAN instrumentation for physaddr.c

[Dave Hansen]

On 6/21/24 02:48, Alexander Potapenko wrote:
> Enabling CONFIG_DEBUG_VIRTUAL=y together with KMSAN led to infinite
> recursion, because kmsan_get_metadata() ended up calling instrumented
> __pfn_valid() from arch/x86/mm/physaddr.c.
> 
> Prevent it by disabling instrumentation of the whole file.

This does seem rather ad-hoc.  It's the same basic reason we have
"noinstr": code instrumentation infrastructure uses generally can't be
instrumented itself.

How hard would it be to make sure that kmsan_get_metadata() and friends
don't call any symbols that were compiled with -fsanitize=kernel-memory?

I do also think I'd much rather see __no_kmsan_checks on the functions
than doing whole files.  I *guarantee* if code gets moved around that
whoever does it will miss the KMSAN_SANITIZE_physaddr.o in the makefile.