Re: [PATCH v7 5/5] can: isotp: fix state machine corruption on signal interruption

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

Pre-existing issues:
- [High] Unconditional timer cancellation in abort paths and lockless softirq handlers can inadvertently cancel a concurrent transmission's timers, causing a permanent stall.
--

--- Patch [5]: [PATCH v7 5/5] can: isotp: fix state machine corruption on signal interruption ---
commit 0650cd1b992a580e80131b223aa61fb92013fdb6
Author: Oliver Hartkopp <[email protected]>

can: isotp: fix state machine corruption on signal interruption

If a thread blocked in isotp_sendmsg() waiting for a previous transfer
to finish is interrupted by a signal, jumping to the cleanup labels
incorrectly cancels active timers and forces 'so->tx.state' back to
ISOTP_IDLE. This commit returns the error directly instead of jumping
to err_event_drop.

> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index fdb2109d7c27e..c9cd78df3a6e7 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
> @@ -1019,7 +1019,7 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
>  		/* wait for complete transmission of current pdu */
>  		err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
>  		if (err)
> -			goto err_event_drop;
> +			return err;
>  	}
>  
>  	/* new transfer: bump generation and drop any echo tag left behind

[Severity: High]
This isn't a bug introduced by this patch, but does the same problem exist
later in isotp_sendmsg() when wait_tx_done is true?

net/can/isotp.c:isotp_sendmsg() {
...
	if (wait_tx_done) {
		/* wait for complete transmission of current pdu */
		err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
		if (err)
			goto err_event_drop;

		err = sock_error(sk);
...
}

If a signal interrupts this second wait_event_interruptible() loop, it jumps
to err_event_drop, which unconditionally cancels timers before verifying
tx_gen ownership.

Additionally, is there a similar race window in the softirq handlers?
In isotp_rcv_fc() and isotp_rcv_echo(), the timer is unconditionally
cancelled after a lockless state check:

net/can/isotp.c:isotp_rcv_fc() {
...
	if (so->tx.state != ISOTP_WAIT_FC &&
	    so->tx.state != ISOTP_WAIT_FIRST_FC)
		return 0;

	hrtimer_cancel(&so->txtimer);
...
}

If the softirq checks the state but is preempted or delayed before calling
hrtimer_cancel(), could a concurrent new transfer start and have its timer
wrongfully cancelled, causing a permanent stall of the socket?

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