serial: core: Clearing the circular buffer before NULLifying it

PATCH In review Andy Shevchenko 4 participants 9 messages Apr 4, 2024 – Apr 8, 2024 TTY layer and serial drivers

Share & tools

Andy Shevchenko

author

2024-04-04 15:00 UTC #0 lore raw

PATCH 1/1 serial: core: Clearing the circular buffer before NULLifying it

The circular buffer is NULLified in uart_tty_port_shutdown() under the spin lock. However, the PM or other timer based callbacks may still trigger after this event without knowning that buffer pointer is not valid. Since the serial code is a bit inconsistent in checking the buffer state (some rely on the head-tail positions, some on the buffer pointer), it's better to have both aligned, i.e. buffer pointer to be NULL and head-tail possitions to be the same, meaning it's empty. This will prevent asynchronous calls to dereference NULL pointer as reported recently in 8250 case:

  BUG: kernel NULL pointer dereference, address: 00000cf5
  Workqueue: pm pm_runtime_work
  EIP: serial8250_tx_chars (drivers/tty/serial/8250/8250_port.c:1809)
  ...
  ? serial8250_tx_chars (drivers/tty/serial/8250/8250_port.c:1809)
  __start_tx (drivers/tty/serial/8250/8250_port.c:1551)
  serial8250_start_tx (drivers/tty/serial/8250/8250_port.c:1654)
  serial_port_runtime_suspend (include/linux/serial_core.h:667 drivers/tty/serial/serial_port.c:63)
  __rpm_callback (drivers/base/power/runtime.c:393)
  ? serial_port_remove (drivers/tty/serial/serial_port.c:50)
  rpm_suspend (drivers/base/power/runtime.c:447)

The proposed change will prevent ->start_tx() to be called during suspend on shut down port.

Fixes 43066e32227e ("serial: port: Don't suspend if the port is still busy")

Reported-by kernel test robot <[email protected]>

Closes https://lore.kernel.org/oe-lkp/[email protected] open here

Signed-off-by Andy Shevchenko <[email protected]>

---

I have got into the very similar issue while working on max3100 driver. I haven't checked the 8250 case, but for mine the culprit is the same and this patch fixes it. Hence I assume it will fix the 8250 case as well.

 drivers/tty/serial/serial_core.c | 1 +
 1 file changed, 1 insertion(+)

drivers/tty/serial/serial_core.c +1 -0

index a005fc06a077..ba3a674a8bbf 100644
@@ -1788,6 +1788,7 @@ static void uart_tty_port_shutdown(struct tty_port *port)
 	 * Free the transmit buffer.
 	 */
 	uart_port_lock_irq(uport);
+	uart_circ_clear(&state->xmit);
 	buf = state->xmit.buf;
 	state->xmit.buf = NULL;
 	uart_port_unlock_irq(uport);

Signature · 1 line

2.43.0.rc1.1.gbec44491f096

Jiri Slaby

maintainer

2024-04-05 05:25 UTC #1 lore raw

Quoted 10 lines · Andy Shevchenko

The circular buffer is NULLified in uart_tty_port_shutdown() under the spin lock. However, the PM or other timer based callbacks may still trigger after this event without knowning that buffer pointer is not valid. Since the serial code is a bit inconsistent in checking the buffer state (some rely on the head-tail positions, some on the buffer pointer), it's better to have both aligned, i.e. buffer pointer to be NULL and head-tail possitions to be the same, meaning it's empty. This will prevent asynchronous calls to dereference NULL pointer as reported recently in 8250 case:

