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@xxxxxxxxx> 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.