Re: [PATCH] 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:

Pre-existing issues:
- [High] Signal interruption in wait_tx_done unconditionally resets socket state, potentially clobbering a concurrent transfer.
--

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

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

This patch addresses several issues found in the CAN ISOTP protocol.
It fixes timer drain ordering, wakeup handling on socket release, and
ensures tx_gen is incremented before state transitions to avoid timeout
stamp races.

> diff --git a/net/can/isotp.c b/net/can/isotp.c
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
[ ... ]
> @@ -1246,15 +1261,18 @@ 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);
>  	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);

[Severity: High]
This is a pre-existing issue, but can this error path clobber a concurrent
transfer?

If Thread A calls isotp_sendmsg() with CAN_ISOTP_WAIT_TX_DONE and is
interrupted by a signal, wait_event_interruptible() evaluates to false and
returns -ERESTARTSYS.

Before Thread A can acquire so->rx_lock at err_event_drop, its transfer
might complete normally. The RX path would set so->tx.state to ISOTP_IDLE
and wake up a concurrently blocked Thread B.

If Thread B then claims the socket, increments so->tx_gen, and starts its
own transfer, Thread A would eventually resume, acquire so->rx_lock, and
blindly cancel timers and reset tx.state to ISOTP_IDLE, corrupting Thread B's
new transfer.

Should this error path verify that so->tx_gen still matches the thread's own
generation before resetting the state, similar to how it is checked in
isotp_tx_timeout()?

>  	spin_unlock_bh(&so->rx_lock);
>  	wake_up_interruptible(&so->wait);
>

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