diff mbox series

[printk,v2,38/38] printk, xen: fbfront: create/use safe function for forcing preferred

Message ID 20221019145600.1282823-39-john.ogness@linutronix.de
State New
Headers
Series reduce console_lock scope |

Commit Message

John Ogness Oct. 19, 2022, 2:56 p.m. UTC 
  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(-)

Comments

Petr Mladek Oct. 27, 2022, 1:18 p.m. UTC | #1 
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
John Ogness Oct. 27, 2022, 1:35 p.m. UTC | #2 
On 2022-10-27, Petr Mladek <pmladek@suse.com> wrote:
>> -	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.
>
> [...]
>
> I would suggest to implement:
>
> [...]
>
> 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?

I wasn't sure if any of the other preferred-console magic in
register_console() was needed, which is why I kept a full
register_console() call. But if it really is just about forcing it the
head and setting a new CON_CONSDEV, then your suggestion is much
simpler. Thanks.

John
Petr Mladek Oct. 27, 2022, 2:27 p.m. UTC | #3 
On Thu 2022-10-27 15:41:23, John Ogness wrote:
> On 2022-10-27, Petr Mladek <pmladek@suse.com> wrote:
> >> -	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.
> >
> > [...]
> >
> > I would suggest to implement:
> >
> > [...]
> >
> > 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?
> 
> I wasn't sure if any of the other preferred-console magic in
> register_console() was needed, which is why I kept a full
> register_console() call. But if it really is just about forcing it the
> head and setting a new CON_CONSDEV, then your suggestion is much
> simpler. Thanks.

I believe that it is just this. I have spent a lot of time
investigating these hacks when thinking about refactoring
of register_console().

Best Regards,
Petr
diff mbox series

Patch

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);
 }
 
 static int xenfb_resume(struct xenbus_device *dev)
diff --git a/include/linux/console.h b/include/linux/console.h
index bf1e8136424a..41378b00bbdd 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -235,6 +235,7 @@  enum con_flush_mode {
 };
 
 extern int add_preferred_console(char *name, int idx, char *options);
+extern void console_force_preferred(struct console *c);
 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 840d581c4b23..9a056a42b8d8 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3207,38 +3207,17 @@  static void try_enable_default_console(struct console *newcon)
 
 static int unregister_console_locked(struct console *console);
 
-/*
- * The console driver calls this routine during kernel initialization
- * to register the console printing procedure with printk() and to
- * print any messages that were printed by the kernel before the
- * console driver was initialized.
- *
- * This can happen pretty early during the boot process (because of
- * early_printk) - sometimes before setup_arch() completes - be careful
- * of what kernel features are used - they may not be initialised yet.
- *
- * There are two types of consoles - bootconsoles (early_printk) and
- * "real" consoles (everything which is not a bootconsole) which are
- * handled differently.
- *  - Any number of bootconsoles can be registered at any time.
- *  - As soon as a "real" console is registered, all bootconsoles
- *    will be unregistered automatically.
- *  - Once a "real" console is registered, any attempt to register a
- *    bootconsoles will be rejected
- */
-void register_console(struct console *newcon)
+static void register_console_locked(struct console *newcon)
 {
 	struct console *con;
 	bool bootcon_enabled = false;
 	bool realcon_enabled = false;
 	int err;
 
-	console_list_lock();
-
 	for_each_console(con) {
 		if (WARN(con == newcon, "console '%s%d' already registered\n",
 					 con->name, con->index)) {
-			goto unlock;
+			return;
 		}
 
 		if (con->flags & CON_BOOT)
@@ -3251,7 +3230,7 @@  void register_console(struct console *newcon)
 	if (newcon->flags & CON_BOOT && realcon_enabled) {
 		pr_info("Too late to register bootconsole %s%d\n",
 			newcon->name, newcon->index);
-		goto unlock;
+		return;
 	}
 
 	/*
@@ -3282,7 +3261,7 @@  void register_console(struct console *newcon)
 
 	/* printk() messages are not printed to the Braille console. */
 	if (err || newcon->flags & CON_BRL)
-		goto unlock;
+		return;
 
 	/*
 	 * If we have a bootconsole, and are switching to a real console,
@@ -3346,7 +3325,31 @@  void register_console(struct console *newcon)
 				unregister_console_locked(con);
 		}
 	}
-unlock:
+}
+
+/*
+ * The console driver calls this routine during kernel initialization
+ * to register the console printing procedure with printk() and to
+ * print any messages that were printed by the kernel before the
+ * console driver was initialized.
+ *
+ * This can happen pretty early during the boot process (because of
+ * early_printk) - sometimes before setup_arch() completes - be careful
+ * of what kernel features are used - they may not be initialised yet.
+ *
+ * There are two types of consoles - bootconsoles (early_printk) and
+ * "real" consoles (everything which is not a bootconsole) which are
+ * handled differently.
+ *  - Any number of bootconsoles can be registered at any time.
+ *  - As soon as a "real" console is registered, all bootconsoles
+ *    will be unregistered automatically.
+ *  - Once a "real" console is registered, any attempt to register a
+ *    bootconsoles will be rejected
+ */
+void register_console(struct console *newcon)
+{
+	console_list_lock();
+	register_console_locked(newcon);
 	console_list_unlock();
 }
 EXPORT_SYMBOL(register_console);
@@ -3411,6 +3414,20 @@  int unregister_console(struct console *console)
 }
 EXPORT_SYMBOL(unregister_console);
 
+void console_force_preferred(struct console *c)
+{
+	console_list_lock();
+
+	if (unregister_console_locked(c) == 0) {
+		c->flags |= CON_CONSDEV;
+		c->flags &= ~CON_PRINTBUFFER; /* don't print again */
+		register_console_locked(c);
+	}
+
+	console_list_unlock();
+}
+EXPORT_SYMBOL(console_force_preferred);
+
 /*
  * Initialize the console device. This is called *early*, so
  * we can't necessarily depend on lots of kernel help here.