On Wed 2022-10-19 17:02:00, John Ogness wrote:
> With commit 9e124fe16ff2("xen: Enable console tty by default in domU
> if it's not a dummy") a hack was implemented to make sure that the
> tty console remains the console behind the /dev/console device. The
> main problem with the hack is that, after getting the console pointer
> to the tty console, it is assumed the pointer is still valid after
> releasing the console_sem. This assumption is incorrect and unsafe.
>
> Make the hack safe by introducing a new function
> console_force_preferred() to perform the full operation under
> the console_list_lock.
>
> Signed-off-by: John Ogness <john.ogn...@linutronix.de>
> ---
> drivers/video/fbdev/xen-fbfront.c | 8 +---
> include/linux/console.h | 1 +
> kernel/printk/printk.c | 69 +++++++++++++++++++------------
> 3 files changed, 46 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/video/fbdev/xen-fbfront.c
> b/drivers/video/fbdev/xen-fbfront.c
> index 2552c853c6c2..aa362b25a60f 100644
> --- a/drivers/video/fbdev/xen-fbfront.c
> +++ b/drivers/video/fbdev/xen-fbfront.c
> @@ -512,12 +512,8 @@ static void xenfb_make_preferred_console(void)
> }
> console_srcu_read_unlock(cookie);
>
> - if (c) {
> - unregister_console(c);
> - c->flags |= CON_CONSDEV;
> - c->flags &= ~CON_PRINTBUFFER; /* don't print again */
> - register_console(c);
> - }
> + if (c)
> + console_force_preferred(c);
I would prefer to fix this a clean way. The current code is a real hack.
It tries to find a console under console_srcu. Then the console
is unregistered, flags are modified, and gets registered again.
The locks are released between all these operations.
I would suggest to implement:
void console_force_preferred_locked(struct console *new_pref_con)
{
struct console *cur_pref_con;
assert_console_list_lock_held();
if (hlist_unhashed(&new_pref_con->node))
return;
for_each_console(cur_pref_con) {
if (cur_pref_con->flags & CON_CONSDEV)
break;
}
/* Already preferred? */
if (cur_pref_con == new_pref_con)
return;
hlist_del_init_rcu(&new_pref_con->node);
/*
* Ensure that all SRCU list walks have completed before @con
* is added back as the first console
*/
synchronize_srcu()
hlist_add_behind_rcu(&new_pref_con->node, console_list.first);
cur_pref_con->flags &= ~CON_CONSDEV;
new_pref_con->flags |= CON_CONSDEV;
}
And do:
static void xenfb_make_preferred_console(void)
{
struct console *c;
if (console_set_on_cmdline)
return;
console_list_lock();
for_each_console(c) {
if (!strcmp(c->name, "tty") && c->index == 0)
break;
}
if (c)
console_force_preferred_locked(c);
console_list_unlock();
}
It is a more code. But it is race-free. Also it is much more clear
what is going on.
How does this sound, please?
Best Regards,
Petr
