[printk,v2,03/38] printk: Prepare for SRCU console list protection

  Provide an NMI-safe SRCU protected variant to walk the console list.

Note that all console fields are now set before adding the console
to the list to avoid the console becoming visible by SCRU readers
before being fully initialized.

This is a preparatory change for a new console infrastructure which
operates independent of the console BKL.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 .clang-format           |  1 +
 include/linux/console.h | 28 +++++++++++++++-
 kernel/printk/printk.c  | 72 ++++++++++++++++++++++++++++++++---------
 3 files changed, 85 insertions(+), 16 deletions(-)
  

On Wed, Oct 19, 2022 at 4:56 PM John Ogness <john.ogness@linutronix.de> wrote:
>
>  .clang-format           |  1 +

Acked-by: Miguel Ojeda <ojeda@kernel.org>

Cheers,
Miguel
  

On Wed, Oct 19, 2022 at 05:01:25PM +0206, John Ogness wrote:
> Provide an NMI-safe SRCU protected variant to walk the console list.
> 
> Note that all console fields are now set before adding the console
> to the list to avoid the console becoming visible by SCRU readers
> before being fully initialized.
> 
> This is a preparatory change for a new console infrastructure which
> operates independent of the console BKL.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

From an RCU viewpoint:

Reviewed-by: Paul E. McKenney <paulmck@kernel.org>

