Re: [PATCH v2] serial: fix ioctl hangup race
From: sashiko-bot@kernel.org
Date: 2026-09-04 11:55:43
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: New issues: - [Medium] Mixing goto-based error handling with scope-based cleanup (scoped_guard) in the same function. Pre-existing issues: - [High] Missing hangup check (tty_io_error) inside the TIOCMIWAIT wait loop causes indefinite hangs on device removal. - [High] Task state is set to TASK_INTERRUPTIBLE after evaluating the wait condition, causing potential missed wakeups. -- commit 971660f9118e5ac27dcc58fb0e95856b75c73a98 Author: Johan Hovold [off-list ref] serial: fix ioctl hangup race This patch adds missing checks to make sure the tty port has not been hung up before accessing the hardware in several serial ioctls. This helps avoid issues like kernel panics due to unclocked accesses on devices that are already gone or powered down.
quoted hunk ↗ jump to hunk
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c index 95774b0f1484b..b0d3902fe4fca 100644 --- a/drivers/tty/serial/serial_core.c +++ b/drivers/tty/serial/serial_core.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -1213,9 +1213,17 @@ static int uart_wait_modem_status(struct uart_state *state, unsigned long arg) uport = uart_port_ref(state); if (!uport) return -EIO; - scoped_guard(uart_port_lock_irq, uport) { - memcpy(&cprev, &uport->icount, sizeof(struct uart_icount)); - uart_enable_ms(uport); + + scoped_guard(mutex, &port->mutex) { + if (tty_io_error(tty)) { + ret = -EIO; + goto out_deref; + }
[Severity: Medium] Does this mix goto-based error handling with scope-based cleanup blocks? According to the kernel cleanup subsystem guidelines in include/linux/cleanup.h, mixing goto-based cleanup targets (like out_deref below) with scoped_guard() in the same function can lead to confusing ownership semantics. Should this function fully convert its resources to scope-based cleanup, or stick entirely to manual locking? [ ... ]
add_wait_queue(&port->delta_msr_wait, &wait);
for (;;) {
scoped_guard(uart_port_lock_irq, uport)
memcpy(&cnow, &uport->icount, sizeof(struct uart_icount));[Severity: High] This is a pre-existing issue, but does this loop lack a hangup check (tty_io_error(tty)) upon waking? If a userspace process calls TIOCMIWAIT and blocks in schedule(), and the device is hot-unplugged or hung up, tty_port_hangup() wakes the queue. If the process ignores SIGHUP, signal_pending() is false. Without a check for device removal here, could the task resume, observe no changes to the hardware counters, and go back to sleep indefinitely?
set_current_state(TASK_INTERRUPTIBLE);
[Severity: High] This is also a pre-existing issue, but does setting the task state to TASK_INTERRUPTIBLE after reading the wait conditions create a race window? If a modem status interrupt fires immediately after the lock is released (when memcpy finishes) but before set_current_state() is called, the interrupt handler will wake the queue and set the task to TASK_RUNNING. Could the task then incorrectly overwrite its state to TASK_INTERRUPTIBLE, see the stale counters, and call schedule(), potentially losing the wakeup? [ ... ]
} __set_current_state(TASK_RUNNING); remove_wait_queue(&port->delta_msr_wait, &wait); +out_deref: uart_port_deref(uport); return ret;
-- Sashiko AI review · https://sashiko.dev/#/patchset/20260904114448.1664502-1-johan@kernel.org?part=1