Re: [PATCH v5] 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] A race condition in the timeout handler silently suppresses a valid timeout error for a completed transfer.

Pre-existing issues:
- [High] Error code stealing between concurrent sendmsg calls leads to swapped return values.
--

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

This commit addresses several findings related to timer drain order, wakeup
handling, and TX generation ordering in isotp_sendmsg(). It serializes state
transitions under the rx_lock and prevents stale callbacks from re-arming
timers.

> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index 54becaf6898f1..4a5897cc21030 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c

[ ... ]

> @@ -965,10 +1007,9 @@ 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' */
>  	if (READ_ONCE(so->tx_gen) == gen) {
> -		/* report 'communication error on send' */
> +		WRITE_ONCE(so->tx_result, isotp_pack_tx_result(gen, ECOMM));
>  		sk->sk_err = ECOMM;

[Severity: High]
Is there a race condition here that could silently suppress a valid timeout
error for a completed transfer?

In isotp_tx_timeout(), the socket state is locklessly transitioned to
ISOTP_IDLE via cmpxchg(). If a concurrent sendmsg() thread observes the
ISOTP_IDLE state at this exact moment, it could claim the socket and increment
so->tx_gen.

When the timeout handler resumes and evaluates:
    if (READ_ONCE(so->tx_gen) == gen)

The condition would evaluate to false because the generation was bumped. This
aborts the handler without recording the error in tx_result or sk_err. The
original transferring thread then wakes up, finds no error recorded, and
returns success despite the timeout, leading to silent data loss.

The commit message mentions that the race that can suppress a valid ECOMM
entry is intentionally left for a later change, but since no subsequent patch
is included to fix this, is this safe to leave as-is?

[ ... ]

> @@ -1223,17 +1273,58 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
>  			       __func__, ERR_PTR(err));
>  
>  		spin_lock_bh(&so->rx_lock);
> +
> +		/* new claimed transfer due to a stuck can_send() ? */
> +		if (READ_ONCE(so->tx_gen) != my_gen) {
> +			/* don't touch timers and state of the new transfer */
> +			spin_unlock_bh(&so->rx_lock);
> +			return err;
> +		}
> +
>  		/* no transmission -> no timeout monitoring */
>  		hrtimer_cancel(tx_hrt);
>  		goto err_out_drop_locked;
>  	}
>  
>  	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;
>  
> +		if (READ_ONCE(so->tx_gen) != my_gen) {
> +			/* a new transfer has since been claimed - so->tx.state
> +			 * already belongs to it, but so->tx_result still
> +			 * carries our own completion status, unless a second
> +			 * transfer has since completed and overwritten it too
> +			 */
> +			u32 result = READ_ONCE(so->tx_result);
> +			int tx_err = 0;
> +
> +			if (isotp_get_tx_gen(result) == my_gen)
> +				tx_err = isotp_get_tx_err(result);
> +
> +			/* drain sk_err and select return value */
> +			err = sock_error(sk);
> +			if (!tx_err && err)
> +				tx_err = -err;
> +
> +			return tx_err ? -tx_err : size;
> +		}
> +
> +		if (READ_ONCE(so->tx.state) == ISOTP_SHUTDOWN) {
> +			/* isotp_release() has taken over the claim */
> +			err = -EADDRNOTAVAIL;
> +			goto err_event_drop;
> +		}
> +
>  		err = sock_error(sk);
>  		if (err)
>  			return err;

[Severity: High]
This is a pre-existing issue, but does this error checking path lead to
swapped return values between concurrent sendmsg() calls?

If Thread 1 completes a successful transfer but is delayed from waking up,
Thread 2 can claim the socket, start a new transfer, time out, and set
sk->sk_err = ECOMM.

When Thread 1 then wakes up, it sees its generation no longer matches
(so->tx_gen != my_gen), and it calls sock_error(sk) in the fallback block:

    err = sock_error(sk);
    if (!tx_err && err)
        tx_err = -err;

Because sock_error() internally consumes the error via xchg(), Thread 1
effectively steals Thread 2's error and incorrectly returns -ECOMM.

Later, when Thread 2 wakes up and calls sock_error(sk) on the normal
error checking path:

    err = sock_error(sk);
    if (err)
        return err;

It gets 0, leading it to falsely return success for its failed transfer.
Could this corrupt the application's view of the network state?

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