linux-kernel - Re: [PATCH v3 3/3] serial: core: Rename preferred console handling for match and update

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: Tue, 18 Jun 2024 15:51:44 +0200
From: Petr Mladek <pmladek@...e.com>
To: Tony Lindgren <tony.lindgren@...ux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jiri Slaby <jirislaby@...nel.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	John Ogness <john.ogness@...utronix.de>,
	Sergey Senozhatsky <senozhatsky@...omium.org>,
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	Tony Lindgren <tony@...mide.com>,
	Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
	linux-serial@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 3/3] serial: core: Rename preferred console handling
 for match and update

On Tue 2024-06-18 07:54:50, Tony Lindgren wrote:
> We are now matching and updating the preferred console, not adding it.
> Let's update the naming accordingly to avoid confusion.
> 
> --- a/drivers/tty/serial/serial_base_bus.c
> +++ b/drivers/tty/serial/serial_base_bus.c
> @@ -304,7 +305,7 @@ int serial_base_add_preferred_console(struct uart_driver *drv,

I was curious whether this patch renamed everything. And it seems
that it did not rename serial_base_add_preferred_console().

>  	const char *port_match __free(kfree) = NULL;
>  	int ret;
>  
> -	ret = serial_base_add_prefcon(drv->dev_name, port->line);
> +	ret = serial_base_match_and_update_prefcon(drv->dev_name, port->line);
>  	if (ret)
>  		return ret;
>  

Honestly, I do not understand what are all these layers for.
Especially, serial_base_match_and_update_prefcon() looks suspicious:

static int serial_base_match_and_update_prefcon(const char *name, int idx)
{
	const char *char_match __free(kfree) = NULL;
	const char *nmbr_match __free(kfree) = NULL;
	int ret;

	/* Handle ttyS specific options */
	if (strstarts(name, "ttyS")) {
		/* No name, just a number */
		nmbr_match = kasprintf(GFP_KERNEL, "%i", idx);
		if (!nmbr_match)
			return -ENODEV;

		ret = serial_base_match_and_update_one_prefcon(nmbr_match, name, idx);
		if (ret)
			return ret;

		/* Sparc ttya and ttyb */
		ret = serial_base_add_sparc_console(name, idx);
		if (ret)
			return ret;
	}

	/* Handle the traditional character device name style console=ttyS0 */
	char_match = kasprintf(GFP_KERNEL, "%s%i", name, idx);
	if (!char_match)
		return -ENOMEM;

	return serial_base_match_and_update_one_prefcon(char_match, name, idx);
}

It seems to try whether c->devname matches a number "X", or "ttySX".
It even tries the sparc-specific transformations in
serial_base_add_sparc_console()

But this is the original format which does _not_ include ":".
It never will be stored in c->devname and will never match.

I think that it has been the case even before this patchset.

I think that we should remove these layers and check just
the "DEVNAME:X.Y" format, aka "%s:%d.%d" [*].


[*] It would be nice to use the same printf format "%s:%d.%d"
    in both serial_base_device_init() and also in the functions
    matching the devname to make it clear that these are
    the same names. Heh, I just guess that these are the same
    names.

Best Regards,
Petr