Messages in this thread | | | Date | Thu, 27 Oct 2022 15:18:21 +0200 | From | Petr Mladek <> | Subject | Re: [PATCH printk v2 38/38] printk, xen: fbfront: create/use safe function for forcing preferred |
| |
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.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
| |