Re: [PATCH v2] can: isotp: use unconditional synchronize_rcu() in isotp_release()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Skipped CAN filter unregistration during concurrent device removal leads to Use-After-Free and memory leak.
--

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

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

This commit addresses a race condition in the timer callback of isotp
sockets. It enforces an unconditional synchronize_rcu() in isotp_release()
before cancelling timers to avoid issues during concurrent
NETDEV_UNREGISTER notifications.

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

> diff --git a/net/can/isotp.c b/net/can/isotp.c
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c

[ ... ]

> @@ -1227,14 +1227,20 @@ static int isotp_release(struct socket *sock)
>  	/* remove current filters & unregister */
>  	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);

[Severity: Critical]
This is a pre-existing issue, but could this dev_get_by_index() call return
NULL if a device unregistration is running concurrently?

If unregister_netdevice_many() unlists the device before the
NETDEV_UNREGISTER notifier executes, and isotp_release() is called at the
same time, it removes the socket from the isotp notifier list.

When it calls dev_get_by_index() here, it will return NULL because the
device is unlisted. The if (dev) condition fails and we skip calling
can_rx_unregister().

The NETDEV_UNREGISTER notifier will miss this socket because it was already
removed from the notifier list.

Since the socket is freed at the end of this function, does this leave an
active CAN filter pointing to a freed socket? Any delayed or looped-back
CAN frames processed during device teardown could invoke isotp_rcv() on
this freed socket, causing a use-after-free and a memory leak.

>  
>  				can_rx_unregister(net, dev, so->txid,
>  						  SINGLE_MASK(so->txid),
>  						  isotp_rcv_echo, sk);
>  				dev_put(dev);
> -				synchronize_rcu();
>  			}
>  		}
>  	}
>  
> +	/* Always wait for a grace period before touching the timers below.
> +	 * A concurrent NETDEV_UNREGISTER may have already unregistered our

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