Quoted 11 lines

   BUG: kernel NULL pointer dereference, address: 00000cf5
   Workqueue: pm pm_runtime_work
   EIP: serial8250_tx_chars (drivers/tty/serial/8250/8250_port.c:1809)
   ...
   ? serial8250_tx_chars (drivers/tty/serial/8250/8250_port.c:1809)
   __start_tx (drivers/tty/serial/8250/8250_port.c:1551)
   serial8250_start_tx (drivers/tty/serial/8250/8250_port.c:1654)
   serial_port_runtime_suspend (include/linux/serial_core.h:667 drivers/tty/serial/serial_port.c:63)
   __rpm_callback (drivers/base/power/runtime.c:393)
   ? serial_port_remove (drivers/tty/serial/serial_port.c:50)
   rpm_suspend (drivers/base/power/runtime.c:447)

Yeah, I noticed start_tx() is called repeatedly after shutdown() yesterday too. So thanks for looking into this.

And it's pretty weird. I think it's new with the runtime PM (sure, /me reads Fixes: now). I am not sure if it is documented, but most of the code in tty/ assumes NO ordinary ->ops (like start_tx()) are called after shutdown(). Actually, to me it occurs like serial8250_start_tx() should not be called in the first place. It makes no sense after all.

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().

Quoted 29 lines

The proposed change will prevent ->start_tx() to be called during suspend on shut down port.
Fixes 43066e32227e ("serial: port: Don't suspend if the port is still busy")
Reported-by kernel test robot <[email protected]>
Closes https://lore.kernel.org/oe-lkp/[email protected] open here
Signed-off-by Andy Shevchenko <[email protected]>
---
I have got into the very similar issue while working on max3100 driver. I haven't checked the 8250 case, but for mine the culprit is the same and this patch fixes it. Hence I assume it will fix the 8250 case as well.
  drivers/tty/serial/serial_core.c | 1 +
  1 file changed, 1 insertion(+)
drivers/tty/serial/serial_core.c +1 -0
index a005fc06a077..ba3a674a8bbf 100644
@@ -1788,6 +1788,7 @@ static void uart_tty_port_shutdown(struct tty_port *port)
  	 * Free the transmit buffer.
  	 */
  	uart_port_lock_irq(uport);
+	uart_circ_clear(&state->xmit);
  	buf = state->xmit.buf;
  	state->xmit.buf = NULL;
  	uart_port_unlock_irq(uport);

Signature · 2 lines

js
suse labs

Tony Lindgren

2024-04-05 05:42 UTC #2 lore raw

Quoted 1 line · Jiri Slaby

But again, serial8250_start_tx() should not be called after shutdown().

Sounds like we should add some check in addition to UPF_DEAD to serial_port_runtime_suspend() and serial_port_runtime_resume() to bail out early on shutdown.

Regards,

Tony

Andy Shevchenko

author

2024-04-05 15:19 UTC #3 lore raw

In reply to Jiri Slaby

Quoted 26 lines · Jiri Slaby

Quoted 10 lines · Andy Shevchenko
The circular buffer is NULLified in uart_tty_port_shutdown() under the spin lock. However, the PM or other timer based callbacks may still trigger after this event without knowning that buffer pointer is not valid. Since the serial code is a bit inconsistent in checking the buffer state (some rely on the head-tail positions, some on the buffer pointer), it's better to have both aligned, i.e. buffer pointer to be NULL and head-tail possitions to be the same, meaning it's empty. This will prevent asynchronous calls to dereference NULL pointer as reported recently in 8250 case:
Quoted 11 lines
   BUG: kernel NULL pointer dereference, address: 00000cf5
   Workqueue: pm pm_runtime_work
   EIP: serial8250_tx_chars (drivers/tty/serial/8250/8250_port.c:1809)
   ...
   ? serial8250_tx_chars (drivers/tty/serial/8250/8250_port.c:1809)
   __start_tx (drivers/tty/serial/8250/8250_port.c:1551)
   serial8250_start_tx (drivers/tty/serial/8250/8250_port.c:1654)
   serial_port_runtime_suspend (include/linux/serial_core.h:667 drivers/tty/serial/serial_port.c:63)
   __rpm_callback (drivers/base/power/runtime.c:393)
   ? serial_port_remove (drivers/tty/serial/serial_port.c:50)
   rpm_suspend (drivers/base/power/runtime.c:447)
