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

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

New issues:
- [High] Missing acquire barrier in wait condition allows stale read of tx_result, spuriously failing transmissions.

Pre-existing issues:
- [High] Invalid Flow Control frames abort the transmission but falsely report success to the sender.
--

--- Patch [1]: [PATCH v8] can: isotp: fix timer drain order, wakeup handling and tx_gen ordering ---
commit 2db87f7eca662e755db3d9c7c9c258bea453c5f2
Author: Oliver Hartkopp <[email protected]>

can: isotp: fix timer drain order, wakeup handling and tx_gen ordering

This patch addresses multiple findings in the CAN ISOTP subsystem
regarding timer drain order, wakeup handling, concurrent transfer
generation tracking, and error reporting.

> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index 54becaf6898f1..6b3d4a429042e 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
[ ... ]
> @@ -450,11 +507,15 @@ static int isotp_rcv_fc(struct isotp_sock *so, struct canfd_frame *cf, int ae)
>  		sk->sk_err = EMSGSIZE;
>  		if (!sock_flag(sk, SOCK_DEAD))
>  			sk_error_report(sk);
> +		tx_err = EMSGSIZE;
>  		fallthrough;
>  
>  	default:
>  		/* stop this tx job */
> -		so->tx.state = ISOTP_IDLE;
> +
> +		isotp_set_tx_result(so, so->tx_gen, tx_err);

[Severity: High]
This is a pre-existing issue, but does this code falsely report success
when an invalid Flow Control frame is received?

If the Flow Status is invalid (not CTS, WT, or OVFLW), it hits this
default case where tx_err is still its initial value of 0. It seems
this will store 0 (success) in tx_result and cause the sender to believe
the payload was successfully transmitted when it was actually aborted
due to a protocol violation.

> +		/* set to IDLE after publishing tx_result */
> +		smp_store_release(&so->tx.state, ISOTP_IDLE);
>  		wake_up_interruptible(&so->wait);
>  	}
>  	return 0;
[ ... ]
> @@ -1223,20 +1306,49 @@ 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 pairs with the tx_result writers, SMP-safe */
> +					       smp_load_acquire(&so->tx.state) == ISOTP_IDLE ||
> +					       READ_ONCE(so->tx.state) == ISOTP_SHUTDOWN);

[Severity: High]
Can this short-circuit evaluation skip the smp_load_acquire() barrier?

If a concurrent thread has already claimed the next transfer,
READ_ONCE(so->tx_gen) != my_gen evaluates to true. The condition then
short-circuits, completely bypassing the smp_load_acquire() on so->tx.state.

Without this acquire barrier, can weakly-ordered CPUs speculatively execute
the subsequent read of tx_result inside isotp_get_tx_result() before the
read of tx_gen?

isotp_get_tx_result(struct isotp_sock *so, u32 gen) {
    u32 result = READ_ONCE(so->tx_result[gen % ISOTP_TX_RESULT_SLOTS]);
    if (isotp_get_tx_gen(result) != gen) {
        ...
        return -ECOMM;
    }
}

If so, this might read the stale result of my_gen - 4, failing the internal
generation check and causing a successful ISOTP transfer to spuriously
return -ECOMM to the user.

>  		if (err)
>  			goto err_event_drop;

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