[PATCH 0/2] serial: core: two fixes for the port removal path

[PATCH 0/2] serial: core: two fixes for the port removal path

[Tapio Reijonen]

Two fixes in serial_core_remove_one_port() and its reference counting.

Patch 1 fixes a deadlock between port removal and a tcdrain() in
flight, present since 8ca59f7baee7 ("serial: fix ioctl hangup race").
Patch 2 fixes a read of the port after its last reference was dropped,
found by review of the same path and not reproduced.

Reproduced on a MAX14830 (max310x) board, on v7.4-rc1 plus the board
support, by unbinding the driver while four processes drained a few
kilobytes each at 300 baud: the first unbind never returned, and the
hung-task detector reported the unbind in wait_event() and a drain in
mutex_lock(), with the drain "blocked on a mutex likely owned by" the
unbind; a power cycle was needed. With the two patches the same test
completed 46 of 46 unbinds with blocked tcdrain() waiters, each unbind
taking 2 to 3.5 s, the time for the drains to be hung up. A two-hour
soak with lockdep and kmemleak enabled, 1472 unbind and rebind cycles
every 3 s while other ports of the same driver carried full traffic,
completed with nothing reported. The change is independent of the
driver: it only moves the wait for the reference holders out from under
the mutex those holders need.

Tool use: Claude Code (Anthropic) was used throughout. It analysed the
hung-task reports and the /proc stacks from the stress bench, located
the lock order between serial_core_remove_one_port() and
uart_wait_until_sent(), drafted this change and the commit message, and
wrote and drove the test harness that reproduced the deadlock and
verified the fix. I reviewed the locking argument against the code,
decided the shape of the change, ran the hardware tests and read every
result. The code and the message are mine to defend.

8ca59f7baee7 is in v7.3 and is tagged for stable back to 2.6.26, so
patch 1 has to follow it into every tree that takes it. 7.3.y has the
same guard() form and takes patch 1 with a three-way merge. 6.18.y and
older still end serial_core_remove_one_port() with mutex_lock()/
mutex_unlock() and need the change written with explicit calls, unlock
before the wait_event(), lock again around the NULL store; I will send
that variant when the stable mails arrive. Patch 2 applies to 6.18.y
and 6.12.y and needs a context refresh for 6.6.y and older.

---
Tapio Reijonen (2):
      serial: core: do not hold the port mutex while waiting for references
      serial: core: do not touch the port after dropping the last reference

 drivers/tty/serial/serial_core.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)
---
base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
change-id: 20261103-serial-core-remove-deadlock-0310bbca5554

Best regards,
-- 
Tapio Reijonen <tapio.reijonen@vaisala.com>

[PATCH 1/2] serial: core: do not hold the port mutex while waiting for references

[Tapio Reijonen]

serial_core_remove_one_port() takes the port mutex and then waits for
the uart_port_ref() holders to drop their references. Since commit
8ca59f7baee7 ("serial: fix ioctl hangup race") uart_wait_until_sent()
takes the port mutex on every poll iteration to check for the hangup
before it touches the hardware, while holding its reference for the
whole wait. A tcdrain() in flight when the port is removed therefore
deadlocks: the removal holds the mutex and waits for the reference,
the drain holds the reference and waits for the mutex. Both tasks sit
uninterruptible, and the port is unusable until a reboot.

Reproduced by unbinding a UART driver while a process drains a few
kilobytes at 300 baud: the unbind blocks in wait_event() under the
mutex, the drain in mutex_lock(), and reading the driver's proc file
blocks on the same mutex.

Drop the mutex around the decrement and the wait. The decrement is an
atomic paired with uart_port_ref() and uart_port_deref(), which never
take the mutex; the clearing of the port pointer stays under it, where
uart_port_check() readers expect it. Between the hangup and this wait
the removal already runs without the mutex, so a mutex holder seeing
the released port is not a new state.
uart_wait_modem_status() takes a reference and then the mutex in the
same way, with a smaller window, and is covered by the same change.

Stable trees before the guard() conversion of this function need the
same change with explicit calls: mutex_unlock() before the
wait_event(), mutex_lock() around the store of the port pointer.
7.3.y has the guard() form and takes the patch with a three-way
merge.

Fixes: 8ca59f7baee7 ("serial: fix ioctl hangup race")
Cc: <stable@vger.kernel.org> # 6.18.y and older need the open-coded form, see below
Signed-off-by: Tapio Reijonen <tapio.reijonen@vaisala.com>
---
 drivers/tty/serial/serial_core.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index c0deec0e6af41db10de15ccca4169c603d9cebf6..2e53d194ecfe853ade1ac6f33e047eb8ff11e283 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -3266,10 +3266,17 @@ static void serial_core_remove_one_port(struct uart_driver *drv,
 	uport->type = PORT_UNKNOWN;
 	uport->port_dev = NULL;
 
-	guard(mutex)(&port->mutex);
 	WARN_ON(atomic_dec_return(&state->refcount) < 0);
+
+	/*
+	 * A reference holder may need the port mutex to see the hangup
+	 * (uart_wait_until_sent() does), so wait for the references
+	 * without holding it.
+	 */
 	wait_event(state->remove_wait, !atomic_read(&state->refcount));
-	state->uart_port = NULL;
+
+	scoped_guard(mutex, &port->mutex)
+		state->uart_port = NULL;
 }
 
 static struct serial_ctrl_device *

