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

On 30.07.26 12:29, [email protected] wrote:
> 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, 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?
> 

No.

sk_err is a single per-socket slot shared by the RX and TX paths, so it 
cannot cleanly separate a TX result from a concurrent RX error.

Note that a plain unreliable datagram protocol like UDP/IP gives no 
feedback for bad or missing received data at all - it just silently 
drops it. ISO-TP does a bit more and surfaces RX problems 
(EBADMSG/EILSEQ/ETIMEDOUT) via sk_err on a best-effort basis, which is 
fine: for an unreliable protocol these are ultimately caught by timeouts 
at a higher ISO layer anyway.

What this patch fixes to 100% is the send path's POSIX contract: 
sendmsg() now returns the TX-specific result from tx_result[] (>0 bytes 
on success, <0 on a real TX error), instead of returning whatever 
happened to sit in sk_err - which could misreport an unrelated 
concurrent RX error as the TX outcome. Draining sk_err afterwards is 
required in CAN_ISOTP_WAIT_TX_DONE mode, since the result is delivered 
synchronously via the return value.

So the send path is now correct and race-free; the shared-sk_err RX 
notification stays best-effort by design. Truly separating the two would 
need distinct RX/TX error channels (e.g. MSG_ERRQUEUE), which is a 
feature, not a fix, and out of scope here.

Best regards,
Oliver