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.senozhat...@gmail.com> > --- > 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
