Re: [PATCH v2] serial: fix ioctl hangup race

[sashiko-bot@kernel.org]

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 <johan@kernel.org>
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.

> 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
[ ... ]
> @@ -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