Re: [PATCH v3] 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] Concurrent `isotp_sendmsg` calls with `CAN_ISOTP_WAIT_TX_DONE` cause threads to block on subsequent transfers and swap error statuses.
--

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

This patch is a follow-up to a previous commit fixing several issues with timer
draining, wakeup handling, and tx_gen ordering in the ISOTP protocol. It also
aligns state accesses and uses skb->hash for echo frames.

> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index 54becaf6898f1..3e5800f440579 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
[ ... ]
> @@ -1230,10 +1246,18 @@ 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);
> +		err = wait_event_interruptible(so->wait,
> +					       READ_ONCE(so->tx.state) == ISOTP_IDLE ||
> +					       READ_ONCE(so->tx.state) == ISOTP_SHUTDOWN);

[Severity: High]
This isn't a bug introduced by this patch, but does checking the global socket
state here cause threads to incorrectly sleep if a concurrent transfer claims
the socket before this condition is evaluated?

If Thread A and B both call isotp_sendmsg() concurrently on the same socket
and Thread A finishes its transfer and wakes up both threads, Thread B might
claim the socket and set tx.state = ISOTP_SENDING before Thread A evaluates
this condition. In that case, Thread A sees ISOTP_SENDING and incorrectly goes
back to sleep waiting for Thread B's transfer to finish.

>  		if (err)
>  			goto err_event_drop;
[ ... ]
> @@ -1246,15 +1270,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;
> +	}

[Severity: High]
This is a pre-existing issue, but when a thread is interrupted while waiting
on a subsequent transfer, it returns an error here instead of returning
success for its own original completed transfer.

Following the scenario above, if Thread A is later woken by Thread B's failure,
could Thread A incorrectly consume Thread B's error via sock_error(), returning
it, while Thread B subsequently gets 0 and incorrectly returns success?

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