> ---
>  .clang-format           |  1 +
>  include/linux/console.h | 28 +++++++++++++++-
>  kernel/printk/printk.c  | 72 ++++++++++++++++++++++++++++++++---------
>  3 files changed, 85 insertions(+), 16 deletions(-)
> 
> diff --git a/.clang-format b/.clang-format
> index 1247d54f9e49..04a675b56b57 100644
> --- a/.clang-format
> +++ b/.clang-format
> @@ -222,6 +222,7 @@ ForEachMacros:
>    - 'for_each_component_dais'
>    - 'for_each_component_dais_safe'
>    - 'for_each_console'
> +  - 'for_each_console_srcu'
>    - 'for_each_cpu'
>    - 'for_each_cpu_and'
>    - 'for_each_cpu_not'
> diff --git a/include/linux/console.h b/include/linux/console.h
> index 7b5f21f9e469..cff86cc615f8 100644
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -15,7 +15,7 @@
>  #define _LINUX_CONSOLE_H_ 1
>  
>  #include <linux/atomic.h>
> -#include <linux/list.h>
> +#include <linux/rculist.h>
>  #include <linux/types.h>
>  
>  struct vc_data;
> @@ -158,8 +158,34 @@ struct console {
>  	struct hlist_node node;
>  };
>  
> +#ifdef CONFIG_LOCKDEP
> +extern bool console_srcu_read_lock_is_held(void);
> +#else
> +static inline bool console_srcu_read_lock_is_held(void)
> +{
> +	return 1;
> +}
> +#endif
> +
> +extern int console_srcu_read_lock(void);
> +extern void console_srcu_read_unlock(int cookie);
> +
>  extern struct hlist_head console_list;
>  
> +/**
> + * for_each_console_srcu() - Iterator over registered consoles
> + * @con:	struct console pointer used as loop cursor
> + *
> + * Although SRCU guarantees the console list will be consistent, the
> + * struct console fields may be updated by other CPUs while iterating.
> + *
> + * Requires console_srcu_read_lock to be held. Can be invoked from
> + * any context.
> + */
> +#define for_each_console_srcu(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
>   */
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 867becc40021..e8a56056cd50 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -85,6 +85,7 @@ EXPORT_SYMBOL(oops_in_progress);
>  static DEFINE_SEMAPHORE(console_sem);
>  HLIST_HEAD(console_list);
>  EXPORT_SYMBOL_GPL(console_list);
> +DEFINE_STATIC_SRCU(console_srcu);
>  
>  /*
>   * System may need to suppress printk message under certain
> @@ -102,6 +103,11 @@ static int __read_mostly suppress_panic_printk;
>  static struct lockdep_map console_lock_dep_map = {
>  	.name = "console_lock"
>  };
> +
> +bool console_srcu_read_lock_is_held(void)
> +{
> +	return srcu_read_lock_held(&console_srcu);
> +}
>  #endif
>  
>  enum devkmsg_log_bits {
> @@ -219,6 +225,32 @@ int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write,
>  }
>  #endif /* CONFIG_PRINTK && CONFIG_SYSCTL */
>  
> +/**
> + * console_srcu_read_lock - Register a new reader for the
> + *	SRCU-protected console list
> + *
> + * Use for_each_console_srcu() to iterate the console list
> + *
> + * Context: Any context.
> + */
> +int console_srcu_read_lock(void)
> +{
> +	return srcu_read_lock_nmisafe(&console_srcu);
> +}
> +EXPORT_SYMBOL(console_srcu_read_lock);
> +
> +/**
> + * console_srcu_read_unlock - Unregister an old reader from
> + *	the SRCU-protected console list
> + *
> + * Counterpart to console_srcu_read_lock()
> + */
> +void console_srcu_read_unlock(int cookie)
> +{
> +	srcu_read_unlock_nmisafe(&console_srcu, cookie);
> +}
> +EXPORT_SYMBOL(console_srcu_read_unlock);
> +
>  /*
>   * Helper macros to handle lockdep when locking/unlocking console_sem. We use
>   * macros instead of functions so that _RET_IP_ contains useful information.
> @@ -2989,6 +3021,9 @@ void console_stop(struct console *console)
>  	console_lock();
>  	console->flags &= ~CON_ENABLED;
>  	console_unlock();
> +
> +	/* Ensure that all SRCU list walks have completed */
> +	synchronize_srcu(&console_srcu);
>  }
>  EXPORT_SYMBOL(console_stop);
>  
> @@ -3179,6 +3214,17 @@ void register_console(struct console *newcon)
>  		newcon->flags &= ~CON_PRINTBUFFER;
>  	}
>  
> +	newcon->dropped = 0;
> +	if (newcon->flags & CON_PRINTBUFFER) {
> +		/* Get a consistent copy of @syslog_seq. */
> +		mutex_lock(&syslog_lock);
> +		newcon->seq = syslog_seq;
> +		mutex_unlock(&syslog_lock);
> +	} else {
> +		/* Begin with next message. */
> +		newcon->seq = prb_next_seq(prb);
> +	}
> +
>  	/*
>  	 * Put this console in the list - keep the
>  	 * preferred driver at the head of the list.
> @@ -3187,28 +3233,20 @@ void register_console(struct console *newcon)
>  	if (hlist_empty(&console_list)) {
>  		/* Ensure CON_CONSDEV is always set for the head. */
>  		newcon->flags |= CON_CONSDEV;
> -		hlist_add_head(&newcon->node, &console_list);
> +		hlist_add_head_rcu(&newcon->node, &console_list);
>  
>  	} else if (newcon->flags & CON_CONSDEV) {
>  		/* Only the new head can have CON_CONSDEV set. */
>  		console_first()->flags &= ~CON_CONSDEV;
> -		hlist_add_head(&newcon->node, &console_list);
> +		hlist_add_head_rcu(&newcon->node, &console_list);
>  
>  	} else {
> -		hlist_add_behind(&newcon->node, console_list.first);
> -	}
> -
> -	newcon->dropped = 0;
> -	if (newcon->flags & CON_PRINTBUFFER) {
> -		/* Get a consistent copy of @syslog_seq. */
> -		mutex_lock(&syslog_lock);
> -		newcon->seq = syslog_seq;
> -		mutex_unlock(&syslog_lock);
> -	} else {
> -		/* Begin with next message. */
> -		newcon->seq = prb_next_seq(prb);
> +		hlist_add_behind_rcu(&newcon->node, console_list.first);
>  	}
>  	console_unlock();
> +
> +	/* No need to synchronize SRCU here! */
> +
>  	console_sysfs_notify();
>  
>  	/*
> @@ -3254,7 +3292,7 @@ int unregister_console(struct console *console)
>  		goto out_unlock;
>  	}
>  
> -	hlist_del_init(&console->node);
> +	hlist_del_init_rcu(&console->node);
>  
>  	/*
>  	 * <HISTORICAL>
> @@ -3269,6 +3307,10 @@ int unregister_console(struct console *console)
>  		console_first()->flags |= CON_CONSDEV;
>  
>  	console_unlock();
> +
> +	/* Ensure that all SRCU list walks have completed */
> +	synchronize_srcu(&console_srcu);
> +
>  	console_sysfs_notify();
>  
>  	if (console->exit)
> -- 
> 2.30.2
>
  

On Wed 2022-10-19 17:01:25, John Ogness wrote:
> Provide an NMI-safe SRCU protected variant to walk the console list.
> 
> Note that all console fields are now set before adding the console
> to the list to avoid the console becoming visible by SCRU readers
> before being fully initialized.
> 
> This is a preparatory change for a new console infrastructure which
> operates independent of the console BKL.
> 
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -15,7 +15,7 @@
>  #define _LINUX_CONSOLE_H_ 1
>  
>  #include <linux/atomic.h>
> -#include <linux/list.h>
> +#include <linux/rculist.h>
>  #include <linux/types.h>
>  
>  struct vc_data;
> @@ -158,8 +158,34 @@ struct console {
>  	struct hlist_node node;
>  };
>  
> +#ifdef CONFIG_LOCKDEP

