Re: [PATCH v6] 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:
- [High] TOCTOU race in `isotp_tx_timeout` exposes intermediate state, corrupting error reporting and overwriting subsequent transfer results.
- [High] Silent failure when TX timeout error is overwritten by rapidly completing concurrent transfers, causing false success reports.
--

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

can: isotp: fix timer drain order, wakeup handling and tx_gen ordering
    
This patch addresses sashiko-bot findings by serializing TX state transitions
and handling concurrent claims during wait_tx_done timeouts. It also aligns
lock-free accesses and uses skb->hash as a unique loopback echo frame indicator.

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

[ ... ]

> @@ -965,10 +1007,10 @@ static enum hrtimer_restart isotp_tx_timeout(struct isotp_sock *so)
>  	/* 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 */
> +	/* detected timeout: report 'communication error on send' */
> +	WRITE_ONCE(so->tx_result, isotp_pack_tx_result(gen, ECOMM));

[Severity: High]
Could this lead to a race condition where a stale error corrupts a new
transfer? 

If this thread is preempted immediately after the cmpxchg() sets the state
to ISOTP_IDLE, the socket becomes available for other threads.

A concurrent sendmsg() thread could then claim the socket, increment
so->tx_gen, and complete a new transfer. 

When this timeout handler resumes and executes the WRITE_ONCE() above, it
would overwrite the new transfer's result with the stale ECOMM error and
the old generation tag. 

Later, when the new transfer's waiter in isotp_sendmsg() reads so->tx_result,
it checks that so->tx_gen still matches its own claim:

isotp_sendmsg() {
        ...
        /* ISOTP_IDLE: so->tx_gen still matches our own claim */
        err = isotp_get_tx_err(READ_ONCE(so->tx_result));
        if (err)
                return -err;
}

Since so->tx_gen matches the new transfer, the waiter extracts the error
without validating the generation tag inside tx_result, falsely returning
-ECOMM instead of success.

>  
>  	if (READ_ONCE(so->tx_gen) == gen) {
> -		/* report 'communication error on send' */
>  		sk->sk_err = ECOMM;
>  		if (!sock_flag(sk, SOCK_DEAD))

[ ... ]

> @@ -1223,20 +1274,57 @@ 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 (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);
> +
> +			return tx_err ? -tx_err : size;
> +		}

[Severity: High]
Can a timed-out transfer falsely return success if its error is overwritten
by a concurrent transfer?

If a transfer times out, so->tx_result is set to the error code and the
waiting sendmsg() thread is woken up.

Before the original thread gets CPU time, a concurrent thread calling
sendmsg() could claim the newly IDLE socket and complete a short transfer.
This successful transfer unconditionally overwrites so->tx_result with 0:

isotp_rcv_echo() {
        ...
        if (so->tx.idx >= so->tx.len) {
                /* we are done */
                WRITE_ONCE(so->tx_result, isotp_pack_tx_result(so->tx_gen, 0));
                WRITE_ONCE(so->tx.state, ISOTP_IDLE);
                ...
}

When the originally woken thread finally runs, it observes that
READ_ONCE(so->tx_gen) != my_gen and enters this block. 

Since the generation tag inside the overwritten tx_result no longer matches
my_gen, it defaults to tx_err = 0 and returns the payload size. This would
cause the application to silently lose data, falsely believing its message
was sent successfully.

> +
> +		if (READ_ONCE(so->tx.state) == ISOTP_SHUTDOWN) {
> +			/* isotp_release() has taken over the claim */
> +			err = -EADDRNOTAVAIL;
> +			goto err_event_drop;
> +		}
> +
> +		/* ISOTP_IDLE: so->tx_gen still matches our own claim */
> +		err = isotp_get_tx_err(READ_ONCE(so->tx_result));
>  		if (err)
> -			return err;
> +			return -err;
>  	}

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