Re: [PATCH v8 8/9] serial: max310x: drive RTS in software when hardware delays are too short

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] tty: use-after-free in max310x_remove due to premature workqueue cancellation
- [High] tty: lockless kfifo check in max310x_delayed_stop_tx truncates RS485 transmission
- [High] tty: TOCTOU race in max310x_rts_work_proc decouples timer arming from RTS assertion
- [High] tty: unprotected tx_state read in max310x_set_rts_ctl_params drops RTS mid-transmission
- [Medium] tty: trailing hrtimer running state in max310x_delayed_stop_tx strands RTS
- [Medium] tty: TOCTOU in max310x_rts_work_proc clobbers break_ctl RTS assertion
--

commit 05eccd91a3e45c1a75cc25465960c12d3c83b68c
Author: Tapio Reijonen <tapio.reijonen@vaisala.com>

serial: max310x: drive RTS in software when hardware delays are too short
    
The commit adds a software-driven RTS path for max310x when requested RS485
delays exceed the hardware's 15 bit-time limit. It uses an hrtimer and a
workqueue to track before-send and after-send RTS envelope phases and
handle teardown interlocking.

> diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
> index c1fe4ad887839..5fdb9dfca6027 100644
> --- a/drivers/tty/serial/max310x.c
> +++ b/drivers/tty/serial/max310x.c

[ ... ]

> +static void max310x_rts_work_proc(struct work_struct *ws)
> +{
> +	struct max310x_one *one = container_of(ws, struct max310x_one, rts_work);
> +	struct uart_port *port = &one->port;
> +	bool rts_on = READ_ONCE(one->tx_state) != MAX310X_TX_OFF ||
> +		      READ_ONCE(one->tx_break);
> +
> +	max310x_rts_ctl(port, max310x_rts_level(port, rts_on));

[Severity: Medium]
Can this sequence clobber a concurrent break_ctl() RTS assertion?

A concurrent max310x_break_ctl() sets tx_break and manually asserts the RTS
pin. If this function evaluates tx_break as false locklessly, gets preempted
while break_ctl() asserts RTS, and then resumes, it could write its stale
'false' evaluation to the hardware and drop the RTS pin prematurely.

> +
> +	guard(spinlock_irqsave)(&port->lock);
> +	if (READ_ONCE(one->tx_state) == MAX310X_TX_WAIT_BEFORE_SEND &&
> +	    !one->cancel_tx_delay_tmr && !hrtimer_active(&one->tx_delay_tmr))
> +		hrtimer_start(&one->tx_delay_tmr,
> +			      ms_to_ktime(port->rs485.delay_rts_before_send),
> +			      HRTIMER_MODE_REL);
> +}

[Severity: High]
Does this lockless read decouple the timer arming from the physical RTS pin
assertion?

If a concurrent start_tx() sets tx_state to MAX310X_TX_WAIT_BEFORE_SEND and
queues a new rts_work after the lockless evaluation of rts_on but before
taking port->lock, the timer could be armed while the pin is still
deasserted, shortening the setup delay.

[ ... ]

> +static void max310x_delayed_stop_tx(struct uart_port *port)
> +{
> +	struct max310x_one *one = to_max310x_port(port);
> +	unsigned int txlvl;
> +
> +	if (READ_ONCE(one->tx_state) == MAX310X_TX_OFF)
> +		return;
> +
> +	/*
> +	 * Data still queued for transmission defers the hold: a refill is
> +	 * coming and the envelope is not over. Stopped output does not
> +	 * count - it drains nowhere, and the envelope must end.
> +	 */
> +	if (!kfifo_is_empty(&port->state->port.xmit_fifo) &&
> +	    !uart_tx_stopped(port))
> +		return;
> +
> +	/*
> +	 * The kfifo can be empty while the chip TX FIFO is still draining, so arm
> +	 * the after-send hold only once the chip FIFO is empty too - the TX-empty
> +	 * interrupt re-invokes us then. Otherwise the hold starts early and RTS
> +	 * drops mid-character, clipping the last byte(s).
> +	 */
> +	txlvl = max310x_port_read(port, MAX310X_TXFIFOLVL_REG);
> +	if (txlvl)
> +		return;
> +
> +	/*
> +	 * Runs from tx_work without port->lock, so re-check the state under it:
> +	 * shutdown() may have cancelled the envelope meanwhile. Only
> +	 * MAX310X_TX_SEND may arm the hold.
> +	 */
> +	guard(spinlock_irqsave)(&one->port.lock);
> +	if (one->cancel_tx_delay_tmr ||
> +	    READ_ONCE(one->tx_state) != MAX310X_TX_SEND)
> +		return;

[Severity: High]
Is it possible for the lockless checks performed earlier in this function
to evaluate as empty, but for new data to be queued concurrently before
acquiring port->lock?

If new data is queued by a concurrent start_tx(), the function will observe
tx_state is still MAX310X_TX_SEND without re-evaluating the FIFO, and arm
the after-send timer. This could drop the RTS pin mid-transmission when the
timer expires, truncating the RS485 frame.

> +
> +	if (!hrtimer_active(&one->tx_delay_tmr)) {

[Severity: Medium]
Could this check strand the RTS pin asserted?

The before-send timer callback releases the port lock and returns
HRTIMER_NORESTART, but the hrtimer core retains the running state until the
callback fully returns.

If tx_work executes concurrently, drains the FIFO, and enters this path
before the hrtimer core clears the running state, this check evaluates to
true. This skips starting the after-send hold, leaving tx_state stuck in
MAX310X_TX_SEND and never releasing the RTS pin.

> +		/*
> +		 * Add one character for the byte still in the shift register -
> +		 * TX-empty fires as it enters, not as it leaves.
> +		 */
> +		ktime_t delay = us_to_ktime(one->char_time_us +
> +					    port->rs485.delay_rts_after_send *
> +					    USEC_PER_MSEC);
> +
> +		WRITE_ONCE(one->tx_state, MAX310X_TX_WAIT_AFTER_SEND);
> +		hrtimer_start(&one->tx_delay_tmr, delay, HRTIMER_MODE_REL);
> +	}
> +}

