Re: [RFC][PATCH 2/2] printk: take console_sem when accessing console drivers list

From: Petr Mladek
Date: Wed Apr 24 2019 - 11:13:09 EST


On Tue 2019-04-23 15:25:11, Sergey Senozhatsky wrote:
> We need to take console_sem lock when we iterate console
> drivers list. Otherwise, another CPU can concurrently
> modify console drivers list or console drivers.
>
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@xxxxxxxxx>
> ---
> kernel/printk/printk.c | 19 +++++++++++++------
> 1 file changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index b0e361ca1bea..c2bccf58d03e 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2674,12 +2674,17 @@ void register_console(struct console *newcon)
> struct console_cmdline *c;
> static bool has_preferred;
>
> - if (console_drivers)
> - for_each_console(bcon)
> - if (WARN(bcon == newcon,
> - "console '%s%d' already registered\n",
> - bcon->name, bcon->index))
> - return;
> + console_lock();
> + if (console_drivers) {
> + for_each_console(bcon) {
> + if (bcon != newcon)
> + continue;
> + WARN(1, "console '%s%d' already registered\n",
> + bcon->name, bcon->index);
> + console_unlock();
> + return;
> + }
> + }
>
> /*
> * before we register a new CON_BOOT console, make sure we don't
> @@ -2691,6 +2696,7 @@ void register_console(struct console *newcon)
> if (!(bcon->flags & CON_BOOT)) {
> pr_info("Too late to register bootconsole %s%d\n",
> newcon->name, newcon->index);
> + console_unlock();
> return;
> }
> }
> @@ -2701,6 +2707,7 @@ void register_console(struct console *newcon)
>
> if (!has_preferred || bcon || !console_drivers)
> has_preferred = preferred_console >= 0;
> + console_unlock();

We should keep it until the console is added into the list. Otherwise
there are races with accessing the static has_preferred and
the global preferred_console variables.

Also the value of bcon should stay synchronized until we decide
about replaying the log.

IMHO, the only danger might be when con->match() or con->setup()
would want to take console_lock() as well. I checked few drivers
and they looked safe. But I did not check all of them.

What do you think, please?

Best Regards,
Petr