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

Pre-existing issues:
- [High] A stale local echo from an interrupted transfer can prematurely complete a new transfer if their first 3 payload bytes match.
- [Medium] Missing READ_ONCE()/WRITE_ONCE() for lock-free accesses to `so->cfecho` causes data races.
--

--- Patch [1]: [PATCH v2] can: isotp: fix timer drain order, wakeup handling and tx_gen ordering ---
commit f9db7885e3deec5f74c17326758f7a0025ece72d
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:

[ ... ]

> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index 54becaf6898f1..ff6299f5c4a0b 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
> @@ -899,32 +901,32 @@ static void isotp_rcv_echo(struct sk_buff *skb, void *data)
>  	spin_lock(&so->rx_lock);
>  
>  	/* so->cfecho may since belong to a new transfer; recheck under lock */
> -	if (so->cfecho != *(u32 *)cf->data)
> +	if (READ_ONCE(so->cfecho) != *(u32 *)cf->data)
>  		goto out_unlock;

[Severity: High]
This is a pre-existing issue, but can a stale local echo prematurely complete
a new transfer if their first 4 payload bytes match?

If a transmission (SF or CF-broadcast) is interrupted by a signal,
isotp_sendmsg() cancels timers, sets the state to IDLE, and returns, leaving
a pending loopback echo in the network queue. 

If the user immediately retries sendmsg with the exact same payload, won't
this stale echo be processed by isotp_rcv_echo() and match the new so->cfecho
tag, since the tag is derived solely from the first 4 bytes of the CAN frame?

This seems like it could cause the sender to falsely believe the transfer is
complete while it may still be in progress, violating the
CAN_ISOTP_WAIT_TX_DONE guarantee.

>  
>  	/* cancel local echo timeout */
>  	hrtimer_cancel(&so->echotimer);
>  
>  	/* local echo skb with consecutive frame has been consumed */
> -	so->cfecho = 0;
> +	WRITE_ONCE(so->cfecho, 0);

[ ... ]

> @@ -1046,29 +1052,32 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
>  		if (msg->msg_flags & MSG_DONTWAIT)
>  			return -EAGAIN;
>  
> -		if (so->tx.state == ISOTP_SHUTDOWN)
> +		if (READ_ONCE(so->tx.state) == ISOTP_SHUTDOWN)
>  			return -EADDRNOTAVAIL;
>  
>  		/* wait for complete transmission of current pdu */
>  		err = wait_event_interruptible(so->wait,
> -					       so->tx.state == ISOTP_IDLE);
> +					       READ_ONCE(so->tx.state) == ISOTP_IDLE ||
> +					       READ_ONCE(so->tx.state) == ISOTP_SHUTDOWN);
>  		if (err)
>  			return err;
>  	}
>  
> -	/* new transfer: bump so->tx_gen and drain the old one's timers,
> -	 * still under the so->rx_lock we just claimed the socket with
> -	 */
> -	WRITE_ONCE(so->tx.state, ISOTP_SENDING);
> -	WRITE_ONCE(so->tx_gen, READ_ONCE(so->tx_gen) + 1);
> +	/* txfrtimer's callback re-arms echotimer lock-free: drain it first */
> +	hrtimer_cancel(&so->txfrtimer);
>  	hrtimer_cancel(&so->txtimer);
>  	hrtimer_cancel(&so->echotimer);
> -	hrtimer_cancel(&so->txfrtimer);
> -	so->cfecho = 0;
> +
> +	/* new transfer: increment so->tx_gen and set tx.state after barrier */
> +	my_gen = READ_ONCE(so->tx_gen) + 1;
> +	WRITE_ONCE(so->tx_gen, my_gen);
> +	smp_wmb(); /* pairs with the cmpxchg() in isotp_tx_timeout() */
> +	WRITE_ONCE(so->tx.state, ISOTP_SENDING);
> +	WRITE_ONCE(so->cfecho, 0);

[Severity: Medium]
This is a pre-existing issue, but does this miss some READ_ONCE() and
WRITE_ONCE() annotations for lock-free accesses to so->cfecho in
isotp_sendmsg() and isotp_bind()?

While the patch aligns many accesses, isotp_sendmsg() sets the transmission
state to ISOTP_SENDING before so->cfecho is initialized and written for
SF-mode and CF-broadcast. Because the state is already ISOTP_SENDING, a
concurrently arriving stale echo can trigger isotp_rcv_echo(), which reads
so->cfecho via READ_ONCE(). 

However, isotp_sendmsg() still performs plain reads and writes later in the
function:

    if (so->cfecho)
        pr_notice_once("can-isotp: uninit cfecho %08X\n", so->cfecho);

    /* ... */
    so->cfecho = *(u32 *)cf->data;

Similarly, isotp_bind() performs a plain write to so->cfecho = 0; while
isotp_rcv_echo() could still be processing an in-flight echo. 

Could this lead to data races or torn reads?

>  	spin_unlock_bh(&so->rx_lock);
>  
>  	/* so->bound is only checked once above - a wakeup may have
> -	 * unbound/rebound the socket meanwhile, so re-validate it
> +	 * unbound/rebound the socket meanwhile => recheck
>  	 */
>  	if (!so->bound) {
>  		err = -EADDRNOTAVAIL;

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