-- 
2.47.3

[PATCH 2/2] serial: core: do not touch the port after dropping the last reference

[Tapio Reijonen]

uart_port_deref() decrements the reference count through uport->state
and then reads uport->state again for the wake_up(). The last
decrement is what lets serial_core_remove_one_port() return, and the
driver may free its struct uart_port right after that. A reference
holder that drops the last reference can therefore read the freed port
on its way to the wake_up().

Read the state pointer once, before the decrement. The uart_state
belongs to the uart_driver and lives until uart_unregister_driver(),
so the wake_up() target outlives the port. Drivers that unregister the
uart_driver in the same remove() are a separate matter and not changed
here.

Found by review of the removal path while fixing the deadlock between
port removal and a blocked tcdrain(); not reproduced.

Fixes: 9ed19428a51d ("serial: core: Prevent unsafe uart port access, part 2")
Cc: stable@vger.kernel.org
Signed-off-by: Tapio Reijonen <tapio.reijonen@vaisala.com>
---
 drivers/tty/serial/serial_core.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 2e53d194ecfe853ade1ac6f33e047eb8ff11e283..c3d21acbd606eb3ea2909736a01a66f204141be1 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -72,8 +72,10 @@ static inline struct uart_port *uart_port_ref(struct uart_state *state)
 
 static inline void uart_port_deref(struct uart_port *uport)
 {
-	if (atomic_dec_and_test(&uport->state->refcount))
-		wake_up(&uport->state->remove_wait);
+	struct uart_state *state = uport->state;
+
+	if (atomic_dec_and_test(&state->refcount))
+		wake_up(&state->remove_wait);
 }
 
 static inline struct uart_port *uart_port_ref_lock(struct uart_state *state, unsigned long *flags)

-- 
2.47.3

Re: [PATCH 2/2] serial: core: do not touch the port after dropping the last reference

[sashiko-bot]

> uart_port_deref() decrements the reference count through uport->state
> and then reads uport->state again for the wake_up(). The last
> decrement is what lets serial_core_remove_one_port() return, and the
> driver may free its struct uart_port right after that. A reference
> holder that drops the last reference can therefore read the freed port
> on its way to the wake_up().
> 
> Read the state pointer once, before the decrement. The uart_state
> belongs to the uart_driver and lives until uart_unregister_driver(),
> so the wake_up() target outlives the port. Drivers that unregister the
> uart_driver in the same remove() are a separate matter and not changed
> here.
> 
> Found by review of the removal path while fixing the deadlock between
> port removal and a blocked tcdrain(); not reproduced.
> 
> Fixes: 9ed19428a51d ("serial: core: Prevent unsafe uart port access, part 2")
> Cc: stable@vger.kernel.org
> Signed-off-by: Tapio Reijonen <tapio.reijonen@vaisala.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-serial-core-remove-deadlock-v1-0-6a30597adcbf@vaisala.com?part=2

Re: [PATCH 1/2] serial: core: do not hold the port mutex while waiting for references

[sashiko-bot]

> serial_core_remove_one_port() takes the port mutex and then waits for
> the uart_port_ref() holders to drop their references. Since commit
> 8ca59f7baee7 ("serial: fix ioctl hangup race") uart_wait_until_sent()
> takes the port mutex on every poll iteration to check for the hangup
> before it touches the hardware, while holding its reference for the
> whole wait. A tcdrain() in flight when the port is removed therefore
> deadlocks: the removal holds the mutex and waits for the reference,
> the drain holds the reference and waits for the mutex. Both tasks sit
> uninterruptible, and the port is unusable until a reboot.
> 
> Reproduced by unbinding a UART driver while a process drains a few
> kilobytes at 300 baud: the unbind blocks in wait_event() under the
> mutex, the drain in mutex_lock(), and reading the driver's proc file
> blocks on the same mutex.
> 
> [ ... ]
> merge.
> 
> Fixes: 8ca59f7baee7 ("serial: fix ioctl hangup race")
> Cc: <stable@vger.kernel.org> # 6.18.y and older need the open-coded form, see below
> Signed-off-by: Tapio Reijonen <tapio.reijonen@vaisala.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-serial-core-remove-deadlock-v1-0-6a30597adcbf@vaisala.com?part=1