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

[Petr Mladek]

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@xxxxxxxxxxxxx>
> ---
>  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