Re: [PATCH net 19/19] can: isotp: serialize TX state transitions under so->rx_lock

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Racy evaluation of `so->tx_gen` in `isotp_tx_timeout` suppresses legitimate timeout errors and forces waiters to block on subsequent transfers.
--

--- Patch [19]: [PATCH net 19/19] can: isotp: serialize TX state transitions under so->rx_lock ---
commit d815650c77b9d2ce6acff4c164869dfbfb037f2e
Author: Oliver Hartkopp <[email protected]>
can: isotp: serialize TX state transitions under so->rx_lock

This commit reorganizes the locking and state machine in the CAN ISOTP
transmit path, making sendmsg() operations fully serialized under
so->rx_lock. It introduces a generation counter (so->tx_gen) to handle
timer callbacks racing with new transfers.

Link: https://patch.msgid.link/[email protected]

> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c

[ ... ]

> +static enum hrtimer_restart isotp_tx_timeout(struct isotp_sock *so)
>  {
>  	struct sock *sk = &so->sk;
> +	u32 gen = READ_ONCE(so->tx_gen);
> +	u32 old_state = READ_ONCE(so->tx.state);
>  
>  	/* don't handle timeouts in IDLE or SHUTDOWN state */
> +	if (old_state == ISOTP_IDLE || old_state == ISOTP_SHUTDOWN)
> +		return HRTIMER_NORESTART;
> +
> +	/* only claim the timeout if the state is still unchanged */
> +	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 */
>  
> -	/* report 'communication error on send' */
> -	sk->sk_err = ECOMM;
> -	if (!sock_flag(sk, SOCK_DEAD))
> -		sk_error_report(sk);
> +	if (READ_ONCE(so->tx_gen) == gen) {

[Severity: High]
Does setting the state to ISOTP_IDLE before checking the generation
counter introduce a race condition?

If cmpxchg() successfully sets the state to ISOTP_IDLE, a concurrent
isotp_sendmsg() thread could immediately acquire the socket, set the state
to ISOTP_SENDING, and bump so->tx_gen.

If this occurs before we evaluate READ_ONCE(so->tx_gen) here, the timer
callback will incorrectly assume the transfer was superseded and skip
setting the ECOMM error. Could this lead to silent data loss?

> +		/* report 'communication error on send' */
> +		sk->sk_err = ECOMM;
> +		if (!sock_flag(sk, SOCK_DEAD))
> +			sk_error_report(sk);
> +	}
>  
> -	/* reset tx state */
> -	so->tx.state = ISOTP_IDLE;
>  	wake_up_interruptible(&so->wait);
>  
>  	return HRTIMER_NORESTART;
>  }

[ ... ]

> @@ -960,13 +1024,24 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)

[ ... ]

> +	/* new transfer: bump so->tx_gen and drain the old one's timers,
> +	 * still under the so->rx_lock we just claimed the socket with
> +	 */
> +	WRITE_ONCE(so->tx.state, ISOTP_SENDING);
> +	WRITE_ONCE(so->tx_gen, READ_ONCE(so->tx_gen) + 1);

[Severity: High]
If the timeout callback races with the state transition above, a
concurrent sender updating so->tx_gen here will poison the check in the
interrupted timer callback.

Could this also cause the sender thread to block waiting for an unrelated
concurrent transmission to finish, rather than handling the timeout?

> +	hrtimer_cancel(&so->txtimer);
> +	hrtimer_cancel(&so->echotimer);
> +	hrtimer_cancel(&so->txfrtimer);
> +	so->cfecho = 0;
> +	spin_unlock_bh(&so->rx_lock);

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