Re: [PATCH v2 1/6] serial: 8250: drop err label from serial8250_register_8250_port()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] tty: serial: 8250: missing cancel_delayed_work_sync() for overrun_backoff
- [High] tty: serial: 8250: workqueue list corruption via INIT_DELAYED_WORK()
--

commit c4166af59c093eb0e0aed408aac8356ad1cf88f2
Author: Jiri Slaby (SUSE) <jirislaby@kernel.org>

serial: 8250: drop err label from serial8250_register_8250_port()

This commit refactors serial8250_register_8250_port() to eliminate
goto-based error handling while holding a guard() lock. The port
configuration logic is moved into a new helper,
__serial8250_register_8250_port(), which returns an int and uses plain
returns on failures. The caller then handles resetting uart->port.dev in a
single place.

> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> index 11424fef7c08b..7ff307ce6aa88 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -743,46 +743,12 @@ static void serial_8250_overrun_backoff_work(struct work_struct *work)
>  	serial_out(up, UART_IER, up->ier);
>  }

[Severity: High]
Can this background work access hardware registers after the port is
unregistered?

When a port is removed (such as during hot-unplug or driver unbind), it
appears there is no matching cancel_delayed_work_sync() for the
overrun_backoff work. 

If the device memory is unmapped and the delayed work executes, will this
serial_out() trigger a synchronous external abort or use-after-free?

[ ... ]

> @@ -927,9 +891,51 @@ int serial8250_register_8250_port(const struct uart_8250_port *up)
>  	}
>  
>  	return ret;
> +}

[Severity: High]
Does this new helper unconditionally re-initialize the delayed work without
checking if it is already pending?

If a device is unbound and then rebound while the overrun_backoff work is
still pending, __serial8250_register_8250_port() is called again. Towards the
end of the helper, the work is initialized:

	if (up->overrun_backoff_time_ms > 0) {
		...
		INIT_DELAYED_WORK(&uart->overrun_backoff,
				serial_8250_overrun_backoff_work);
	}

Re-initializing an active delayed work item with INIT_DELAYED_WORK() calls
INIT_LIST_HEAD() on its list nodes.

Can this corrupt the timer or workqueue linked lists if the work is still
active from a previous registration?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1791873876.git.jirislaby@kernel.org?part=1