linux-kernel - Re: [PATCH v1 1/1] serial: core: Clearing the circular buffer before NULLifying it

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: Sat, 6 Apr 2024 08:46:17 +0300
From: Tony Lindgren <tony@...mide.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: Jiri Slaby <jirislaby@...nel.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org,
	Yicong Yang <yangyicong@...ilicon.com>,
	kernel test robot <oliver.sang@...el.com>
Subject: Re: [PATCH v1 1/1] serial: core: Clearing the circular buffer before
 NULLifying it

* Andy Shevchenko <andriy.shevchenko@...ux.intel.com> [240405 22:37]:
> On Fri, Apr 05, 2024 at 06:17:54PM +0300, Andy Shevchenko wrote:
> > On Fri, Apr 05, 2024 at 07:25:03AM +0200, Jiri Slaby wrote:
> > > BTW cannot be x_char en/queued at that time too (the other check in the if)?
> > > But again, serial8250_start_tx() should not be called after shutdown().
> > 
> > Yes, and I have no clue how we can check this as startup can be called again
> > and so on. The PM callback is timer based AFAIU, meaning it may happen at any
> > time.

So below is an incomplete pseudo patch just showing where we could disable
tx for runtime PM.

The patch won't compile, and assumes we only disable tx for runtime PM.

However, if we need it elsewhere also, then we may want to set up some
UPF_TX_ENABLED type flag instead of serial_base_port specific calls.

My preference would be to limit it to serial_port.c if we can get away
with that.

Anybody have better ideas for enabling and disabling tx?

> > But do you agree that this patch has value on its own?
> 
> FWIW, https://lore.kernel.org/all/0000000000009e2dd805ffc595a3@google.com/T/

No objections from me for clearing the xmit. But should it also be done for
uart_shutdown() in addition to uart_tty_port_shutdown()?

Regards,

Tony

8< -----------------------
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -345,16 +345,23 @@ static int uart_startup(struct tty_struct *tty, struct uart_state *state,
 			bool init_hw)
 {
 	struct tty_port *port = &state->port;
+	struct uart_port *uport;
 	int retval;
 
 	if (tty_port_initialized(port))
-		return 0;
+		goto enable_tx;
 
 	retval = uart_port_startup(tty, state, init_hw);
-	if (retval)
+	if (retval) {
 		set_bit(TTY_IO_ERROR, &tty->flags);
+		return retval;
+	}
 
-	return retval;
+enable_tx:
+	uport = uart_port_check(state);
+	serial_base_port_enable_tx(uport);
+
+	return 0;
 }
 
 /*
@@ -377,6 +384,9 @@ static void uart_shutdown(struct tty_struct *tty, struct uart_state *state)
 	if (tty)
 		set_bit(TTY_IO_ERROR, &tty->flags);
 
+	if (uport)
+		serial_base_port_disable_tx(uport);
+
 	if (tty_port_initialized(port)) {
 		tty_port_set_initialized(port, false);
 
@@ -1821,6 +1831,7 @@ static void uart_tty_port_shutdown(struct tty_port *port)
 	uport->ops->stop_rx(uport);
 	uart_port_unlock_irq(uport);
 
+	serial_base_port_disable_tx(uport);
 	uart_port_shutdown(port);
 
 	/*