[ ... ]

> @@ -957,25 +1165,35 @@ static void max310x_set_rts_ctl_params(struct max310x_one *one)
>  	struct uart_port *port = &one->port;
>  	unsigned int setup = 0, hold = 0;
>  	u8 mode1 = 0, irda = 0;
> +	bool sw_rts = false;

[ ... ]

> +	if (port->rs485.flags & SER_RS485_ENABLED) {
> +		if (sw_rts) {
> +			/*
> +			 * Only settle RTS at idle when no transmission owns it.
> +			 * A reconfigure while one is in flight - rs_work runs
> +			 * on every TIOCSRS485 - would otherwise release the
> +			 * transceiver mid-character. A transfer the hardware
> +			 * path owns is invisible to tx_state - adopt it
> +			 * instead of settling. The reverse strand also
> +			 * exists: an envelope the hardware path inherited
> +			 * mid-envelope leaves tx_state set with nothing
> +			 * driving the pin.
> +			 */
> +			if (READ_ONCE(one->tx_state) != MAX310X_TX_OFF ||
> +			    max310x_adopt_hw_tx(one)) {
> +				/*
> +				 * The transfer may be running under auto-RTS:
> +				 * assert before auto-RTS is disabled below so
> +				 * the pin is never released under it, and arm
> +				 * the after-send hold - with the chip FIFO
> +				 * already drained no TX-empty interrupt
> +				 * arrives to arm it.
> +				 */
> +				max310x_rts_ctl(port,
> +						max310x_rts_level(port, true));
> +				max310x_delayed_stop_tx(port);
> +			} else {
> +				max310x_rts_ctl(port,
> +						max310x_rts_level(port, false));
> +				/*
> +				 * serial_core calls set_termios() without
> +				 * port->lock, so an envelope may have started
> +				 * while the idle level was written and the
> +				 * settle can land after its RTS assert.
> +				 * rts_work re-derives the level from
> +				 * tx_state; requeue it to converge.
> +				 */
> +				if (READ_ONCE(one->tx_state) != MAX310X_TX_OFF)
> +					schedule_work(&one->rts_work);
> +			}

[Severity: High]
Is there a race that could cause the RTS pin to drop mid-transmission?

Since tx_state is read locklessly, a concurrent start_tx() could initiate a
transmission and assert the RTS pin before this slow hardware write executes.

The mitigation to requeue rts_work only re-asserts the RTS pin after it
was already dropped, which would irreparably corrupt the active RS485 frame.

[ ... ]

> @@ -1676,6 +2058,8 @@ static void max310x_remove(struct device *dev)
>  	int i;
>  
>  	for (i = 0; i < s->devtype->nr; i++) {
> +		hrtimer_cancel(&s->p[i].tx_delay_tmr);
> +		cancel_work_sync(&s->p[i].rts_work);
>  		cancel_work_sync(&s->p[i].tx_work);
>  		cancel_work_sync(&s->p[i].md_work);
>  		cancel_work_sync(&s->p[i].rs_work);
>  
>  		if (test_and_clear_bit(s->p[i].port.line, max310x_lines))
>  			uart_remove_one_port(&max310x_uart, &s->p[i].port);

[Severity: Critical]
Can this sequence cause a use-after-free by cancelling background tasks
too early?

uart_remove_one_port() is called after these workqueues and timers are
cancelled. Because the TTY port is still fully registered and active during
this window, userspace operations or hardware interrupts could re-queue
these works.

Once max310x_remove() returns and the memory is freed, any re-queued
works would execute on freed memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008-max310x-rs485-sw-delay-v8-0-6ba881f81e1f@vaisala.com?part=8