[printk,v2,33/38] printk: introduce console_list_lock
Commit Message
Currently there exist races in console_register(), where the types
of registered consoles are checked (without holding the console_lock)
and then after acquiring the console_lock, it is assumed that the list
has not changed. Also, some code that performs console_unregister()
make similar assumptions.
Introduce a console_list_lock to provide full synchronization for any
console list changes. The console_list_lock also provides
synchronization for updates to console->flags.
Note that one of the various responsibilities of the console_lock is
also intended to provide this synchronization. Later changes will
update call sites relying on the console_lock for this purpose. Once
all call sites have been updated, the console_lock will be relieved
of synchronizing console_list and console->flags updates.
Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
include/linux/console.h | 20 ++++++++--
kernel/printk/printk.c | 82 +++++++++++++++++++++++++++++++++++------
2 files changed, 88 insertions(+), 14 deletions(-)
Comments
On Wed, Oct 19, 2022 at 05:01:55PM +0206, John Ogness wrote:
> Currently there exist races in console_register(), where the types
> of registered consoles are checked (without holding the console_lock)
> and then after acquiring the console_lock, it is assumed that the list
> has not changed. Also, some code that performs console_unregister()
> make similar assumptions.
>
> Introduce a console_list_lock to provide full synchronization for any
> console list changes. The console_list_lock also provides
> synchronization for updates to console->flags.
>
> Note that one of the various responsibilities of the console_lock is
> also intended to provide this synchronization. Later changes will
> update call sites relying on the console_lock for this purpose. Once
> all call sites have been updated, the console_lock will be relieved
> of synchronizing console_list and console->flags updates.
>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
> include/linux/console.h | 20 ++++++++--
> kernel/printk/printk.c | 82 +++++++++++++++++++++++++++++++++++------
> 2 files changed, 88 insertions(+), 14 deletions(-)
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Adding Paul into Cc so that he is aware of using a custom SRCU lockdep
check in console_list_lock().
On Wed 2022-10-19 17:01:55, John Ogness wrote:
> Currently there exist races in console_register(), where the types
> of registered consoles are checked (without holding the console_lock)
> and then after acquiring the console_lock, it is assumed that the list
> has not changed. Also, some code that performs console_unregister()
> make similar assumptions.
This sounds like that the list lock is added just to fix the races.
People might wonder why it is not done using console_lock().
My understanding is that removing this responsibility from console_lock() is
the main motivation. It would deserve a comment, e.g.
<proposal>
A solution would be to synchronize this using console_lock(). But it
would require a complex analyze of all console drivers to make sure
that console_lock() is not taken in match() and setup() callbacks.
And splitting the responsibilities of console_lock() is actually
one big motivation here.
Instead, introduce a console_list_lock...
</proposal>
> Introduce a console_list_lock to provide full synchronization for any
> console list changes.
> The console_list_lock also provides synchronization for updates
> to console->flags.
This would deserve some explanation. The synchronization of the list
manipulation is obvious, especially when the lock is called
console_list_lock. But the motivation to use this lock
for console->flags is much more complicated.
I had some problems to create a reasonable mental model about it.
I did split the flags into groups:
1. Flags that are driver-specific and static. They do not need
any synchronization:
+ CON_BOOT
+ CON_ANYTIME
2. Flags that are modified only during console registration [*]:
+ CON_PRINTBUFFER
+ CON_CONSDEV
+ CON_BRL
+ CON_EXTENDED
3. Flags that might be modified by more operations, namely: console
registration, start, and stop [*].
+ CON_ENABLED
[*] It is actually more complicated. Some flags are modified also
outside console registration code. But we are not going to
synchronize these changes because they are not visible
to others via console_drivers list.
This explained why it made sense to synchronize console->flags using
console_list_lock. Also this explained why
console_start()/console_stop() were updated in this patch.
The following description would have helped me:
<proposal>
In addition, use console_list_lock also for synchronization of
console->flags updates. All flags are either static or modified
only during the console registration. There are only few
exceptions.
The only exception is CON_ENABLED that is modified also by
console_start()/console_stop(). We need to take console_list_lock()
here as well.
Another exception is when the flags are modified by the console
driver init code before the console gets registered. These will
be ignored because they are not visible to the rest of the system
via console_drivers list.
</proposal>
> Note that one of the various responsibilities of the console_lock is
> also intended to provide this synchronization. Later changes will
> update call sites relying on the console_lock for this purpose. Once
> all call sites have been updated, the console_lock will be relieved
> of synchronizing console_list and console->flags updates.
It seems that this patch actually updates all writers. It would be
nice to mention it to define the scope of this patch.
<proposal>
Note that one of the various responsibilities of the console_lock is
also intended to provide this synchronization. Only the callers that
modify the list or flags are updated here. Later changes will
update the readers Once all call sites have been updated,
the console_lock will be relieved of synchronizing console_list
and console->flags updates.
</proposal>
> diff --git a/include/linux/console.h b/include/linux/console.h
> index 60195cd086dc..bf1e8136424a 100644
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -159,8 +159,12 @@ struct console {
> };
>
> #ifdef CONFIG_LOCKDEP
> +extern void lockdep_assert_console_list_lock_held(void);
> extern bool console_srcu_read_lock_is_held(void);
> #else
> +static inline void lockdep_assert_console_list_lock_held(void)
> +{
> +}
> static inline bool console_srcu_read_lock_is_held(void)
> {
> return 1;
> @@ -170,6 +174,9 @@ static inline bool console_srcu_read_lock_is_held(void)
> extern int console_srcu_read_lock(void);
> extern void console_srcu_read_unlock(int cookie);
>
> +extern void console_list_lock(void) __acquires(console_mutex);
> +extern void console_list_unlock(void) __releases(console_mutex);
> +
> extern struct hlist_head console_list;
>
> /**
> @@ -206,10 +213,17 @@ static inline bool console_is_enabled(const struct console *con)
> hlist_for_each_entry_srcu(con, &console_list, node, \
> console_srcu_read_lock_is_held())
>
> -/*
> - * for_each_console() allows you to iterate on each console
> +/**
> + * for_each_console() - Iterator over registered consoles
> + * @con: struct console pointer used as loop cursor
> + *
> + * The console list and all struct console fields are immutable while
> + * iterating.
s/all struct console fields/console->flags/ ?
> + *
> + * Requires console_list_lock to be held.
> */
> -#define for_each_console(con) \
> +#define for_each_console(con) \
> + lockdep_assert_console_list_lock_held(); \
> hlist_for_each_entry(con, &console_list, node)
>
> extern int console_set_on_cmdline;
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 2faa6e3e2fb8..3615ee6d68fd 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -78,6 +78,9 @@ EXPORT_SYMBOL(ignore_console_lock_warning);
> int oops_in_progress;
> EXPORT_SYMBOL(oops_in_progress);
>
> +/* console_mutex protects console_list and console->flags updates. */
/*
* console_mutex protects console_list updates and console->flags updates.
* The flags are synchronized only for consoles that are registered,
* accessible via the list.
*/
> +static DEFINE_MUTEX(console_mutex);
> +
> /*
> * console_sem protects console_list and console->flags updates, and also
> * provides serialization for access to the entire console driver system.
> @@ -104,6 +107,11 @@ static struct lockdep_map console_lock_dep_map = {
> .name = "console_lock"
> };
>
> +void lockdep_assert_console_list_lock_held(void)
> +{
> + lockdep_assert_held(&console_mutex);
> +}
> +
> bool console_srcu_read_lock_is_held(void)
> {
> return srcu_read_lock_held(&console_srcu);
> @@ -225,6 +233,40 @@ int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write,
> }
> #endif /* CONFIG_PRINTK && CONFIG_SYSCTL */
>
> +/**
> + * 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));
> +#endif
> + mutex_lock(&console_mutex);
> +}
> +EXPORT_SYMBOL(console_list_lock);
> +
> +/**
> + * console_list_unlock - Unlock the console list
> + *
> + * Counterpart to console_list_lock()
> + */
> +void console_list_unlock(void)
> +{
> + mutex_unlock(&console_mutex);
> +}
> +EXPORT_SYMBOL(console_list_unlock);
> +
> /**
> * console_srcu_read_lock - Register a new reader for the
> * SRCU-protected console list
> @@ -3304,13 +3350,15 @@ void register_console(struct console *newcon)
>
> hlist_for_each_entry_safe(con, tmp, &console_list, node) {
> if (con->flags & CON_BOOT)
> - unregister_console(con);
> + unregister_console_locked(con);
> }
> }
> +unlock:
> + console_list_unlock();
> }
> EXPORT_SYMBOL(register_console);
>
> -int unregister_console(struct console *console)
We should make it clear that it must be locked by console_list_lock().
Many people would expect console_lock() ;-) I would add a comment
and assert.
/* Must be called under console_list_lock(). */
> +static int unregister_console_locked(struct console *console)
> {
assert_console_list_lock_held();
> int res;
>
With updated comments:
Reviewed-by: Petr Mladek <pmladek@suse.com>
Best Regards,
Petr
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
On Thu, Oct 27, 2022 at 11:50:07AM -0700, Paul E. McKenney wrote:
> 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. ;-)
>
First I think reader-writer deadlock detection won't treat this as a
deadlock, because srcu_read_lock() is a recursive read lock ;-) in other
words, lockdep knows they don't block each other.
I was actually considering to equip SRCU with reader-writer deadlock
detection when:
https://lore.kernel.org/lkml/20180412021233.ewncg5jjuzjw3x62@tardis/
The problem (for SRCU to use reader-writer deadlock detection) is
letting lockdep know synchronize_srcu() doesn't block srcu_read_lock(),
so looks like I owe you a new lockdep annotation primitive ;-)
Regards,
Boqun
> Thanx, Paul
On Fri, Oct 28, 2022 at 11:09:35AM -0700, Boqun Feng wrote:
> On Thu, Oct 27, 2022 at 11:50:07AM -0700, Paul E. McKenney wrote:
> > 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. ;-)
>
> First I think reader-writer deadlock detection won't treat this as a
> deadlock, because srcu_read_lock() is a recursive read lock ;-) in other
> words, lockdep knows they don't block each other.
Nice!
> I was actually considering to equip SRCU with reader-writer deadlock
> detection when:
>
> https://lore.kernel.org/lkml/20180412021233.ewncg5jjuzjw3x62@tardis/
>
> The problem (for SRCU to use reader-writer deadlock detection) is
> letting lockdep know synchronize_srcu() doesn't block srcu_read_lock(),
> so looks like I owe you a new lockdep annotation primitive ;-)
Even better! ;-)
Thanx, Paul
On 2022-10-27, "Paul E. McKenney" <paulmck@kernel.org> wrote:
> 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));
Unfortunately this suggestion does not work because
debug_lockdep_rcu_enabled() only exists if CONFIG_DEBUG_LOCK_ALLOC is
enabled. Would you be interested in having an empty implementation?
Then my check would not need to be concerned about
CONFIG_DEBUG_LOCK_ALLOC at all. It would become:
WARN_ON_ONCE(debug_lockdep_rcu_enabled() &&
srcu_read_lock_held(&console_srcu));
The patch below could be used to achieve that.
John
--------8<-------------
From 71d9e484d5128cd1e57e5bff5391d91789f444fa Mon Sep 17 00:00:00 2001
From: John Ogness <john.ogness@linutronix.de>
Date: Mon, 7 Nov 2022 11:06:40 +0106
Subject: [PATCH] rcu: implement lockdep_rcu_enabled for
!CONFIG_DEBUG_LOCK_ALLOC
Provide an implementation for debug_lockdep_rcu_enabled() when
CONFIG_DEBUG_LOCK_ALLOC is not enabled. This allows code to check
if rcu lockdep debugging is available without needing an extra
check if CONFIG_DEBUG_LOCK_ALLOC is enabled.
Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
include/linux/rcupdate.h | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 08605ce7379d..65178c40ab6f 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -340,6 +340,11 @@ static inline int rcu_read_lock_any_held(void)
return !preemptible();
}
+static inline int debug_lockdep_rcu_enabled(void)
+{
+ return 0;
+}
+
#endif /* #else #ifdef CONFIG_DEBUG_LOCK_ALLOC */
#ifdef CONFIG_PROVE_RCU
On Mon, Nov 07, 2022 at 11:16:56AM +0106, John Ogness wrote:
> On 2022-10-27, "Paul E. McKenney" <paulmck@kernel.org> wrote:
> > 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));
>
> Unfortunately this suggestion does not work because
> debug_lockdep_rcu_enabled() only exists if CONFIG_DEBUG_LOCK_ALLOC is
> enabled. Would you be interested in having an empty implementation?
> Then my check would not need to be concerned about
> CONFIG_DEBUG_LOCK_ALLOC at all. It would become:
>
> WARN_ON_ONCE(debug_lockdep_rcu_enabled() &&
> srcu_read_lock_held(&console_srcu));
>
> The patch below could be used to achieve that.
Looks quite sensible to me! There are probably a few other places
where it might be used.
Thanx, Paul
> John
>
> --------8<-------------
> >From 71d9e484d5128cd1e57e5bff5391d91789f444fa Mon Sep 17 00:00:00 2001
> From: John Ogness <john.ogness@linutronix.de>
> Date: Mon, 7 Nov 2022 11:06:40 +0106
> Subject: [PATCH] rcu: implement lockdep_rcu_enabled for
> !CONFIG_DEBUG_LOCK_ALLOC
>
> Provide an implementation for debug_lockdep_rcu_enabled() when
> CONFIG_DEBUG_LOCK_ALLOC is not enabled. This allows code to check
> if rcu lockdep debugging is available without needing an extra
> check if CONFIG_DEBUG_LOCK_ALLOC is enabled.
>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
> include/linux/rcupdate.h | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index 08605ce7379d..65178c40ab6f 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -340,6 +340,11 @@ static inline int rcu_read_lock_any_held(void)
> return !preemptible();
> }
>
> +static inline int debug_lockdep_rcu_enabled(void)
> +{
> + return 0;
> +}
> +
> #endif /* #else #ifdef CONFIG_DEBUG_LOCK_ALLOC */
>
> #ifdef CONFIG_PROVE_RCU
> --
> 2.30.2
@@ -159,8 +159,12 @@ struct console {
};
#ifdef CONFIG_LOCKDEP
+extern void lockdep_assert_console_list_lock_held(void);
extern bool console_srcu_read_lock_is_held(void);
#else
+static inline void lockdep_assert_console_list_lock_held(void)
+{
+}
static inline bool console_srcu_read_lock_is_held(void)
{
return 1;
@@ -170,6 +174,9 @@ static inline bool console_srcu_read_lock_is_held(void)
extern int console_srcu_read_lock(void);
extern void console_srcu_read_unlock(int cookie);
+extern void console_list_lock(void) __acquires(console_mutex);
+extern void console_list_unlock(void) __releases(console_mutex);
+
extern struct hlist_head console_list;
/**
@@ -206,10 +213,17 @@ static inline bool console_is_enabled(const struct console *con)
hlist_for_each_entry_srcu(con, &console_list, node, \
console_srcu_read_lock_is_held())
-/*
- * for_each_console() allows you to iterate on each console
+/**
+ * for_each_console() - Iterator over registered consoles
+ * @con: struct console pointer used as loop cursor
+ *
+ * The console list and all struct console fields are immutable while
+ * iterating.
+ *
+ * Requires console_list_lock to be held.
*/
-#define for_each_console(con) \
+#define for_each_console(con) \
+ lockdep_assert_console_list_lock_held(); \
hlist_for_each_entry(con, &console_list, node)
extern int console_set_on_cmdline;
@@ -78,6 +78,9 @@ EXPORT_SYMBOL(ignore_console_lock_warning);
int oops_in_progress;
EXPORT_SYMBOL(oops_in_progress);
+/* console_mutex protects console_list and console->flags updates. */
+static DEFINE_MUTEX(console_mutex);
+
/*
* console_sem protects console_list and console->flags updates, and also
* provides serialization for access to the entire console driver system.
@@ -104,6 +107,11 @@ static struct lockdep_map console_lock_dep_map = {
.name = "console_lock"
};
+void lockdep_assert_console_list_lock_held(void)
+{
+ lockdep_assert_held(&console_mutex);
+}
+
bool console_srcu_read_lock_is_held(void)
{
return srcu_read_lock_held(&console_srcu);
@@ -225,6 +233,40 @@ int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write,
}
#endif /* CONFIG_PRINTK && CONFIG_SYSCTL */
+/**
+ * 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));
+#endif
+ mutex_lock(&console_mutex);
+}
+EXPORT_SYMBOL(console_list_lock);
+
+/**
+ * console_list_unlock - Unlock the console list
+ *
+ * Counterpart to console_list_lock()
+ */
+void console_list_unlock(void)
+{
+ mutex_unlock(&console_mutex);
+}
+EXPORT_SYMBOL(console_list_unlock);
+
/**
* console_srcu_read_lock - Register a new reader for the
* SRCU-protected console list
@@ -3055,9 +3097,11 @@ struct tty_driver *console_device(int *index)
void console_stop(struct console *console)
{
__pr_flush(console, 1000, true);
+ console_list_lock();
console_lock();
console->flags &= ~CON_ENABLED;
console_unlock();
+ console_list_unlock();
/* Ensure that all SRCU list walks have completed */
synchronize_srcu(&console_srcu);
@@ -3066,9 +3110,11 @@ EXPORT_SYMBOL(console_stop);
void console_start(struct console *console)
{
+ console_list_lock();
console_lock();
console->flags |= CON_ENABLED;
console_unlock();
+ console_list_unlock();
__pr_flush(console, 1000, true);
}
EXPORT_SYMBOL(console_start);
@@ -3164,6 +3210,8 @@ static void try_enable_default_console(struct console *newcon)
#define console_first() \
hlist_entry(console_list.first, struct console, node)
+static int unregister_console_locked(struct console *console);
+
/*
* The console driver calls this routine during kernel initialization
* to register the console printing procedure with printk() and to
@@ -3188,15 +3236,14 @@ void register_console(struct console *newcon)
struct console *con;
bool bootcon_enabled = false;
bool realcon_enabled = false;
- int cookie;
int err;
- cookie = console_srcu_read_lock();
- for_each_console_srcu(con) {
+ console_list_lock();
+
+ for_each_console(con) {
if (WARN(con == newcon, "console '%s%d' already registered\n",
con->name, con->index)) {
- console_srcu_read_unlock(cookie);
- return;
+ goto unlock;
}
if (con->flags & CON_BOOT)
@@ -3204,13 +3251,12 @@ void register_console(struct console *newcon)
else
realcon_enabled = true;
}
- console_srcu_read_unlock(cookie);
/* Do not register boot consoles when there already is a real one. */
if (newcon->flags & CON_BOOT && realcon_enabled) {
pr_info("Too late to register bootconsole %s%d\n",
newcon->name, newcon->index);
- return;
+ goto unlock;
}
/*
@@ -3241,7 +3287,7 @@ void register_console(struct console *newcon)
/* printk() messages are not printed to the Braille console. */
if (err || newcon->flags & CON_BRL)
- return;
+ goto unlock;
/*
* If we have a bootconsole, and are switching to a real console,
@@ -3304,13 +3350,15 @@ void register_console(struct console *newcon)
hlist_for_each_entry_safe(con, tmp, &console_list, node) {
if (con->flags & CON_BOOT)
- unregister_console(con);
+ unregister_console_locked(con);
}
}
+unlock:
+ console_list_unlock();
}
EXPORT_SYMBOL(register_console);
-int unregister_console(struct console *console)
+static int unregister_console_locked(struct console *console)
{
int res;
@@ -3362,6 +3410,16 @@ int unregister_console(struct console *console)
console_unlock();
return res;
}
+
+int unregister_console(struct console *console)
+{
+ int res;
+
+ console_list_lock();
+ res = unregister_console_locked(console);
+ console_list_unlock();
+ return res;
+}
EXPORT_SYMBOL(unregister_console);
/*
@@ -3414,6 +3472,7 @@ static int __init printk_late_init(void)
struct console *con;
int ret;
+ console_list_lock();
hlist_for_each_entry_safe(con, tmp, &console_list, node) {
if (!(con->flags & CON_BOOT))
continue;
@@ -3431,9 +3490,10 @@ static int __init printk_late_init(void)
*/
pr_warn("bootconsole [%s%d] uses init memory and must be disabled even before the real one is ready\n",
con->name, con->index);
- unregister_console(con);
+ unregister_console_locked(con);
}
}
+ console_list_unlock();
ret = cpuhp_setup_state_nocalls(CPUHP_PRINTK_DEAD, "printk:dead", NULL,
console_cpu_notify);