Re: [PATCH v3] serial: max310x: drive RTS in software when hardware delays are too short

[Tapio Reijonen]

> Sashiko AI review found 5 potential issue(s) to consider:
> - [High] serial: max310x: port lock dropped in serial core callbacks
>   breaks atomicity
> - [High] serial: max310x: lockless 64-bit ktime and state updates
>   cause torn reads
> - [High] serial: max310x: TOCTOU race in delayed_stop_tx resurrects
>   cancelled timer
> - [High] serial: max310x: race in rts_work_proc transmits data with
>   RTS disabled
> - [High] serial: max310x: shutdown bypasses timer cancellation if
>   sw_rts toggles

Three of these lead to changes in v4; two do not. Please do not apply
this version.

On "shutdown bypasses timer cancellation if sw_rts toggles":

Correct, and it is the one that does not even need a race. The hardware
path is selected per port from the current baud and delays, so a
TIOCSRS485 that moves a port from the software path to the hardware path
sets sw_rts_during_tx to false for good. If a software timed envelope
was in flight at that moment its timer stays armed, and
max310x_shutdown() then takes the branch that never calls
hrtimer_cancel() or cancel_work_sync(). The port is powered down with
the timer still pending, and rts_work runs an SPI write against it
afterwards. A write, a TIOCSRS485 and a close inside the before-send
delay reach this with no unusual scheduling at all.

v4 keeps the two drain loops in the conditional, since they legitimately
differ, and moves the cancellation out of it so it runs unconditionally.
On the hardware path that is a cancel of a timer that was never armed
and of work that was never queued.

On "TOCTOU race in delayed_stop_tx resurrects cancelled timer":

Correct. max310x_delayed_stop_tx() reads tx_state, then does an SPI read
of TXFIFOLVL, then takes the lock. Across that window
max310x_shutdown() can cancel the timer, set tx_state to
MAX310X_TX_OFF and power the port down. What resurrects the envelope is
that the code then clears cancel_tx_delay_tmr unconditionally and arms
on tx_state != MAX310X_TX_WAIT_BEFORE_SEND, which is also true for
MAX310X_TX_OFF.

v4 drops the clear, since that flag belongs to whoever set it and
max310x_delayed_start_tx() already clears it at the one point where a
new envelope legitimately begins, and re-checks under the lock with a
positive test: arm only while tx_state is MAX310X_TX_SEND. That is the
only state the after-send hold may be armed from, and it covers OFF,
WAIT_BEFORE_SEND and WAIT_AFTER_SEND in one condition.

On "lockless 64-bit ktime and state updates cause torn reads":

Half of this is real. max310x_set_rts_ctl_params() assigns
sw_rts_during_tx false and only then recomputes it, so every concurrent
reader can observe a spurious false, and the permanent case above comes
through the same field. v4 computes the decision into a local and
publishes it once.

The torn read of one_character_duration I do not think is reachable. It
holds one frame at the current baud, and the lowest baud selectable here
is uartclk / 16 / 0xffff, which is 42 on this part (uartclk 44.2 MHz).
One 12-bit frame at 42 baud is 286 ms. The upper word only becomes
non-zero past 4.3 s, which would need a baud below 3, so every value the
driver can hold has a zero upper word and a tear cannot change it.

On "port lock dropped in serial core callbacks breaks atomicity":

This one is deliberate and I do not plan to change it.
max310x_tmr_tx() takes port->lock, so calling hrtimer_cancel() while
holding it would deadlock against a callback already running on another
CPU. hrtimer_try_to_cancel() is tried first and the unlock only happens
on -1, which is exactly the case where that callback is spinning on the
lock being dropped; cancel_tx_delay_tmr is what makes the re-entry safe.
Several other serial drivers drop and retake the lock the same way. The
review's own dismissed-concerns section reaches the same conclusion,
and separately notes that uart_rs485_config() wraps the rs485_config()
call in scoped_guard(uart_port_lock_irqsave, port), so the lock state
on entry is what the code assumes.

On "race in rts_work_proc transmits data with RTS disabled":

I could not construct a reachable ordering for this one. If the worker
reads MAX310X_TX_OFF and max310x_start_tx() then sets
MAX310X_TX_WAIT_BEFORE_SEND, start_tx also schedules rts_work again, and
that run reads the new state and asserts RTS. The before-send delay the
first run arms is longer than 15 bit-times by construction, since that
is what selects the software path in the first place, so the second run
lands well inside it. The timer only schedules tx_work, which queues
behind the pending rts_work. If there is a concrete ordering where data
is shifted with RTS deasserted, I would like to see it.

On the three concerns in the web report that are not in the mail: I
agree with all three and with the preexisting flag on each. The
cancel-before-uart_remove_one_port() ordering in max310x_remove() and
the out_uart path are both unchanged from the base commit, which already
cancels the three original work items in that order, and the unlocked
xmit_fifo and icount access from tx_work is how this driver has always
transmitted. They are worth fixing; they are not this patch's to fix.

Tapio