Re: [PATCH net 03/20] can: isotp: fix timer drain order, wakeup handling and tx_gen ordering

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Silently dropped asynchronous RX socket errors in isotp_sendmsg() due to ignored return value of sock_error(sk).
--

--- Patch [3]: [PATCH net 03/20] can: isotp: fix timer drain order, wakeup handling and tx_gen ordering ---
commit 3e2ba28e6e9e34bb3ea9b3075735283b9c9490c0
Author: Oliver Hartkopp <socketcan@hartkopp.net>
can: isotp: fix timer drain order, wakeup handling and tx_gen ordering

This commit addresses several concurrent state and timer handling issues in
the ISOTP protocol. It introduces a generation counter and packed result
array to track transfer state accurately across lock-free accesses.

Link: https://patch.msgid.link/20260724181525.43556-1-socketcan@hartkopp.net

> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index 54becaf6898f1..1f11c66b343c8 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
[ ... ]
> @@ -1223,20 +1321,49 @@ 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()
> +		 * isotp_tx_gen_done() uses common tx.state/tx_gen read sequence
> +		 */
> +		err = wait_event_interruptible(so->wait,
> +					       isotp_tx_gen_done(so, my_gen));
>  		if (err)
>  			goto err_event_drop;
>  
> -		err = sock_error(sk);
> -		if (err)
> -			return err;
> +		/* still our claim, but isotp_release() force-shut it down */
> +		if (smp_load_acquire(&so->tx.state) == ISOTP_SHUTDOWN &&
> +		    READ_ONCE(so->tx_gen) == my_gen) {
> +			err = -EADDRNOTAVAIL;
> +			goto err_event_drop;
> +		}
> +
> +		/* own completion, or tx_gen moved on - either way this is
> +		 * what isotp_get_tx_result() recorded for my_gen
> +		 */
> +		err = isotp_get_tx_result(so, my_gen);
> +
> +		/* drain to avoid stale error for a later poll()/SO_ERROR */
> +		sock_error(sk);

[Severity: High]
Does this unconditionally clear and discard concurrent asynchronous socket
errors? 

By ignoring the return value of sock_error() here in isotp_sendmsg(), it
appears any unrelated RX errors (such as ETIMEDOUT set by
isotp_rx_timer_handler()) would be permanently lost without being reported
to the user.

Could this cause applications to hang in recvmsg() or poll() indefinitely
because they never see the RX connection failure?

> +
> +		return err ? err : size;
>  	}
>  
>  	return size;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729102802.505168-1-mkl@pengutronix.de?part=3