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. Fixes: 8ca59f7baee7 ("serial: fix ioctl hangup race") Cc: Reviewed-by: Johan Hovold Tested-by: Johan Hovold Signed-off-by: Tapio Reijonen --- 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 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 Reviewed-by: Johan Hovold Signed-off-by: Tapio Reijonen --- 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