linux-kernel - Re: [PATCH v3 0/3] Add support for DEVNAME:0.0 style hardware based addressing

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] [day] [month] [year] [list]
Message-ID: <20231204075131.GK5169@atomide.com>
Date:   Mon, 4 Dec 2023 09:51:31 +0200
From:   Tony Lindgren <tony@...mide.com>
To:     Petr Mladek <pmladek@...e.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jirislaby@...nel.org>,
        "David S . Miller" <davem@...emloft.net>,
        Sergey Senozhatsky <senozhatsky@...omium.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Andy Shevchenko <andriy.shevchenko@...el.com>,
        Dhruva Gole <d-gole@...com>,
        Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
        John Ogness <john.ogness@...utronix.de>,
        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
Subject: Re: [PATCH v3 0/3] Add support for DEVNAME:0.0 style hardware based
 addressing

* Petr Mladek <pmladek@...e.com> [231201 14:36]:
> Well, my understanding is that it solves the problem only for the newly
> added console=DEVICENAME:0.0 format. But it does not handle the
> existing problems with matching console names passed via earlycon=
> and console= parameters. Am I right?

Yes that's where the remaining problems are.

> Now, the bad news. This patchset causes regressions which are
> not acceptable. I have found two so far but there might be more.
> 
> I used the following kernel command line:
> 
>    earlycon=uart8250,io,0x3f8,115200 console=ttyS0,115200 console=tty0 ignore_loglevel log_buf_len=1M
> 
> 
> 1. 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
> 
>    This is most likely caused by the different ordering of
>    __add_preferred_console() calls.

Yes I noticed that too. We can't drop the console parsing from
console_setup() until we have some solution for flagging
register_console() that we do have a console specified on the
kernel command line and try_enable_default_console() should not
be called. It seems some changes to the console_set_on_cmdline
handling might do the trick here.

>    The ordering is important because it defines which console
>    will get associated with /dev/console. It is a so called
>    preferred console defined by the last console= parameter.
> 
>    Unfortunately also the ordering of the other parameters
>    is important when a console defined by the last console=
>    parameter is not registered at all. In this case,
>    /dev/console gets associated with the first console
>    with tty binding according to the order on the command line.
> 
>    If you think that it is weird behavior then I agree.
>    But it is a historical mess. It is how people used it
>    when the various features were added. Many changes
>    in this code caused regressions and had to be reverted.

Yeah agreed it's a mess :)

>    See the following to get the picture:
> 
>        + commit c6c7d83b9c9e6a8 ("Revert "console: don't
> 	 prefer first registered if DT specifies stdout-path")
> 
>        + commit dac8bbbae1d0ccb ("Revert "printk: fix double
> 	 printing with earlycon"").

OK thanks.

> 2. The serial console gets registered much later with this
>    patchset:
> 
> 	original # dmesg | grep printk:
> 	[    0.000000] printk: legacy bootconsole [uart8250] enabled
> 	[    0.000000] printk: debug: ignoring loglevel setting.
> 	[    0.016859] printk: log_buf_len: 1048576 bytes
> 	[    0.017324] printk: early log buf free: 259624(99%)
> 	[    0.141859] printk: legacy console [tty0] enabled
> 	[    0.142399] printk: legacy bootconsole [uart8250] disabled
> 	[    0.143032] printk: legacy console [ttyS0] enabled
> 
>    vs.
> 
> 	patched # dmesg | grep printk:
> 	[    0.000000] printk: legacy bootconsole [uart8250] enabled
> 	[    0.000000] printk: debug: ignoring loglevel setting.
> 	[    0.018142] printk: log_buf_len: 1048576 bytes
> 	[    0.018757] printk: early log buf free: 259624(99%)
> 	[    0.160706] printk: legacy console [tty0] enabled
> 	[    0.161213] printk: legacy bootconsole [uart8250] disabled
> 	[    1.592929] printk: legacy console [ttyS0] enabled
> 
>    This is pretty bad because it would complicate or even prevent
>    debugging of the boot stage via serial console.

I think I have a patch coming for 8250 isa ports for that issue.
This issue should go away if we call add_preferred_console_match()
from serial8250_isa_init_ports() with options for the port like
"ttyS0", "ttyS", 0.

>    The graphical console is not usable when the system dies. Also
>    finding the right arguments for the earlycon= parameter is
>    tricky so that people enable it only when they have to debug
>    very early messages.
> 
> 
> I am going to look at the patches more closely to see if I could
> provide some hints.

Great, help with the early console handling is much appreciated.

I'll post an updated patchset this week that does not touch
console_setup() beyond saving the console options. And then we
hopefully have something that avoids the regressions and can be
used for further changes later on.

Regards,

Tony