Yeah, I noticed start_tx() is called repeatedly after shutdown() yesterday too. So thanks for looking into this.

Quoted 8 lines

And it's pretty weird. I think it's new with the runtime PM (sure, /me reads
Fixes: now). I am not sure if it is documented, but most of the code in tty/ assumes NO ordinary ->ops (like start_tx()) are called after shutdown(). Actually, to me it occurs like serial8250_start_tx() should not be called in the first place. It makes no sense after all.
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.

But do you agree that this patch has value on its own?

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andy Shevchenko

author

2024-04-05 22:37 UTC #4 lore raw

Quoted 33 lines · Andy Shevchenko

Quoted 32 lines · Jiri Slaby
Quoted 10 lines · Andy Shevchenko
The circular buffer is NULLified in uart_tty_port_shutdown() under the spin lock. However, the PM or other timer based callbacks may still trigger after this event without knowning that buffer pointer is not valid. Since the serial code is a bit inconsistent in checking the buffer state (some rely on the head-tail positions, some on the buffer pointer), it's better to have both aligned, i.e. buffer pointer to be NULL and head-tail possitions to be the same, meaning it's empty. This will prevent asynchronous calls to dereference NULL pointer as reported recently in 8250 case:
Quoted 11 lines
   BUG: kernel NULL pointer dereference, address: 00000cf5
   Workqueue: pm pm_runtime_work
   EIP: serial8250_tx_chars (drivers/tty/serial/8250/8250_port.c:1809)
   ...
   ? serial8250_tx_chars (drivers/tty/serial/8250/8250_port.c:1809)
   __start_tx (drivers/tty/serial/8250/8250_port.c:1551)
   serial8250_start_tx (drivers/tty/serial/8250/8250_port.c:1654)
   serial_port_runtime_suspend (include/linux/serial_core.h:667 drivers/tty/serial/serial_port.c:63)
   __rpm_callback (drivers/base/power/runtime.c:393)
   ? serial_port_remove (drivers/tty/serial/serial_port.c:50)
   rpm_suspend (drivers/base/power/runtime.c:447)
Yeah, I noticed start_tx() is called repeatedly after shutdown() yesterday too. So thanks for looking into this.
And it's pretty weird. I think it's new with the runtime PM (sure, /me reads
Fixes: now). I am not sure if it is documented, but most of the code in tty/ assumes NO ordinary ->ops (like start_tx()) are called after shutdown(). Actually, to me it occurs like serial8250_start_tx() should not be called in the first place. It makes no sense after all.

Quoted 8 lines

Quoted 2 lines
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.
But do you agree that this patch has value on its own?

FWIW, https://lore.kernel.org/all/[email protected]/T/

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Tony Lindgren

2024-04-06 05:46 UTC #5 lore raw

Quoted 8 lines · Andy Shevchenko

Quoted 7 lines · Andy Shevchenko
Quoted 2 lines · Jiri Slaby
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?

Quoted 3 lines

Quoted 1 line
But do you agree that this patch has value on its own?
FWIW, https://lore.kernel.org/all/[email protected]/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< -----------------------

drivers/tty/serial/serial_core.c +14 -3

