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 <[email protected]>
> ---
>  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

Reply via email to