Re: [PATCH v10 3/3] 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] Lockless state transition in the timeout handler creates a race condition that causes timeout errors to be silently dropped or misattributed, leading sendmsg() to erroneously return success for timed-out transfers.
--

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

can: isotp: serialize TX state transitions under so->rx_lock

This commit serializes CAN ISOTP TX state transitions and timer cancellations
under so->rx_lock to prevent concurrent modifications. It also changes the
timeout handlers to locklessly cancel transfers by checking and updating
so->tx_gen and so->tx.state.

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

> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index 44c044eb83e12..54becaf6898f1 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
[ ... ]
> @@ -893,53 +927,83 @@ static void isotp_rcv_echo(struct sk_buff *skb, void *data)
[ ... ]
> +static enum hrtimer_restart isotp_tx_timeout(struct isotp_sock *so)
>  {
> -	struct isotp_sock *so = container_of(hrtimer, struct isotp_sock,
> -					     txtimer);
>  	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 (so->tx.state == ISOTP_IDLE || so->tx.state == ISOTP_SHUTDOWN)
> +	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) {
> +		/* report 'communication error on send' */
> +		sk->sk_err = ECOMM;
> +		if (!sock_flag(sk, SOCK_DEAD))
> +			sk_error_report(sk);
> +	}

[Severity: High]
Does this lockless state transition create a race window where timeout
errors are lost?

By setting so->tx.state to ISOTP_IDLE before setting sk->sk_err = ECOMM,
a thread waiting in isotp_sendmsg() for wait_tx_done might wake up, observe
ISOTP_IDLE, and read sock_error() before the error is set, falsely returning
success.

Additionally, could another concurrent sendmsg() see the IDLE state,
claim the socket, and increment tx_gen before the error is set?

If this happens, the check if (READ_ONCE(so->tx_gen) == gen) will fail,
and the error will never be reported to the original waiting thread.

[ ... ]
> @@ -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);
> +	hrtimer_cancel(&so->txtimer);
> +	hrtimer_cancel(&so->echotimer);

[Severity: High]
When this tx_gen increment occurs concurrently with isotp_tx_timeout()
after the timeout has already transitioned the state to ISOTP_IDLE, doesn't it
cause the timeout handler to suppress the error for the previous transfer?

[ ... ]

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