Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 27 Oct 2022 11:50:07 -0700
From:   "Paul E. McKenney" <paulmck@kernel.org>
To:     Petr Mladek <pmladek@suse.com>
Cc:     John Ogness <john.ogness@linutronix.de>,
        Sergey Senozhatsky <senozhatsky@chromium.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        linux-kernel@vger.kernel.org,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        boqun.feng@gmail.com
Subject: Re: [PATCH printk v2 33/38] printk: introduce console_list_lock
Message-ID: <20221027185007.GG5600@paulmck-ThinkPad-P17-Gen-1>
Reply-To: paulmck@kernel.org
References: <20221019145600.1282823-1-john.ogness@linutronix.de>
 <20221019145600.1282823-34-john.ogness@linutronix.de>
 <Y1pY3I1ufABvroYj@alley>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y1pY3I1ufABvroYj@alley>
Precedence: bulk

On Thu, Oct 27, 2022 at 12:09:32PM +0200, Petr Mladek wrote:
> Adding Paul into Cc so that he is aware of using a custom SRCU lockdep
> check in console_list_lock().

[ . . . ]

> > +/**
> > + * console_list_lock - Lock the console list
> > + *
> > + * For console list or console->flags updates
> > + */
> > +void console_list_lock(void)
> > +{
> > +#ifdef CONFIG_DEBUG_LOCK_ALLOC
> > +	/*
> > +	 * In unregister_console(), synchronize_srcu() is called with the
> > +	 * console_list_lock held. Therefore it is not allowed that the
> > +	 * console_list_lock is taken with the srcu_lock held.
> > +	 *
> > +	 * Whether or not this context is in the read-side critical section
> > +	 * can only be detected if the appropriate debug options are enabled.
> > +	 */
> > +	WARN_ON_ONCE(debug_lockdep_rcu_enabled() &&
> > +		     srcu_read_lock_held(&console_srcu));

Yes, this is an interesting case.

The problem that John is facing is that srcu_read_lock_held() believes
that it is safer to err on the side of there being an SRCU reader.
This is because the standard use is to complain if there is -not-
an SRCU reader.  So as soon as there is a lockdep issue anywhere,
srcu_read_lock_held() switches to unconditionally returning true.

Which is exactly what John does not want in this case.

So he excludes the CONFIG_DEBUG_LOCK_ALLOC=n case and the
!debug_lockdep_rcu_enabled() case, both of which cause
srcu_read_lock_held() to unconditionally return true.

This can result in false-positive splats if some other CPU issues a
lockdep warning after debug_lockdep_rcu_enabled() is invoked but before
srcu_read_lock_held() is invoked.  But similar vulnerabilities are
present in RCU_LOCKDEP_WARN(), so unless and until there is a problem,
this code should suffice.

One way to save a line is as follows:

	WARN_ON_ONCE(IS_ENABLED(CONFIG_DEBUG_LOCK_ALLOC) &&
		     debug_lockdep_rcu_enabled() &&
		     srcu_read_lock_held(&console_srcu));

Longer term, it might be possible to teach lockdep about this sort of
SRCU deadlock.  This is not an issue for vanilla RCU because the RCU
reader context prohibits such deadlocks.  This isn't exactly the same
as reader-writer locking because this is perfectly OK with SRCU:

	CPU 0:
		spin_lock(&mylock);
		idx = srcu_read_lock(&mysrcu);
		do_something();
		srcu_read_unlock(&mysrcu, idx);
		spin_unlock(&mylock);

	CPU 1:
		idx = srcu_read_lock(&mysrcu);
		spin_lock(&mylock);
		do_something();
		spin_unlock(&mylock);
		srcu_read_unlock(&mysrcu, idx);

Adding Boqun on CC in case it is easier than I think.  ;-)

							Thanx, Paul