linux-kernel - Re: [PATCH v7 1/7] printk: Save console options for add_preferred_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]

Message-ID: <ZlRqz2b0ZrtkxScL@tlindgre-MOBL1>
Date: Mon, 27 May 2024 14:13:19 +0300
From: Tony Lindgren <tony.lindgren@...ux.intel.com>
To: Petr Mladek <pmladek@...e.com>
Cc: Tony Lindgren <tony@...mide.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jiri Slaby <jirislaby@...nel.org>, Jonathan Corbet <corbet@....net>,
	Steven Rostedt <rostedt@...dmis.org>,
	John Ogness <john.ogness@...utronix.de>,
	Sergey Senozhatsky <senozhatsky@...omium.org>,
	"David S . Miller" <davem@...emloft.net>,
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	Dhruva Gole <d-gole@...com>,
	Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
	Johan Hovold <johan@...nel.org>,
	Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
	Vignesh Raghavendra <vigneshr@...com>, linux-kernel@...r.kernel.org,
	linux-serial@...r.kernel.org, Sebastian Reichel <sre@...nel.org>,
	linux-doc@...r.kernel.org
Subject: Re: [PATCH v7 1/7] printk: Save console options for
 add_preferred_console_match()

Hi,

On Fri, May 24, 2024 at 06:06:21PM +0200, Petr Mladek wrote:
> I have finally found time to looks at this again.

Great good to hear.

> First, about breaking the preferred console:
> 
> The patchset still causes the regression with /dev/console association
> as already reported for v3, see
> https://lore.kernel.org/r/ZWnvc6-LnXdjOQLY@alley

Thanks and sorry for missing this issue. I thought I had this issue
already handled, but looking at what I tested with earlier, looks like
I had the console options the wrong way around.
 
> I used the following kernel command line:
> 
>    earlycon=uart8250,io,0x3f8,115200 console=ttyS0,115200 console=tty0 ignore_loglevel log_buf_len=1M
> 
> The patchset caused that /dev/console became associated with
> ttyS0 instead of tty0, see the "C" flag:
> 
> 	original # cat /proc/consoles
> 	tty0                 -WU (EC    )    4:1
> 	ttyS0                -W- (E  p a)    4:64
> 
>    vs.
> 
> 	patched # cat /proc/consoles
> 	ttyS0                -W- (EC p a)    4:64
> 	tty0                 -WU (E     )    4:1

OK

> I have added some debugging messages which nicely show the reason.
> In the original code, __add_preferred_console() is called twice
> when processing the command line:
> 
> [    0.099312] __add_preferred_console[0]: ttyS, 0 (preferrred_console == 0)
> [    0.099982] __add_preferred_console[1]: tty, 0 (preferrred_console == 1)

OK thanks for tracking down where things go wrong.

> The code thinks that "ttyS0" has been mentioned on the command line
> once again. And preferred_console is _wrongly_ set back to '0'.
> 
> My view:
> 
> The delayed __add_preferred_console() is a way to hell.
> 
> The preferences are defined by the ordering on the command line.
> All entries have to be added when the command line options are
> being proceed to keep the order.

To me it seems we can fix this by keeping track of the console position
in the kernel command line. I'll send a fix for this to discuss.

> A solution might be to store "devname" separately in
> struct console_cmdline and allow empty "name". We could
> implement then a function similar to
> add_preferred_console_match() which would try to match
> "devname" and set/update "name", "index" value when matched.
> 
> Note that we might also need to add some synchronization
> if it might be possible to modify struct console_cmdline
> in parallel.

OK certainly no objection from me if we can make this happen without
making things more complex :)
 
> Second, about the possible duplication:
> 
> I might get it wrong. IMHO, in principle, this patchset tries
> to achieve similar thing as the "match()" callback, see
> the commit c7cef0a84912cab3c9 ("console: Add extensible
> console matching").
> 
> The .match() callback in struct console is to match, for example,
> console=uart8250,io,0x3f8,115200 when the uart8250 driver
> calls register_console() when it is being properly initialized
> as "ttyS".
> 
> BTW: The .match() needs saved options because it internally calls
>      .setup() callback. IMHO, this is a very ugly detail
>      which complicates design of the register_console() code.
> 
> 
> Both approaches try to match a "driver/device-specific name" with
> the generic "ttySX".
> 
>     console=uart8250,io,0x3f8,115200	=> ttyS0
> vs.
>     console=00:00:0.0,115200		=> ttyS0
> 
> 
> Where console=uart8250,io,0x3f8,115200 is handled by:
> 
>     - "uart" is added to console_cmdline[]
>     - matched directly via newcon->match() callback
> 
> vs. console=00:00:0.0,115200
> 
>     - 00:00:0.0 is added to conopt[]
>     - "ttyS0" added to console_cmdline[] when "00:00:0.0" initialized
>     - "ttyS0" is then matched directly
> 
> 
> Question: Would it it be able to use the existing .match() callback
> 	  also to match the DEVNAME?
> 
> 	  Or somehow reuse the approach?

Thanks, I'll take a look if .match(), or some parts related to it, can
be used.

> 	  Could register_console() know how to generate possible
> 	  DEVNAME for the given struct console?

I don't think we can make much assumptions about the devices early on,
and we also have the console index -1 issue.

Regards,

Tony