Re: [PATCH] printk: CON_PRINTBUFFER console registration is a bit racy

From: Petr Mladek
Date: Fri Sep 14 2018 - 04:59:39 EST


On Fri 2018-09-14 11:34:28, Sergey Senozhatsky wrote:
> CON_PRINTBUFFER console registration requires us to do several
> preparation steps:
> - Rollback console_seq to replay logbuf messages which were already
> seen on other consoles;
> - Set exclusive_console flag so console_unlock() will ->write() logbuf
> messages only to the exclusive_console driver.
>
> The way we do it, however, is a bit racy
>
> logbuf_lock_irqsave(flags);
> console_seq = syslog_seq;
> console_idx = syslog_idx;
> logbuf_unlock_irqrestore(flags);
> << preemption enabled
> << irqs enabled
> exclusive_console = newcon;
> console_unlock();
>
> We rollback console_seq under logbuf_lock with IRQs disabled, but
> we set exclusive_console with local IRQs enabled and logbuf unlocked.
> If the system oops-es or panic-s before we set exclusive_console - and
> given that we have IRQs and preemption enabled there is such a
> possibility - we will re-play all logbuf messages to every registered
> console, which may be a bit annoying and time consuming.
>
> Move exclusive_console assignment under the same IRQs-disabled and
> logbuf_lock protected section where we rollback console_seq.
>
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@xxxxxxxxx>
> ---
> kernel/printk/printk.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 992bb6bb7ac2..c52a986a72b3 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2705,7 +2705,6 @@ void register_console(struct console *newcon)
> logbuf_lock_irqsave(flags);
> console_seq = syslog_seq;
> console_idx = syslog_idx;
> - logbuf_unlock_irqrestore(flags);
> /*
> * We're about to replay the log buffer. Only do this to the
> * just-registered console to avoid excessive message spam to
> @@ -2713,6 +2712,7 @@ void register_console(struct console *newcon)
> */
> exclusive_console = newcon;
> exclusive_console_stop_seq = console_seq;
> + logbuf_unlock_irqrestore(flags);

This would make a false feeling that these variables need to
be synchronized using logbug_lock. It might either confuse people
or it could get removed during a code clean up.

A better solution would be to explicitly disable preemption
around the entire section and add a comment.

Well, I am not sure if it is worth the code complexity.

Best Regards,
Petr