Re: [PATCH v4] 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] The `tx_result` error reporting bypasses `sock_error(sk)`, causing failed transfers to falsely report success and leaving pending errors to falsely fail subsequent successful transfers.

Pre-existing issues:
- [High] The error path after `can_send()` in `isotp_sendmsg()` corrupts the state of concurrent transfers because it unconditionally resets socket state variables without verifying transfer ownership.
--

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

    can: isotp: fix timer drain order, wakeup handling and tx_gen ordering
    
    This patch is a follow-up to commit cf070fe33bfb ("can: isotp: serialize
    TX state transitions under so->rx_lock") which addresses following
    sashiko-bot findings:
    
    - isotp_sendmsg(): drain so->txfrtimer first so a stale callback can't
      re-arm echotimer after the claim

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

[ ... ]

> @@ -1229,11 +1279,39 @@ 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;
>  
> +		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]
Does this early return bypass sock_error(sk) and leave sk->sk_err unconsumed?

If this thread's transfer times out, isotp_tx_timeout() will set sk->sk_err to
ECOMM. If another thread claims the socket and completes a new transfer before
this thread wakes up, tx_result is overwritten with 0 (success).

When this thread eventually wakes up, it sees tx_gen != my_gen and the
generation in tx_result no longer matches my_gen. As a result, tx_err defaults
to 0 and it returns size (success) without calling sock_error(sk), leaving
ECOMM pending in sk->sk_err.

> +
> +		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]
Will a subsequent successful transfer falsely fail here?

If a previous transfer left sk->sk_err unconsumed as described above, a
subsequent successful transfer from another thread would hit this sock_error(sk)
call, consume the pending ECOMM, and falsely return a failure.

>  	}
>  
>  	return size;

[ ... ]

> @@ -1246,15 +1324,26 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
>  	spin_lock_bh(&so->rx_lock);
>  	goto err_out_drop_locked;
>  err_event_drop:
> -	/* interrupted waiting on our own transfer - drain its timers */
> +	/* interrupted or shut down while waiting on our own transfer */
>  	spin_lock_bh(&so->rx_lock);
> +
> +	/* new transfer already started by concurrent sendmsg()? */
> +	if (READ_ONCE(so->tx_gen) != my_gen) {
> +		/* don't touch timers and states of the new transfer */
> +		spin_unlock_bh(&so->rx_lock);
> +		return err;
> +	}
> +
>  	hrtimer_cancel(&so->txfrtimer);
>  	hrtimer_cancel(&so->txtimer);
>  	hrtimer_cancel(&so->echotimer);
>  err_out_drop_locked:
>  	/* release the claim; so->rx_lock still held from above */
> -	so->cfecho = 0;
> -	so->tx.state = ISOTP_IDLE;
> +	WRITE_ONCE(so->cfecho, 0);
> +
> +	/* only claim to IDLE if isotp_release() has not taken over */
> +	if (READ_ONCE(so->tx.state) != ISOTP_SHUTDOWN)
> +		WRITE_ONCE(so->tx.state, ISOTP_IDLE);
>  	spin_unlock_bh(&so->rx_lock);
>  	wake_up_interruptible(&so->wait);
>  

[Severity: High]
This is a pre-existing issue, but does this unconditionally clobber the state
of a concurrent transfer?

If a thread gets preempted or delayed for a long time during can_send() which
happens earlier in isotp_sendmsg(), isotp_tx_timeout() could fire and reset
the state to ISOTP_IDLE, allowing another thread to claim the socket and begin
a new transfer.

When the original thread resumes, if can_send() returns an error (e.g.,
-ENOBUFS), it will jump directly to err_out_drop_locked:

    err = can_send(skb, 1);
    if (err) {
        ...
        goto err_out_drop_locked;
    }

And then blindly clear so->cfecho and set tx.state to ISOTP_IDLE, destroying
the active transfer state of the other thread.

While this patch adds a tx_gen ownership check for err_event_drop, it seems to
have missed adding the same verification for the err_out_drop_locked path.

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