Re: [PATCH] serial: 8250: clear RX trigger callbacks when removing a port

[Jiri Slaby <jirislaby@kernel.org>]

On 24. 10. 26, 22:05, James Hilliard wrote:
> serial8250_unregister_port() removes the device supplied by a driver and
> normally re-registers the persistent slot as an unknown legacy port. The
> slot retains its RX trigger callbacks and sysfs attribute group, so the
> replacement rx_trig_bytes attribute can still call the removed driver's
> code. Unloading that driver leaves the callbacks pointing into freed
> module text.
> 
> Clear both callbacks after uart_remove_one_port() has drained the old
> sysfs users and before registering the fallback port. Its RX trigger
> attribute then uses the generic handlers, which reject a port without
> FIFO capabilities.
> 
> Also copy the callbacks unconditionally during registration. A replacement
> port without custom handlers must use the generic implementation instead
> of inheriting callbacks from the previous owner of the slot.

I don't understand why this is a problem only for the two callbacks. 
Care to elaborate?

> Fixes: 8a6c543cf8b3 ("serial: 8250: allow UART drivers to override rx_trig_bytes handling")
> Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> ---
>   drivers/tty/serial/8250/8250_core.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> index 42319843cb5b..c638e10c43e2 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -830,10 +830,8 @@ static int __serial8250_register_8250_port(struct uart_8250_port *uart,
>   		uart->port.get_divisor = up->port.get_divisor;
>   	if (up->port.set_divisor)
>   		uart->port.set_divisor = up->port.set_divisor;
> -	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;
> +	uart->port.get_rxtrig = up->port.get_rxtrig;
> +	uart->port.set_rxtrig = up->port.set_rxtrig;
>   	if (up->port.startup)
>   		uart->port.startup = up->port.startup;
>   	if (up->port.shutdown)
> @@ -953,6 +951,9 @@ void serial8250_unregister_port(struct uart_8250_port *uart)
>   	}
>   
>   	uart_remove_one_port(&serial8250_reg, &uart->port);
> +	/* The fallback port must use the generic RX trigger handlers. */
> +	uart->port.get_rxtrig = NULL;
> +	uart->port.set_rxtrig = NULL;
>   	if (serial8250_isa_devs) {
>   		uart->port.flags &= ~UPF_BOOT_AUTOCONF;
>   		uart->port.type = PORT_UNKNOWN;

thanks,
-- 
js
suse labs