| Message ID | 20221107141638.3790965-34-john.ogness@linutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | reduce console_lock scope | expand |
On Mon 2022-11-07 15:22:31, 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_locked() and perform the full operation > under the console_list_lock. > > --- a/kernel/printk/printk.c > +++ b/kernel/printk/printk.c > @@ -3457,6 +3458,43 @@ int unregister_console(struct console *console) > } > EXPORT_SYMBOL(unregister_console); > > +/** > + * console_force_preferred_locked - force a registered console preferred > + * @con: The registered console to force preferred. > + * > + * Must be called under console_list_lock(). > + */ > +void console_force_preferred_locked(struct console *con) > +{ > + struct console *cur_pref_con; > + > + if (!console_is_registered_locked(con)) > + return; > + > + cur_pref_con = console_first(); > + > + /* Already preferred? */ > + if (cur_pref_con == con) > + return; > + > + hlist_del_init_rcu(&con->node); We actually should re-initialize the node only after all existing console list walks are finished. Se we should use here: hlist_del_rcu(&con->node); > + > + /* > + * Ensure that all SRCU list walks have completed so that the console > + * can be added to the beginning of the console list and its forward > + * list pointer can be re-initialized. The comment is right ;-) > + */ > + synchronize_srcu(&console_srcu); > + > + con->flags |= CON_CONSDEV; > + WARN_ON(!con->device); > + > + /* Only the new head can have CON_CONSDEV set. */ > + WRITE_ONCE(cur_pref_con->flags, cur_pref_con->flags & ~CON_CONSDEV); As mentioned in the reply for 7th patch, I would prefer to hide this WRITE_ONCE into a wrapper, e.g. console_set_flag(). It might also check that the console_list_lock is taken... > + hlist_add_behind_rcu(&con->node, console_list.first); > +} > +EXPORT_SYMBOL(console_force_preferred_locked); > + > /* > * Initialize the console device. This is called *early*, so > * we can't necessarily depend on lots of kernel help here. Best Regards, Petr
On 2022-11-10, Petr Mladek <pmladek@suse.com> wrote: >> +void console_force_preferred_locked(struct console *con) >> +{ >> + struct console *cur_pref_con; >> + >> + if (!console_is_registered_locked(con)) >> + return; >> + >> + cur_pref_con = console_first(); >> + >> + /* Already preferred? */ >> + if (cur_pref_con == con) >> + return; >> + >> + hlist_del_init_rcu(&con->node); > > We actually should re-initialize the node only after all existing > console list walks are finished. Se we should use here: > > hlist_del_rcu(&con->node); hlist_del_init_rcu() only re-initializes @pprev pointer. But maybe you are concerned that there is a window where list_unhashed() becomes true? I agree that it should be changed to hlist_del_rcu() because there should not be a window where this console appears unregistered. >> + /* Only the new head can have CON_CONSDEV set. */ >> + WRITE_ONCE(cur_pref_con->flags, cur_pref_con->flags & ~CON_CONSDEV); > > As mentioned in the reply for 7th patch, I would prefer to hide this > WRITE_ONCE into a wrapper, e.g. console_set_flag(). It might also > check that the console_list_lock is taken... Agreed. For v4 it will become: console_srcu_write_flags(cur_pref_con->flags & ~CON_CONSDEV); John
On Thu 2022-11-10 17:09:12, John Ogness wrote: > On 2022-11-10, Petr Mladek <pmladek@suse.com> wrote: > >> +void console_force_preferred_locked(struct console *con) > >> +{ > >> + struct console *cur_pref_con; > >> + > >> + if (!console_is_registered_locked(con)) > >> + return; > >> + > >> + cur_pref_con = console_first(); > >> + > >> + /* Already preferred? */ > >> + if (cur_pref_con == con) > >> + return; > >> + > >> + hlist_del_init_rcu(&con->node); > > > > We actually should re-initialize the node only after all existing > > console list walks are finished. Se we should use here: > > > > hlist_del_rcu(&con->node); > > hlist_del_init_rcu() only re-initializes @pprev pointer. Ah, I was not aware of it. > But maybe you > are concerned that there is a window where list_unhashed() becomes true? > I agree that it should be changed to hlist_del_rcu() because there > should not be a window where this console appears unregistered. Makes sense. > >> + /* Only the new head can have CON_CONSDEV set. */ > >> + WRITE_ONCE(cur_pref_con->flags, cur_pref_con->flags & ~CON_CONSDEV); > > > > As mentioned in the reply for 7th patch, I would prefer to hide this > > WRITE_ONCE into a wrapper, e.g. console_set_flag(). It might also > > check that the console_list_lock is taken... > > Agreed. For v4 it will become: > > console_srcu_write_flags(cur_pref_con->flags & ~CON_CONSDEV); I am happy that your are going to introduce an API for this. Just to be sure. The _srcu_ in the name means that the write will use WRITE_ONCE() so that it can be read safely in SRCU context using READ_ONCE(). Do I get it correctly, please? I expect that the counter part will be console_srcu_read_flags(). I like the name. It is better than _unsafe_ that I proposed earlier. Best Regards, Petr
On 2022-11-10, Petr Mladek <pmladek@suse.com> wrote: >>>> + /* Only the new head can have CON_CONSDEV set. */ >>>> + WRITE_ONCE(cur_pref_con->flags, cur_pref_con->flags & ~CON_CONSDEV); >>> >>> As mentioned in the reply for 7th patch, I would prefer to hide this >>> WRITE_ONCE into a wrapper, e.g. console_set_flag(). It might also >>> check that the console_list_lock is taken... >> >> Agreed. For v4 it will become: >> >> console_srcu_write_flags(cur_pref_con, cur_pref_con->flags & ~CON_CONSDEV); > > I am happy that your are going to introduce an API for this. > > Just to be sure. The _srcu_ in the name means that the write > will use WRITE_ONCE() so that it can be read safely in SRCU > context using READ_ONCE(). Do I get it correctly, please? Yes. > I expect that the counter part will be console_srcu_read_flags(). > I like the name. It is better than _unsafe_ that I proposed earlier. Since the only flag that is ever checked in this way is CON_ENABLED, I was planning on calling it console_srcu_is_enabled(). But I suppose I could have it be: (console_srcu_read_flags() & CON_ENABLED). That would keep it more abstract in case anyone should want to read other flag bits under SRCU. There are only 4 call sites that need this, so I suppose we don't need a special _is_enabled() variant. Thanks for the suggestion! John
On Mon 2022-11-07 15:22:31, 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_locked() and perform the full operation > under the console_list_lock. > > --- a/kernel/printk/printk.c > +++ b/kernel/printk/printk.c > @@ -3457,6 +3458,43 @@ int unregister_console(struct console *console) > } > EXPORT_SYMBOL(unregister_console); > > +/** > + * console_force_preferred_locked - force a registered console preferred > + * @con: The registered console to force preferred. > + * > + * Must be called under console_list_lock(). > + */ > +void console_force_preferred_locked(struct console *con) > +{ > + struct console *cur_pref_con; One more thing. It would make sense to check that it has really been called under console_list_lock(): lockdep_assert_console_list_lock_held(); > + > + if (!console_is_registered_locked(con)) > + return; > + > + cur_pref_con = console_first(); > + > + /* Already preferred? */ > + if (cur_pref_con == con) > + return; > + Best Regards, Petr
diff --git a/drivers/video/fbdev/xen-fbfront.c b/drivers/video/fbdev/xen-fbfront.c index 4d2694d904aa..8752d389e382 100644 --- a/drivers/video/fbdev/xen-fbfront.c +++ b/drivers/video/fbdev/xen-fbfront.c @@ -504,18 +504,14 @@ static void xenfb_make_preferred_console(void) if (console_set_on_cmdline) return; - console_lock(); + console_list_lock(); for_each_console(c) { if (!strcmp(c->name, "tty") && c->index == 0) break; } - console_unlock(); - 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_locked(c); + console_list_unlock(); } static int xenfb_resume(struct xenbus_device *dev) diff --git a/include/linux/console.h b/include/linux/console.h index cdae70e27377..b6b5d796d15c 100644 --- a/include/linux/console.h +++ b/include/linux/console.h @@ -273,6 +273,7 @@ enum con_flush_mode { }; extern int add_preferred_console(char *name, int idx, char *options); +extern void console_force_preferred_locked(struct console *con); extern void register_console(struct console *); extern int unregister_console(struct console *); extern void console_lock(void); diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c index be40a9688403..d74e6e609f7d 100644 --- a/kernel/printk/printk.c +++ b/kernel/printk/printk.c @@ -247,9 +247,10 @@ int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write, void console_list_lock(void) { /* - * In unregister_console(), synchronize_srcu() is called with the - * console_list_lock held. Therefore it is not allowed that the - * console_list_lock is taken with the srcu_lock held. + * In unregister_console() and console_force_preferred_locked(), + * synchronize_srcu() is called with the console_list_lock held. + * Therefore it is not allowed that the console_list_lock is taken + * with the srcu_lock held. * * Whether or not this context is in the read-side critical section * can only be detected if the appropriate debug options are enabled. @@ -3457,6 +3458,43 @@ int unregister_console(struct console *console) } EXPORT_SYMBOL(unregister_console); +/** + * console_force_preferred_locked - force a registered console preferred + * @con: The registered console to force preferred. + * + * Must be called under console_list_lock(). + */ +void console_force_preferred_locked(struct console *con) +{ + struct console *cur_pref_con; + + if (!console_is_registered_locked(con)) + return; + + cur_pref_con = console_first(); + + /* Already preferred? */ + if (cur_pref_con == con) + return; + + hlist_del_init_rcu(&con->node); + + /* + * Ensure that all SRCU list walks have completed so that the console + * can be added to the beginning of the console list and its forward + * list pointer can be re-initialized. + */ + synchronize_srcu(&console_srcu); + + con->flags |= CON_CONSDEV; + WARN_ON(!con->device); + + /* Only the new head can have CON_CONSDEV set. */ + WRITE_ONCE(cur_pref_con->flags, cur_pref_con->flags & ~CON_CONSDEV); + hlist_add_behind_rcu(&con->node, console_list.first); +} +EXPORT_SYMBOL(console_force_preferred_locked); + /* * Initialize the console device. This is called *early*, so * we can't necessarily depend on lots of kernel help here.
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_locked() and perform the full operation under the console_list_lock. Signed-off-by: John Ogness <john.ogness@linutronix.de> --- drivers/video/fbdev/xen-fbfront.c | 12 +++------ include/linux/console.h | 1 + kernel/printk/printk.c | 44 ++++++++++++++++++++++++++++--- 3 files changed, 46 insertions(+), 11 deletions(-)