Re: [PATCH] printk: queue wake_up_klogd irq_work only if per-CPU areas are ready

[Petr Mladek]

On Tue 2020-03-03 13:40:59, Sergey Senozhatsky wrote:
> Lech Perczak [0] reports that after commit 1b710b1b10ef
> ("char/random: silence a lockdep splat with printk()")
> user-space syslog/kmsg readers are not able to read new
> kernel messages. The reason is printk_deferred() being
> called too early (as was pointed out by Petr and John).
> 
> Fix printk_deferred() and do not queue per-CPU irq_work
> before per-CPU areas are initialized.
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index ad4606234545..d951d35a0786 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -1147,12 +1159,25 @@ static void __init log_buf_add_cpu(void)
>  static inline void log_buf_add_cpu(void) {}
>  #endif /* CONFIG_SMP */
>  
> +static void __init set_percpu_data_ready(void)
> +{
> +	__printk_percpu_data_ready = true;
> +}
> +
>  void __init setup_log_buf(int early)
>  {
>  	unsigned long flags;
>  	char *new_log_buf;
>  	unsigned int free;
>  
> +	/*
> +	 * Some archs call setup_log_buf() multiple times - first is very
> +	 * early, e.g. from setup_arch(), and second - when percpu_areas
> +	 * are initialised.
> +	 */
> +	if (!early)
> +		set_percpu_data_ready();
> +
>  	if (log_buf != __log_buf)
>  		return;
>  
> diff --git a/kernel/printk/printk_safe.c b/kernel/printk/printk_safe.c
> index b4045e782743..d9a659a686f3 100644
> --- a/kernel/printk/printk_safe.c
> +++ b/kernel/printk/printk_safe.c
> @@ -27,7 +27,6 @@
a>   * There are situations when we want to make sure that all buffers
>   * were handled or when IRQs are blocked.
>   */
> -static int printk_safe_irq_ready __read_mostly;
>  
>  #define SAFE_LOG_BUF_LEN ((1 << CONFIG_PRINTK_SAFE_LOG_BUF_SHIFT) -	\
>  				sizeof(atomic_t) -			\
> @@ -51,7 +50,7 @@ static DEFINE_PER_CPU(struct printk_safe_seq_buf, nmi_print_seq);
>  /* Get flushed in a more safe context. */
>  static void queue_flush_work(struct printk_safe_seq_buf *s)
>  {
> -	if (printk_safe_irq_ready)
> +	if (printk_percpu_data_ready())
>  		irq_work_queue(&s->work);

This is not safe. printk_percpu_data_ready() returns true even before
s->work gets initialized by printk_safe_init().

Solution would be to call printk_safe_init() from
setup_log_buf() before calling set_percpu_data_ready().

Or I would keep printk_safe code as it. I hope that it will get
removed rather soon anyway.

Otherwise, it looks good to me.

Best Regards,
Petr