@@ -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);
 
 	/*

Andy Shevchenko

author

2024-04-08 15:45 UTC #6 lore raw

In reply to Jiri Slaby

Quoted 23 lines · Jiri Slaby

Quoted 10 lines · Andy Shevchenko
The circular buffer is NULLified in uart_tty_port_shutdown() under the spin lock. However, the PM or other timer based callbacks may still trigger after this event without knowning that buffer pointer is not valid. Since the serial code is a bit inconsistent in checking the buffer state (some rely on the head-tail positions, some on the buffer pointer), it's better to have both aligned, i.e. buffer pointer to be NULL and head-tail possitions to be the same, meaning it's empty. This will prevent asynchronous calls to dereference NULL pointer as reported recently in 8250 case:
Quoted 11 lines
   BUG: kernel NULL pointer dereference, address: 00000cf5
   Workqueue: pm pm_runtime_work
   EIP: serial8250_tx_chars (drivers/tty/serial/8250/8250_port.c:1809)
   ...
   ? serial8250_tx_chars (drivers/tty/serial/8250/8250_port.c:1809)
   __start_tx (drivers/tty/serial/8250/8250_port.c:1551)
   serial8250_start_tx (drivers/tty/serial/8250/8250_port.c:1654)
   serial_port_runtime_suspend (include/linux/serial_core.h:667 drivers/tty/serial/serial_port.c:63)
   __rpm_callback (drivers/base/power/runtime.c:393)
   ? serial_port_remove (drivers/tty/serial/serial_port.c:50)
   rpm_suspend (drivers/base/power/runtime.c:447)

Quoted 8 lines

Yeah, I noticed start_tx() is called repeatedly after shutdown() yesterday too. So thanks for looking into this.
And it's pretty weird. I think it's new with the runtime PM (sure, /me reads
Fixes: now). I am not sure if it is documented, but most of the code in tty/ assumes NO ordinary ->ops (like start_tx()) are called after shutdown(). Actually, to me it occurs like serial8250_start_tx() should not be called in the first place. It makes no sense after all.

So, with PM autosuspend the [PM] callback can be called in to cases:
- port is open and busy, but PM is not informed (yet) of it
- port is closed while PM timer is still counting

The Fixes seems about the first case (so we need to call Tx there). The second one probably can be fixed properly with PM barrier.

This fix is just against the oops AFAIU the bug report and my own case.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Yicong Yang

2024-04-07 09:49 UTC #7 lore raw

In reply to Andy Shevchenko

Hi Andy,

Quoted 22 lines · Andy Shevchenko

The circular buffer is NULLified in uart_tty_port_shutdown() under the spin lock. However, the PM or other timer based callbacks may still trigger after this event without knowning that buffer pointer is not valid. Since the serial code is a bit inconsistent in checking the buffer state (some rely on the head-tail positions, some on the buffer pointer), it's better to have both aligned, i.e. buffer pointer to be NULL and head-tail possitions to be the same, meaning it's empty. This will prevent asynchronous calls to dereference NULL pointer as reported recently in 8250 case:
  BUG: kernel NULL pointer dereference, address: 00000cf5
  Workqueue: pm pm_runtime_work
  EIP: serial8250_tx_chars (drivers/tty/serial/8250/8250_port.c:1809)
  ...
  ? serial8250_tx_chars (drivers/tty/serial/8250/8250_port.c:1809)
  __start_tx (drivers/tty/serial/8250/8250_port.c:1551)
  serial8250_start_tx (drivers/tty/serial/8250/8250_port.c:1654)
  serial_port_runtime_suspend (include/linux/serial_core.h:667 drivers/tty/serial/serial_port.c:63)
  __rpm_callback (drivers/base/power/runtime.c:393)
  ? serial_port_remove (drivers/tty/serial/serial_port.c:50)
  rpm_suspend (drivers/base/power/runtime.c:447)

Quoted 3 lines

The proposed change will prevent ->start_tx() to be called during suspend on shut down port.

Just saw the issue and thanks for your timely fix. I didn't got a board with 8250 and sorry for didn't found this issue.

FYI, I checked device_shutdown() and seems it called pm_runtime_barrier() for waiting all the scheduled RPM callbacks finished and keep the device in resume state. So ideally there shouldn't be any pending requests later since we handled them before shutdown?

There's someone encountered the same issue in shutdown() due to runtime pm and fixed it in
af8db1508f2c ("PM / driver core: disable device's runtime PM during shutdown")
patch above seems to still have some problem and later fixed by:
fe6b91f47080 ("PM / Driver core: leave runtime PM enabled during system shutdown")

But seems the handling in the driver core doesn't cover the case here..

Quoted 27 lines

Fixes 43066e32227e ("serial: port: Don't suspend if the port is still busy")
Reported-by kernel test robot <[email protected]>
Closes https://lore.kernel.org/oe-lkp/[email protected] open here
Signed-off-by Andy Shevchenko <[email protected]>
---
I have got into the very similar issue while working on max3100 driver. I haven't checked the 8250 case, but for mine the culprit is the same and this patch fixes it. Hence I assume it will fix the 8250 case as well.
 drivers/tty/serial/serial_core.c | 1 +
 1 file changed, 1 insertion(+)
drivers/tty/serial/serial_core.c +1 -0
index a005fc06a077..ba3a674a8bbf 100644
@@ -1788,6 +1788,7 @@ static void uart_tty_port_shutdown(struct tty_port *port)
 	 * Free the transmit buffer.
 	 */
 	uart_port_lock_irq(uport);
