Re: [PATCH v6 4/5] can: isotp: fix lock-free state transition in tx timer handler

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Early state transition to ISOTP_IDLE in the TX timer handler introduces a race condition that can corrupt the error state of a concurrent transmission.

Pre-existing issues:
- [High] A signal during the `isotp_sendmsg()` wait loop can abort another thread's active transmission on a shared socket.
--

--- Patch [4]: [PATCH v6 4/5] can: isotp: fix lock-free state transition in tx timer handler ---
commit 3453d78776681a05466b1147a03618091e6287bc
Author: Oliver Hartkopp <[email protected]>
can: isotp: fix lock-free state transition in tx timer handler

This commit introduces a cmpxchg to safely transition the state machine
to ISOTP_IDLE in the tx timer handler. It ensures the state is only updated
if it hasn't been changed concurrently.

Link: https://lore.kernel.org/linux-can/[email protected]/

> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index 5ef2a610fc172..0826ae8372467 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
> @@ -939,9 +939,16 @@ static enum hrtimer_restart isotp_tx_timer_handler(struct hrtimer *hrtimer)

[ ... ]

> +	/* Only claim timeout if state is unchanged to avoid overwriting
> +	 * concurrent state updates from isotp_sendmsg/isotp_rcv_echo.
> +	 */
> +	if (cmpxchg(&so->tx.state, old_state, ISOTP_IDLE) != old_state)
>  		return HRTIMER_NORESTART;
>  
>  	/* we did not get any flow control or echo frame in time */
> @@ -951,8 +958,6 @@ static enum hrtimer_restart isotp_tx_timer_handler(struct hrtimer *hrtimer)
>  	if (!sock_flag(sk, SOCK_DEAD))
>  		sk_error_report(sk);

[Severity: High]
Does this early state transition to ISOTP_IDLE introduce a race condition?

If the cmpxchg successfully sets the state to ISOTP_IDLE, another thread 
calling isotp_sendmsg() on a different CPU could see the IDLE state and
immediately start a new transmission before this timer handler completes.

If that happens, the timer thread would resume and execute sk->sk_err = ECOMM
and sk_error_report(sk), which might incorrectly taint the new transmission
with a communication error.

> -	/* reset tx state */
> -	so->tx.state = ISOTP_IDLE;
>  	wake_up_interruptible(&so->wait);
>  
>  	return HRTIMER_NORESTART;

[Severity: High]
This is a pre-existing issue, but can a signal during the wait loop in
isotp_sendmsg() abort another thread's active transmission?

Looking at isotp_sendmsg(), if a concurrent sender fails to acquire the
state and waits:

net/can/isotp.c:isotp_sendmsg()
        err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
        if (err)
                goto err_event_drop;

If a signal interrupts this wait, the thread jumps to err_event_drop where
it unconditionally cancels the timers and resets the state:

err_event_drop:
        hrtimer_cancel(&so->txfrtimer);
        hrtimer_cancel(&so->txtimer);
err_out_drop:
        so->tx.state = ISOTP_IDLE;

Since this thread was only waiting and never claimed the state machine, will
this forcefully cancel the active transmission of the thread that actually
owned it?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4