Re: [PATCH can test v11a] 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] Silent loss of asynchronous RX socket errors (e.g., timeouts, malformed PDUs) due to unconditionally draining `sk->sk_err` and discarding its value in the `wait_tx_done` path.
--

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

This commit addresses several issues in the CAN ISOTP subsystem including timer
drain ordering, wakeup handling on shutdown, and tx_gen checking. It introduces
a generation-tagged result slot system to safely record transfer results.

> diff --git a/net/can/isotp.c b/net/can/isotp.c
> --- 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);
> +
> +		return err ? err : size;

[Severity: High]
Does discarding the return value of sock_error() here silently drop
asynchronous RX socket errors?

If a concurrent RX timeout or malformed PDU sets sk->sk_err to ETIMEDOUT
or EBADMSG, calling sock_error(sk) will unconditionally clear it to 0. Since
the returned value is ignored and the function prioritizes returning the TX
result or size, it seems the application will never be notified of these RX
failures via sendmsg(), poll(), or getsockopt(SO_ERROR).

Should we preserve or return this error if no TX error occurred?

>  	}
>  
>  	return size;
[ ... ]

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