+	uart_circ_clear(&state->xmit);
 	buf = state->xmit.buf;
 	state->xmit.buf = NULL;
 	uart_port_unlock_irq(uport);

Andy Shevchenko

author

2024-04-08 14:36 UTC #8 lore raw

Quoted 23 lines · Yicong Yang

Quoted 22 lines · Andy Shevchenko
The circular buffer is NULLified in uart_tty_port_shutdown() under the spin lock. However, the PM or other timer based callbacks may still trigger after this event without knowning that buffer pointer is not valid. Since the serial code is a bit inconsistent in checking the buffer state (some rely on the head-tail positions, some on the buffer pointer), it's better to have both aligned, i.e. buffer pointer to be NULL and head-tail possitions to be the same, meaning it's empty. This will prevent asynchronous calls to dereference NULL pointer as reported recently in 8250 case:
  BUG: kernel NULL pointer dereference, address: 00000cf5
  Workqueue: pm pm_runtime_work
  EIP: serial8250_tx_chars (drivers/tty/serial/8250/8250_port.c:1809)
  ...
  ? serial8250_tx_chars (drivers/tty/serial/8250/8250_port.c:1809)
  __start_tx (drivers/tty/serial/8250/8250_port.c:1551)
  serial8250_start_tx (drivers/tty/serial/8250/8250_port.c:1654)
  serial_port_runtime_suspend (include/linux/serial_core.h:667 drivers/tty/serial/serial_port.c:63)
  __rpm_callback (drivers/base/power/runtime.c:393)
  ? serial_port_remove (drivers/tty/serial/serial_port.c:50)
  rpm_suspend (drivers/base/power/runtime.c:447)

Quoted 5 lines

Quoted 2 lines
The proposed change will prevent ->start_tx() to be called during suspend on shut down port.
Just saw the issue and thanks for your timely fix. I didn't got a board with 8250 and sorry for didn't found this issue.

But does this change make no regression in your case? Can you test it?

Quoted 8 lines

FYI, I checked device_shutdown() and seems it called pm_runtime_barrier() for waiting all the scheduled RPM callbacks finished and keep the device in resume state. So ideally there shouldn't be any pending requests later since we handled them before shutdown?
There's someone encountered the same issue in shutdown() due to runtime pm and fixed it in
af8db1508f2c ("PM / driver core: disable device's runtime PM during shutdown")
patch above seems to still have some problem and later fixed by:
fe6b91f47080 ("PM / Driver core: leave runtime PM enabled during system shutdown")

Ah, yes, thanks for reminding (yeah, I saw those patches, let me test it on my setup.

Quoted 1 line

But seems the handling in the driver core doesn't cover the case here..

Of course, since we have our own shutdown on the upper level, we don't kill the device, but we do release _some_ resources.

Signature · 2 lines

With Best Regards,
Andy Shevchenko