srcu_read_lock_held() actually depends on CONFIG_DEBUG_LOCK_ALLOC.
I suggest to avoid these problems and define:


extern struct srcu_struct console_srcu;

static inline bool console_srcu_read_lock_is_held(void)
{
	return srcu_read_lock_held(&console_srcu);
}

I guess that you wanted to keep console_srcu static to avoid
a misuse. It is true that we need to keep it NMI safe.

I do not have a strong opinion. I would personally risk it
and make it public. The CONFIG_* dependencies are hard to
maintain. We could add a comment that people need to use
the NMI safe API when manipulating the context.

> +extern bool console_srcu_read_lock_is_held(void);
> +#else
> +static inline bool console_srcu_read_lock_is_held(void)
> +{
> +	return 1;
> +}
> +#endif
> +
> +extern int console_srcu_read_lock(void);
> +extern void console_srcu_read_unlock(int cookie);
> +
>  extern struct hlist_head console_list;
>  
> +/**
> + * for_each_console_srcu() - Iterator over registered consoles
> + * @con:	struct console pointer used as loop cursor
> + *
> + * Although SRCU guarantees the console list will be consistent, the
> + * struct console fields may be updated by other CPUs while iterating.
> + *
> + * Requires console_srcu_read_lock to be held. Can be invoked from
> + * any context.
> + */
> +#define for_each_console_srcu(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
>   */
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 867becc40021..e8a56056cd50 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2989,6 +3021,9 @@ void console_stop(struct console *console)
>  	console_lock();
>  	console->flags &= ~CON_ENABLED;
>  	console_unlock();
> +
> +	/* Ensure that all SRCU list walks have completed */
> +	synchronize_srcu(&console_srcu);

The comment is not much helpful. It describes the behavior
that is more or less obvious. The important question
is why it is needed. And it is not clear to me.

What is the motivation for this synchronization, please?

This function disables one particular console. What exact walk are
we waiting for, please?

Is it really needed?

>  }
>  EXPORT_SYMBOL(console_stop);
>  
> @@ -3179,6 +3214,17 @@ void register_console(struct console *newcon)
>  		newcon->flags &= ~CON_PRINTBUFFER;
>  	}
>  
> +	newcon->dropped = 0;
> +	if (newcon->flags & CON_PRINTBUFFER) {
> +		/* Get a consistent copy of @syslog_seq. */
> +		mutex_lock(&syslog_lock);
> +		newcon->seq = syslog_seq;
> +		mutex_unlock(&syslog_lock);
> +	} else {
> +		/* Begin with next message. */
> +		newcon->seq = prb_next_seq(prb);

Hmm, prb_next_seq() is the next-to-be written message. It does not
guarantee that all the existing messages already reached the boot console.

This used to be synchronized by console_lock(). But we broke this
by the commit a699449bb13b70b8bd10d ("printk: refactor and rework
printing logic") that removed the global console_seq.

It actually kind of worked because console_unlock() was called before
the boot consoles were removed. So that the boot console
printed the pending messages before it was removed. Except when
the console lock was passed to another printk() caller.

Ideally, we should set it to con->seq from the related boot consoles.
But we do not know which one it is.

A good enough solution would be to set it to the minimal con->seq
of all registered boot consoles. They would most likely be on
the same sequence number. Anyway, con->seq has to be read under
console_lock() at least at this stage of the patchset.


> +	}
> +
>  	/*
>  	 * Put this console in the list - keep the
>  	 * preferred driver at the head of the list.
> @@ -3187,28 +3233,20 @@ void register_console(struct console *newcon)
>  	if (hlist_empty(&console_list)) {
>  		/* Ensure CON_CONSDEV is always set for the head. */
>  		newcon->flags |= CON_CONSDEV;
> -		hlist_add_head(&newcon->node, &console_list);
> +		hlist_add_head_rcu(&newcon->node, &console_list);
>  
>  	} else if (newcon->flags & CON_CONSDEV) {
>  		/* Only the new head can have CON_CONSDEV set. */
>  		console_first()->flags &= ~CON_CONSDEV;
> -		hlist_add_head(&newcon->node, &console_list);
> +		hlist_add_head_rcu(&newcon->node, &console_list);
>  
>  	} else {
> -		hlist_add_behind(&newcon->node, console_list.first);
> -	}
> -
> -	newcon->dropped = 0;
> -	if (newcon->flags & CON_PRINTBUFFER) {
> -		/* Get a consistent copy of @syslog_seq. */
> -		mutex_lock(&syslog_lock);
> -		newcon->seq = syslog_seq;
> -		mutex_unlock(&syslog_lock);
> -	} else {
> -		/* Begin with next message. */
> -		newcon->seq = prb_next_seq(prb);
> +		hlist_add_behind_rcu(&newcon->node, console_list.first);
>  	}
>  	console_unlock();
> +
> +	/* No need to synchronize SRCU here! */

