Re: [PATCH 1/2] can: isotp: use unconditional synchronize_rcu() in isotp_release()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Concurrent lockless access to TX state variables leads to buffer corruption and invalid frame transmission.
- [Medium] Concurrent socket release and netdevice unregister skips CAN filter unregistration, leading to a memory leak.
--

--- Patch [1]: [PATCH 1/2] can: isotp: use unconditional synchronize_rcu() in isotp_release() ---
commit fd9ce93db70bc0c54d097a98764dcfab2ecff812
Author: Oliver Hartkopp <socketcan@hartkopp.net>

can: isotp: use unconditional synchronize_rcu() in isotp_release()

This commit moves the synchronize_rcu() call in isotp_release() to run
unconditionally before timer cancellation. It ensures we always wait for any
in-flight isotp_rcv() callers to finish, preventing a use-after-free timer
callback race when NETDEV_UNREGISTER clears so->bound early.

Link: https://lore.kernel.org/linux-can/20260707085210.6B6C01F000E9@smtp.kernel.org/

> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index c48b4a818297e..d30937345bcdc 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
> @@ -1237,11 +1237,18 @@ static int isotp_release(struct socket *sock)
>  						  SINGLE_MASK(so->txid),
>  						  isotp_rcv_echo, sk);
>  				dev_put(dev);
> -				synchronize_rcu();
>  			}

[Severity: Medium]
This isn't a bug introduced by this patch, but earlier in isotp_release(),
there is a check for dev_get_by_index():

	if (so->bound) {
		if (so->ifindex) {
			struct net_device *dev;

			dev = dev_get_by_index(net, so->ifindex);
			if (dev) {
				if (isotp_register_rxid(so))
					can_rx_unregister(net, dev, so->rxid,
							  SINGLE_MASK(so->rxid),
							  isotp_rcv, sk);

If isotp_release() runs concurrently with a NETDEV_UNREGISTER event,
dev_get_by_index() can return NULL. When that happens, does this skip the
can_rx_unregister() calls and permanently leak the CAN filters?

>  		}
>  	}
>  
> +	/* Always wait for a grace period before touching the timers below.
> +	 * A concurrent NETDEV_UNREGISTER may have already unregistered our
> +	 * filters and cleared so->bound in isotp_notify() without waiting
> +	 * for in-flight isotp_rcv() callers to finish, so this call must not
> +	 * be skipped just because so->bound is already 0 here.
> +	 */
> +	synchronize_rcu();
> +
>  	hrtimer_cancel(&so->txfrtimer);
>  	hrtimer_cancel(&so->txtimer);

[Severity: High]
This is a pre-existing issue, but could there be a lockless state corruption
involving the txtimer?

If isotp_tx_timer_handler() executes on CPU A, it sets
so->tx.state = ISOTP_IDLE and wakes up waiters:

	so->tx.state = ISOTP_IDLE;
	wake_up_interruptible(&so->wait);

Concurrently, if isotp_rcv_fc() executes on CPU B, it calls hrtimer_cancel():

	hrtimer_cancel(&so->txtimer);

CPU B will spin waiting for the timer handler on CPU A to finish.

While CPU B spins, a blocked task in isotp_sendmsg() on CPU C can wake up,
see ISOTP_IDLE, and successfully execute the state exchange:

	while (cmpxchg(&so->tx.state, ISOTP_IDLE, ISOTP_SENDING) != ISOTP_IDLE) {

CPU C will then begin writing into so->tx.buf.

When the timer handler on CPU A finishes, hrtimer_cancel() on CPU B returns.
isotp_rcv_fc() then unconditionally sets the state to ISOTP_SENDING and calls
isotp_send_cframe():

	case ISOTP_FC_CTS:
		so->tx.bs = 0;
		so->tx.state = ISOTP_SENDING;
		...
		isotp_send_cframe(so);

Does this lead to isotp_send_cframe() concurrently reading from so->tx.buf
while isotp_sendmsg() is still writing to it?

>  	hrtimer_cancel(&so->rxtimer);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710-isotp-fixes-v1-0-75a1d11d7df9@hartkopp.net?part=1