linux-kernel - Re: [PATCH 2/3] printk/console: Clean up logic around fallback console

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 14 Jun 2017 17:38:03 +0900
From:   Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
To:     Petr Mladek <pmladek@...e.com>
Cc:     Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Aleksey Makarov <aleksey.makarov@...aro.org>,
        Sabrina Dubroca <sd@...asysnail.net>,
        Sudeep Holla <sudeep.holla@....com>,
        linux-kernel@...r.kernel.org,
        Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
Subject: Re: [PATCH 2/3] printk/console: Clean up logic around fallback
 console

Thanks for taking a look at this.

On (06/13/17 14:54), Petr Mladek wrote:
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 8ebc480fdbc6..6e651f68bffd 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2413,51 +2413,45 @@ void register_console(struct console *newcon)
>  	unsigned long flags;
>  	struct console *bcon = NULL;

I was going to ask if we can also rename `bcon' to just `con', because not
every console we see in for_each_console() is a boot console, that's why we
test for CON_BOOT bit set and then 'if boot_console->flags & CON_BOOT' looks
a bit odd; but looking at the bottom of register_console() we probably better
keep it the way it is. but who knows... suggestions?


>  	struct console_cmdline *c;
> -	static bool has_preferred;
> +	bool have_real_console = false;

ok, `real console' probably can work. boot console can also be real,
right? it's just it's not a fully featured one. may be there is a better
naming here, can't immediately think of any tho. ideas?


> -	if (console_drivers)
> -		for_each_console(bcon)
> +	if (console_drivers) {
> +		for_each_console(bcon) {
>  			if (WARN(bcon == newcon,
>  					"console '%s%d' already registered\n",
>  					bcon->name, bcon->index))
>  				return;
>  
> -	/*
> -	 * before we register a new CON_BOOT console, make sure we don't
> -	 * already have a valid console

'a valid console'. so there are boot consoles, valid consoles and real
consoles  :)  it's good to see this comment being removed then.

> -	 */
> -	if (console_drivers && newcon->flags & CON_BOOT) {
> -		/* find the last or real console */
> -		for_each_console(bcon) {
> -			if (!(bcon->flags & CON_BOOT)) {
> -				pr_info("Too late to register bootconsole %s%d\n",
> -					newcon->name, newcon->index);
> -				return;
> -			}
> +			if (!(bcon->flags & CON_BOOT))
> +				have_real_console = true;
>  		}
>  	}
>  
> +	if (have_real_console && newcon->flags & CON_BOOT) {
> +		pr_info("Too late to register bootconsole %s%d\n",
> +			newcon->name, newcon->index);
> +		return;
> +	}
> +
> +	/*
> +	 * We have not registered the real preferred console if a boot
> +	 * console is still the first one.
> +	 */
>  	if (console_drivers && console_drivers->flags & CON_BOOT)
>  		bcon = console_drivers;
>  
> -	if (!has_preferred || bcon || !console_drivers)
> -		has_preferred = preferred_console >= 0;
> -
>  	/*
> -	 *	See if we want to use this console driver. If we
> -	 *	didn't select a console we take the first one
> -	 *	that registers here.
> +	 * Enable any boot console and the first real one when
> +	 * consoles are not configured.
>  	 */
> -	if (!has_preferred) {
> +	if (!have_real_console && preferred_console < 0) {
>  		if (newcon->index < 0)
>  			newcon->index = 0;
>  		if (newcon->setup == NULL ||
>  		    newcon->setup(newcon, NULL) == 0) {
>  			newcon->flags |= CON_ENABLED;
> -			if (newcon->device) {
> +			if (newcon->device)
>  				newcon->flags |= CON_CONSDEV;
> -				has_preferred = true;
> -			}
>  		}
>  	}

dunno, looking at this `newcon->flags |= CON_CONSDEV' I think I'd
probably also add the following patch to the series

---

diff --git a/include/linux/console.h b/include/linux/console.h
index b8920a031a3e..a2525e0d7605 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -127,7 +127,7 @@ static inline int con_debug_leave(void)
  */
 
 #define CON_PRINTBUFFER        (1)
-#define CON_CONSDEV    (2) /* Last on the command line */
+#define CON_CONSDEV    (2)
 #define CON_ENABLED    (4)
 #define CON_BOOT       (8)
 #define CON_ANYTIME    (16) /* Safe to call when cpu is offline */

---


CON_CONSDEV can be at any place on the command line. right? and
unregister_console() even takes care of it.


	/*
	 * If this isn't the last console and it has CON_CONSDEV set, we
	 * need to set it on the next preferred console.
	 */
	if (console_drivers != NULL && console->flags & CON_CONSDEV)
		console_drivers->flags |= CON_CONSDEV;



so, as far as I can see, the patch shouldn't change the logic of
registration. just a clean up. need to run tests/etc. etc. I just
read the patch so far.

	-ss