Re: [PATCHv2 3/4] printk: factor out register_console() code

From: Petr Mladek
Date: Wed May 15 2019 - 10:38:23 EST


On Fri 2019-04-26 14:33:01, 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. Current register_console() has several
> race conditions - for_each_console() must be done under console_sem.
>
> Factor out console registration code and hold console_sem throughout
> entire registration process. Note that we need to unlock console_sem
> and lock it again after we added new console to the list and before
> we unregister boot consoles. This might look a bit weird, but this
> is how we print pending logbuf messages to all registered and
> available consoles.

My main concern was whether we could call newcon->setup() under
console_lock. I checked many console drivers and all looked safe.
There should not be much reasons to do it.


> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@xxxxxxxxx>
> ---
> kernel/printk/printk.c | 15 ++++++++++-----
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 3ac71701afa3..3b36e26d4a51 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2771,7 +2771,6 @@ void register_console(struct console *newcon)
> * Put this console in the list - keep the
> * preferred driver at the head of the list.
> */
> - console_lock();
> if ((newcon->flags & CON_CONSDEV) || console_drivers == NULL) {
> newcon->next = console_drivers;
> console_drivers = newcon;
> @@ -2818,6 +2817,7 @@ void register_console(struct console *newcon)
>
> console_unlock();
> console_sysfs_notify();
> + console_lock();

I have got an idea how to get rid of this weirdness:

1. The check for bcon seems to be just an optimization. There is not need
to remove boot consoles when there are none.

2. The condition (newcon->flags & (CON_CONSDEV|CON_BOOT)) == CON_CONSDEV)
is valid only when the preferred console was really added.

Therefore we could move the code to a separate function, e.g.

void unregister_boot_consoles(void)
{
struct console *bcon;

console_lock();
for_each_console(bcon)
if (bcon->flags & CON_BOOT)
__unregister_console(bcon);
}
console_unlock();
console_sysfs_notify();
}

Then we could do something like:

void register_console(struct console *newcon)
{
bool newcon_is_preferred = false;

console_lock();
__register_console(newcon);
if ((newcon->flags & (CON_CONSDEV|CON_BOOT)) == CON_CONSDEV)
newcon_is_preferred = true;
console_unlock();
console_sysfs_notify();

/*
* By unregistering the bootconsoles after we enable the real console
* we get the "console xxx enabled" message on all the consoles -
* boot consoles, real consoles, etc - this is to ensure that end
* users know there might be something in the kernel's log buffer that
* went to the bootconsole (that they do not see on the real console)
*/
if (newcon_is_preferred && !keep_bootcon)
unregister_boot_consoles();
}

How does that sound?

Otherwise, the patch looks fine to me.

Best Regards,
Petr