This would deserve explanation why it is not needed. At least some
hint.

I think that it is not needed here. I can't think about any function
that would depend on it.

Anyway, I do not see any difference against console_stop(). I do not
understand why the synchronization is needed there and not here.

> +
>  	console_sysfs_notify();
>  
>  	/*
> @@ -3254,7 +3292,7 @@ int unregister_console(struct console *console)
>  		goto out_unlock;
>  	}
>  
> -	hlist_del_init(&console->node);
> +	hlist_del_init_rcu(&console->node);
>  
>  	/*
>  	 * <HISTORICAL>
> @@ -3269,6 +3307,10 @@ int unregister_console(struct console *console)
>  		console_first()->flags |= CON_CONSDEV;
>  
>  	console_unlock();
> +
> +	/* Ensure that all SRCU list walks have completed */
> +	synchronize_srcu(&console_srcu);

Again, we should explain why.

I agree that it should be there. It makes sure that the driver
is not longer used for printing messages before it can be removed.

> +
>  	console_sysfs_notify();
>  
>  	if (console->exit)
> -- 
> 2.30.2

Best Regards,
Petr
  

On 2022-10-21, Petr Mladek <pmladek@suse.com> wrote:
>> +#ifdef CONFIG_LOCKDEP
>> +bool console_srcu_read_lock_is_held(void)

>> +#ifdef CONFIG_LOCKDEP
>> +extern bool console_srcu_read_lock_is_held(void);
>> +#else
>> +static inline bool console_srcu_read_lock_is_held(void)
>> +{
>> +	return 1;
>> +}
>> +#endif
>
> srcu_read_lock_held() actually depends on CONFIG_DEBUG_LOCK_ALLOC.

I really want to keep @console_srcu private. For v3 I am changing it to
depend on ifdef CONFIG_DEBUG_LOCK_ALLOC.

>> @@ -2989,6 +3021,9 @@ void console_stop(struct console *console)
>>  	console_lock();
>>  	console->flags &= ~CON_ENABLED;
>>  	console_unlock();
>> +
>> +	/* Ensure that all SRCU list walks have completed */
>> +	synchronize_srcu(&console_srcu);
>
> What is the motivation for this synchronization, please?

For v3 I change it to:

    /*
     * Ensure that all SRCU list walks have completed. All contexts must
     * be able to see that this console is disabled so that (for example)
     * the caller can suspend the port without risk of another context
     * using the port.
     */

