linux-kernel - Re: [RFC/PATCH] printk: Fix preferred console selection with multiple matches

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]

Message-ID: <98df321d16adb67c5579ac4b67d845fc0c2c97df.camel@kernel.crashing.org>
Date:   Wed, 11 Dec 2019 09:26:28 +1100
From:   Benjamin Herrenschmidt <benh@...nel.crashing.org>
To:     Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
Cc:     linux-kernel@...r.kernel.org, Petr Mladek <pmladek@...e.com>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        AlekseyMakarov <aleksey.makarov@...aro.org>
Subject: Re: [RFC/PATCH] printk: Fix preferred console selection with
 multiple matches

On Tue, 2019-12-10 at 17:01 +0900, Sergey Senozhatsky wrote:
> On (19/12/10 11:57), Benjamin Herrenschmidt wrote:
> [..]
> >  - add_preferred_console is called early to register "uart0". In
> > our case that happens from acpi_parse_spcr() on arm64 since the
> > "enable_console" argument is true on that architecture. This causes
> > "uart0" to become entry 0 of the console_cmdline array.
> 
> Hmm, two independent console list configuration sources.

Yes, we've had that for a while. So far it "worked" because the
explicit calls to add_preferred_console() tends to happen before the
parsing of the command line, and thus are "overriden" by the latter if
any.

> [..]
> > +++ b/kernel/printk/printk.c
> > @@ -2646,8 +2646,8 @@ void register_console(struct console *newcon)
> >  		if (i == preferred_console) {
> >  			newcon->flags |= CON_CONSDEV;
> >  			has_preferred = true;
> > +			break;
> >  		}
> > -		break;
> >  	}
> >  
> >  	if (!(newcon->flags & CON_ENABLED))
> 
> Wouldn't this, basically, mean that we want to match only consoles,
> which were in the kernel's console= cmdline? IOW, ignore consoles
> that were placed into consoles list via alternative path - ACPI.

No not exactly. Architectures/platforms use add_preferred_console()
(such as arm64 with ACPI but powerpc at least does it too) based on
various factors to select a reasonable "default" for that specific
platform. Without that the kernel will basically default to the first
one to register which may not be what you want.

The command line ones however want to override the defaults (provided
they exist, ie, it's possible that whever is specified on the command
line doesn't actually exist, and thus shall be ignored. That typically
happens when there is either no match or ->setup fails).

> Hmm.
> 
> The patch may affect setups where alias matching is expected to
> happen. E.g.:
> 
> 	console=uartFOO,BAR
> 
> Is 8250 the only console that does alias matching?

Why would the patch affect this negatively ? Today we stop on the first
match, mark the driver enabled, and make it preferred if the match
index matches preferred_console.

My patch makes us continue searching if it wasnt' the preferred console
(but we still mark it enabled). Which means either of those two things
happen:

 - No more match or another match that isn't the preferred_console,
this won't change the existing behaviour.

 - Another match that is marked preferred_console, in which case in
addition to being enabled, the newly registered console will also be
made the default console (ie, first in the list with CONSDEV set). This
is actually what we want ! IE. The console matches the last specified
one on the command line.

Cheers,
Ben.

> 	-ss