Re: [PATCH net 03/20] 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:
- [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 <[email protected]>
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/[email protected]

> 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, str=
uct msghdr *msg, size_t size)
[ ... ]
>  	if (wait_tx_done) {
> -		/* wait for complete transmission of current pdu */
> -		err =3D wait_event_interruptible(so->wait, so->tx.state =3D=3D ISOTP_I=
DLE);
> +		/* 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 =3D wait_event_interruptible(so->wait,
> +					       isotp_tx_gen_done(so, my_gen));
>  		if (err)
>  			goto err_event_drop;
> =20
> -		err =3D sock_error(sk);
> -		if (err)
> -			return err;
> +		/* still our claim, but isotp_release() force-shut it down */
> +		if (smp_load_acquire(&so->tx.state) =3D=3D ISOTP_SHUTDOWN &&
> +		    READ_ONCE(so->tx_gen) =3D=3D my_gen) {
> +			err =3D -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 =3D 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?=20

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;
>  	}
> =20
>  	return size;

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260729102802.5051=
[email protected]?part=3D3