Re: [PATCH v9] 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:
- [Medium] A 24-bit generation counter wrap-around combined with uninitialized tx_result slots can lead to falsely reporting success for timed-out transfers.
--

--- Patch [1]: [PATCH v9] can: isotp: fix timer drain order, wakeup handling and tx_gen ordering ---
commit ff6af136ee9e3a0bf07e3a23e40b4fe8fbb8be75
Author: Oliver Hartkopp <socketcan@hartkopp.net>

can: isotp: fix timer drain order, wakeup handling and tx_gen ordering

This patch addresses various lock-free state transition and timer handling
issues in the ISOTP TX path. It introduces a generation counter and packed
result slots to safely pass transmission status back to the sender.

> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index 54becaf6898f1..09c0ee0fc9f3b 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
> @@ -177,6 +187,52 @@ static LIST_HEAD(isotp_notifier_list);

[ ... ]

> +/* fetch the result recorded for 'gen', as a (negative) errno (0 for success) */
> +static int isotp_get_tx_result(struct isotp_sock *so, u32 gen)
> +{
> +	u32 result = READ_ONCE(so->tx_result[gen % ISOTP_TX_RESULT_SLOTS]);
> +
> +	if (isotp_get_tx_gen(result) != gen) {
> +		pr_notice_once("can-isotp: tx_result[] slot reused before read\n");
> +
> +		/* report failure rather than risk a false success */
> +		return -ECOMM;
> +	}
> +
> +	return -(isotp_get_tx_err(result));
> +}

[Severity: Medium]
Can this stale check pass incorrectly if the 24-bit generation counter wraps
around?

If a previous transfer succeeded, this slot will hold a success value.
If a user subsequently performs exactly 16,777,216 failed transfers
(for example, hitting local ENOBUFS errors that increment the generation
but never update the result slot), the generation counter will wrap back
to this exact value, making the stale success value look like a valid result
for the current generation.

[ ... ]

> @@ -946,10 +1017,14 @@ static void isotp_rcv_echo(struct sk_buff *skb, void *data)

[ ... ]

>  static enum hrtimer_restart isotp_tx_timeout(struct isotp_sock *so)
>  {
> @@ -965,14 +1040,14 @@ static enum hrtimer_restart isotp_tx_timeout(struct isotp_sock *so)
>  	if (cmpxchg(&so->tx.state, old_state, ISOTP_IDLE) != old_state)
>  		return HRTIMER_NORESTART;
>  
> -	/* we did not get any flow control or echo frame in time */
> +	/* detected timeout: report 'communication error on send' */
>  
> -	if (READ_ONCE(so->tx_gen) == gen) {
> -		/* report 'communication error on send' */
> -		sk->sk_err = ECOMM;
> -		if (!sock_flag(sk, SOCK_DEAD))
> -			sk_error_report(sk);
> -	}
> +	/* a stale read of this slot by a waiter still falls back to ECOMM */
> +	isotp_set_tx_result(so, gen, ECOMM);

[Severity: Medium]
Does this order of operations allow a timed-out transfer to incorrectly return
success?

The state is updated to ISOTP_IDLE via cmpxchg before the result is set.
Updating the state to ISOTP_IDLE wakes up the waiting process in
isotp_sendmsg(), which then reads the result slot.

If the generation counter wrapped around as described above, and the waiter
reads the slot before isotp_set_tx_result() writes the ECOMM error, it seems
the waiter will read the perfectly matching stale success value.

[ ... ]

> @@ -1046,29 +1123,32 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)

[ ... ]

> -	/* 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 = isotp_inc_tx_gen(READ_ONCE(so->tx_gen));
> +	WRITE_ONCE(so->tx_gen, my_gen);
> +	smp_wmb(); /* pairs with the cmpxchg() in isotp_tx_timeout() */
> +	WRITE_ONCE(so->tx.state, ISOTP_SENDING);

[Severity: Medium]
Should the result slot be cleared when a new transfer claims a generation?

Since the new generation is claimed here, setting the corresponding result slot
to an uninitialized or pending state would ensure that a stale success value
from a wrapped-around generation counter cannot be mistakenly read by the
waiter on a timeout.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721161350.33874-1-socketcan@hartkopp.net?part=1