Re: [PATCH printk v2 38/38] printk, xen: fbfront: create/use safe function for forcing preferred
From: Petr Mladek <pmladek@suse.com>
Date: 2022-10-27 13:18:33
Also in:
dri-devel, lkml
On Wed 2022-10-19 17:02:00, John Ogness wrote:
quoted hunk ↗ jump to hunk
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.ogness@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