Re: [PATCH 05/24] serial: 8250: clear subdriver state on port unregister

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] serial: 8250: clear rxtrig, break_ctl, attr_group, rs485 gpios on unregister
--

commit 84854ce6768bb6d9f299a8c1baf6b88228a5ea32
Author: Joshua Rogers <linux@joshua.hu>

serial: 8250: clear subdriver state on port unregister

This commit clears subdriver-copied callbacks and per-driver fields on
struct uart_8250_port and calls serial8250_set_defaults() in
serial8250_unregister_port() before re-registering the port as a fallback
ISA port.

> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> index 0951d3e42e1f3..673fde55828a9 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -956,7 +956,35 @@ void serial8250_unregister_port(struct uart_8250_port *uart)
>  		uart->port.dev = &serial8250_isa_devs->dev;
>  		uart->port.port_id = uart->port.line;
>  		uart->capabilities = 0;
> +
> +		/*
> +		 * Clear subdriver-owned state copied by
> +		 * serial8250_register_8250_port() to prevent dangling
> +		 * pointers after the original driver module is unloaded.
> +		 */
> +		uart->port.private_data = NULL;
> +		uart->port.set_termios = NULL;
> +		uart->port.set_ldisc = NULL;
> +		uart->port.get_mctrl = NULL;
> +		uart->port.set_mctrl = NULL;
> +		uart->port.get_divisor = NULL;
> +		uart->port.set_divisor = NULL;
> +		uart->port.startup = NULL;
> +		uart->port.shutdown = NULL;
> +		uart->port.throttle = NULL;
> +		uart->port.unthrottle = NULL;
> +		uart->port.handle_break = NULL;
> +		uart->port.rs485_config = NULL;
> +		memset(&uart->port.rs485, 0, sizeof(uart->port.rs485));
> +		memset(&uart->port.rs485_supported, 0,
> +		       sizeof(uart->port.rs485_supported));
> +		uart->gpios = NULL;
> +		uart->dma = NULL;
> +		uart->rs485_start_tx = NULL;
> +		uart->rs485_stop_tx = NULL;

[Severity: High]
Should serial8250_unregister_port() also clear get_rxtrig, set_rxtrig,
break_ctl, attr_group, rs485_term_gpio, and rs485_rx_during_tx_gpio on
uart->port?

During registration, __serial8250_register_8250_port() in
drivers/tty/serial/8250/8250_core.c conditionally copies get_rxtrig,
set_rxtrig, and break_ctl from the subdriver template:

	if (up->port.get_rxtrig)
		uart->port.get_rxtrig = up->port.get_rxtrig;
	if (up->port.set_rxtrig)
		uart->port.set_rxtrig = up->port.set_rxtrig;
	if (up->port.startup)
		uart->port.startup = up->port.startup;
	if (up->port.shutdown)
		uart->port.shutdown = up->port.shutdown;
	if (up->port.break_ctl)
		uart->port.break_ctl = up->port.break_ctl;

When a modular subdriver such as mxpcie8250_probe() in
drivers/tty/serial/8250/8250_mxpcie.c registers a port with a type that sets
uart->port.attr_group = &serial8250_dev_attr_group via
serial8250_config_port() -> register_dev_spec_attr_grp(), unbinding the
subdriver leaves attr_group, get_rxtrig, and set_rxtrig set on the fallback
ISA port. When uart_add_one_port() -> serial_core_add_one_port() re-adds the
fallback port, it re-attaches uport->attr_group (exposing rx_trig_bytes in
sysfs), and reading or writing rx_trig_bytes calls rx_trig_bytes_show() or
rx_trig_bytes_store() in drivers/tty/serial/8250/8250_port.c:

	if (uport->get_rxtrig)
		rxtrig_bytes = uport->get_rxtrig(uport);
	else
		rxtrig_bytes = do_serial8250_get_rxtrig(port);

If 8250_mxpcie was unloaded, this calls into freed module text; if the
device was only unbound, mxpcie8250_set_rxtrig() dereferences NULL from
dev_get_drvdata(port->dev) on &serial8250_isa_devs->dev and accesses
unmapped port->membase.

Similarly, if the fallback port is later configured or its serial8250_ports
slot is reused by another subdriver that does not set break_ctl (or if the
previous subdriver used UPIO_AU, where set_io_from_upio() does not reset
serial_in and serial_out), serial8250_break_ctl() will invoke the previous
subdriver's dangling break_ctl callback.

In addition, __serial8250_register_8250_port() calls uart_get_rs485_mode()
in drivers/tty/serial/serial_core.c, which allocates devm-managed GPIO
descriptors on the subdriver's device:

	desc = devm_gpiod_get_optional(dev, "rs485-term", GPIOD_OUT_LOW);
	if (IS_ERR(desc))
		return dev_err_probe(dev, PTR_ERR(desc), "Cannot get rs485-term-gpios\n");
	port->rs485_term_gpio = desc;
	...
	desc = devm_gpiod_get_optional(dev, "rs485-rx-during-tx", dflags);
	if (IS_ERR(desc))
		return dev_err_probe(dev, PTR_ERR(desc), "Cannot get rs485-rx-during-tx-gpios\n");
	port->rs485_rx_during_tx_gpio = desc;

When the subdriver unbinds, devres frees these descriptors, but
rs485_term_gpio and rs485_rx_during_tx_gpio remain non-NULL. If a subsequent
RS485-capable subdriver without a firmware node (!dev_fwnode(dev)) reuses
the slot, uart_get_rs485_mode() returns early without overwriting those
fields, and uart_set_rs485_termination() or uart_set_rs485_rx_during_tx()
will pass the freed gpio_desc pointers to gpiod_set_value_cansleep().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027-tty-llm-v1-0-99b0202ae546@linuxfoundation.org?part=5