Re: [PATCH v7] can: isotp: fix timer drain order, wakeup handling and tx_gen ordering

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Data race and missing memory barriers in lock-free tx_result delivery causing false transmission errors
--

--- Patch [1]: [PATCH v7] can: isotp: fix timer drain order, wakeup handling and tx_gen ordering ---
commit 542b44e72848ff961294d256e29571f2a35141b8
Author: Oliver Hartkopp <[email protected]>
Subject: can: isotp: fix timer drain order, wakeup handling and tx_gen ordering

This commit aims to fix timer drain order, wakeup handling, and tx_gen
ordering in CAN ISOTP. It aligns lock-free state accesses and tracks per
generation transmission results.

> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index 54becaf6898f1..29a4db93876da 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
> @@ -899,32 +963,33 @@ static void isotp_rcv_echo(struct sk_buff *skb, void *data)
[ ... ]
>  	if (so->tx.idx >= so->tx.len) {
>  		/* we are done */
> -		so->tx.state = ISOTP_IDLE;
> +		isotp_set_tx_result(so, so->tx_gen, 0);
> +		WRITE_ONCE(so->tx.state, ISOTP_IDLE);

[Severity: High]
Does this code need a memory barrier between setting the tx_result and
updating the state?

Without an smp_wmb() (or smp_store_release()) between writing the result
and setting the state to ISOTP_IDLE, could CPU reordering expose the IDLE
state prematurely? This might cause waiting threads to read a stale tx_result
before it is properly updated.

>  		wake_up_interruptible(&so->wait);
>  		goto out_unlock;
>  	}

[ ... ]
> @@ -965,14 +1034,12 @@ static enum hrtimer_restart isotp_tx_timeout(struct isotp_sock *so)
>  	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 */
> +	/* detected timeout: report 'communication error on send' */
> +	isotp_set_tx_result(so, gen, ECOMM);

[Severity: High]
Because the state is explicitly changed to ISOTP_IDLE via cmpxchg() before 
isotp_set_tx_result() is called, is it possible for a thread waiting in 
isotp_sendmsg() to wake up when it sees ISOTP_IDLE and read an uninitialized 
or stale tx_result?

>  
> -	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);
> -	}
> +	sk->sk_err = ECOMM;
> +	if (!sock_flag(sk, SOCK_DEAD))
> +		sk_error_report(sk);

[ ... ]
> @@ -1223,20 +1299,48 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
[ ... ]
>  	if (wait_tx_done) {
> -		/* wait for complete transmission of current pdu */
> -		err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
> +		/* wake up for:
> +		 * - concurrent sendmsg() claiming a new transfer
> +		 * - complete transmission of current PDU
> +		 * - shutdown state change in isotp_release()
> +		 */
> +		err = wait_event_interruptible(so->wait,
> +					       READ_ONCE(so->tx_gen) != my_gen ||
> +					       READ_ONCE(so->tx.state) == ISOTP_IDLE ||
> +					       READ_ONCE(so->tx.state) == ISOTP_SHUTDOWN);
>  		if (err)
>  			goto err_event_drop;
>  
> -		err = sock_error(sk);
> -		if (err)
> -			return err;
> +		/* still our claim, but isotp_release() force-shut it down */
> +		if (READ_ONCE(so->tx_gen) == my_gen &&
> +		    READ_ONCE(so->tx.state) == ISOTP_SHUTDOWN) {
> +			err = -EADDRNOTAVAIL;
> +			goto err_event_drop;
> +		}
> +
> +		/* either our own claim completed normally, or so->tx_gen has
> +		 * since moved on to a new transfer - either way, our result
> +		 * is whatever isotp_get_tx_result() finds recorded for
> +		 * my_gen's so->tx_result[] slot
> +		 */
> +		err = isotp_get_tx_result(so, my_gen);

[Severity: High]
Does this read path also need a memory barrier?

When isotp_get_tx_result() reads the result after waiting, it does so without a
subsequent smp_rmb() (or smp_load_acquire()) after checking tx.state. Could the
CPU reorder and execute the read of tx_result before tx.state is actually
confirmed as ISOTP_IDLE? This might lead to isotp_sendmsg() reading a stale
tx_result on a successful transmission.

> +		return err ? err : size;
>  	}

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