>> @@ -3179,6 +3214,17 @@ void register_console(struct console *newcon)
>>  		newcon->flags &= ~CON_PRINTBUFFER;
>>  	}
>>  
>> +	newcon->dropped = 0;
>> +	if (newcon->flags & CON_PRINTBUFFER) {
>> +		/* Get a consistent copy of @syslog_seq. */
>> +		mutex_lock(&syslog_lock);
>> +		newcon->seq = syslog_seq;
>> +		mutex_unlock(&syslog_lock);
>> +	} else {
>> +		/* Begin with next message. */
>> +		newcon->seq = prb_next_seq(prb);
>
> Hmm, prb_next_seq() is the next-to-be written message. It does not
> guarantee that all the existing messages already reached the boot
> console.
>
> Ideally, we should set it to con->seq from the related boot
> consoles. But we do not know which one it is.

Yes. It is really sad that we do not know this. We need to fix this boot
console handover at some point in the future.

> A good enough solution would be to set it to the minimal con->seq
> of all registered boot consoles. They would most likely be on
> the same sequence number. Anyway, con->seq has to be read under
> console_lock() at least at this stage of the patchset.

Well, for v3 I would do the following:

- explicitly have boot consoles also behave like CON_PRINTBUFFER

- use the oldest boot+enabled message

The code would include the additional changes:

-	if (newcon->flags & CON_PRINTBUFFER) {
+	if (newcon->flags & (CON_PRINTBUFFER | CON_BOOT)) {
 		/* Get a consistent copy of @syslog_seq. */
 		mutex_lock(&syslog_lock);
 		newcon->seq = syslog_seq;
 		mutex_unlock(&syslog_lock);
 	} else {
-		/* Begin with next message. */
+		/* Begin with next message added to the ringbuffer. */
 		newcon->seq = prb_next_seq(prb);
+
+		/*
+		 * If an enabled boot console is not caught up, start with
+		 * that message instead. That boot console will be
+		 * unregistered shortly and may be the same device.
+		 */
+		for_each_console(con) {
+			if ((con->flags & (CON_BOOT | CON_ENABLED)) == (CON_BOOT | CON_ENABLED) &&
+			    con->seq < newcon->seq) {
+				newcon->seq = con->seq;
+			}
+		}
 	}
>> +		hlist_add_behind_rcu(&newcon->node, console_list.first);
>>  	}
>>  	console_unlock();
>> +
>> +	/* No need to synchronize SRCU here! */
>
> This would deserve explanation why it is not needed. At least some
> hint.

For v3 I change it to:

    /*
     * No need to synchronize SRCU here! The caller does not rely
     * on all contexts being able to see the new console before
     * register_console() completes.
     */

>> @@ -3269,6 +3307,10 @@ int unregister_console(struct console *console)
>>  		console_first()->flags |= CON_CONSDEV;
>>  
>>  	console_unlock();
>> +
>> +	/* Ensure that all SRCU list walks have completed */
>> +	synchronize_srcu(&console_srcu);
>
> Again, we should explain why.

For v3 I change it to:

    /*
     * Ensure that all SRCU list walks have completed. All contexts
     * must not be able to see this console in the list so that any
     * exit/cleanup routines can be performed safely.
     */

John
  

On Mon 2022-10-31 14:12:36, John Ogness wrote:
> On 2022-10-21, Petr Mladek <pmladek@suse.com> wrote:
> >> @@ -3179,6 +3214,17 @@ void register_console(struct console *newcon)
> >>  		newcon->flags &= ~CON_PRINTBUFFER;
> >>  	}
> >>  
> >> +	newcon->dropped = 0;
> >> +	if (newcon->flags & CON_PRINTBUFFER) {
> >> +		/* Get a consistent copy of @syslog_seq. */
> >> +		mutex_lock(&syslog_lock);
> >> +		newcon->seq = syslog_seq;
> >> +		mutex_unlock(&syslog_lock);
> >> +	} else {
> >> +		/* Begin with next message. */
> >> +		newcon->seq = prb_next_seq(prb);
> >
> > Hmm, prb_next_seq() is the next-to-be written message. It does not
> > guarantee that all the existing messages already reached the boot
> > console.
> >
> > Ideally, we should set it to con->seq from the related boot
> > consoles. But we do not know which one it is.
> 
> Yes. It is really sad that we do not know this. We need to fix this boot
> console handover at some point in the future.
> 
> > A good enough solution would be to set it to the minimal con->seq
> > of all registered boot consoles. They would most likely be on
> > the same sequence number. Anyway, con->seq has to be read under
> > console_lock() at least at this stage of the patchset.
> 
> Well, for v3 I would do the following:
> 
> - explicitly have boot consoles also behave like CON_PRINTBUFFER
> 
> - use the oldest boot+enabled message
> 
> The code would include the additional changes:
> 
> -	if (newcon->flags & CON_PRINTBUFFER) {
> +	if (newcon->flags & (CON_PRINTBUFFER | CON_BOOT)) {
>  		/* Get a consistent copy of @syslog_seq. */
>  		mutex_lock(&syslog_lock);
>  		newcon->seq = syslog_seq;
>  		mutex_unlock(&syslog_lock);
>  	} else {
> -		/* Begin with next message. */
> +		/* Begin with next message added to the ringbuffer. */
>  		newcon->seq = prb_next_seq(prb);
> +
> +		/*
> +		 * If an enabled boot console is not caught up, start with
> +		 * that message instead. That boot console will be
> +		 * unregistered shortly and may be the same device.
> +		 */
> +		for_each_console(con) {
> +			if ((con->flags & (CON_BOOT | CON_ENABLED)) == (CON_BOOT | CON_ENABLED) &&
> +			    con->seq < newcon->seq) {
> +				newcon->seq = con->seq;
> +			}
> +		}
>  	}

Makes sense. Just please do it in a separate patch. It might
potentially change the behavior. And the problem have been
there since the global "console_seq" was moved to struct console_seq.

> >> +		hlist_add_behind_rcu(&newcon->node, console_list.first);
> >>  	}
> >>  	console_unlock();
> >> +

All the other proposed changes look good.

Best Regards,
Petr