Re: [PATCH net-next 11/11] can: protect the receive lists with a per-device lock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] can: raw/isotp: race in bind() overwrites ENODEV with ENETDOWN on unbind
- [Medium] can: isotp: premature so->dev publication in isotp_bind() races with NETDEV_DOWN
--

commit 8449957dc1b032324bb9194614a09818da738dd0
Author: Oliver Hartkopp <socketcan@hartkopp.net>

can: protect the receive lists with a per-device lock

This commit adds a per-device spinlock to struct can_dev_rcv_lists so that
CAN receive list registrations and unregistrations are protected
independently of network namespace moves. This allows CAN_RAW, CAN_BCM, and
CAN_ISOTP to manage their receive filters under lock_sock() without holding
rtnl_lock().

Link: https://lore.kernel.org/linux-can/CEA6A38A-2646-4ADA-95B4-CBAE2F301A8E@doyensec.com/
Link: https://patch.msgid.link/20261020203257.230955-6-socketcan@hartkopp.net

> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index 5e1c3971ebfce..8ec37d252557e 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c

[ ... ]

> @@ -1614,7 +1612,17 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
>  		goto out;
>  	}
>  
> -	dev = dev_get_by_index(net, addr->can_ifindex);
> +	/* Publish the device inside the RCU section of the lookup: When it
> +	 * is unlisted concurrently, either isotp_notify() sees so->dev after
> +	 * synchronize_net() or the lookup fails.
> +	 */
> +	rcu_read_lock();
> +	dev = dev_get_by_index_rcu(net, addr->can_ifindex);
> +	if (dev) {
> +		dev_hold(dev);
> +		WRITE_ONCE(so->dev, dev);
> +	}
> +	rcu_read_unlock();
>  	if (!dev) {
>  		err = -ENODEV;
>  		goto out;
> @@ -1627,7 +1635,12 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
>  		err = -EINVAL;
>  		goto out_put_dev;
>  	}
> -	if (!(dev->flags & IFF_UP))
> +
> +	/* Either isotp_notify() sees so->dev on NETDEV_DOWN or we see the
> +	 * cleared IFF_UP, pairs with isotp_notify()
> +	 */
> +	smp_mb();
> +	if (!(READ_ONCE(dev->flags) & IFF_UP))
>  		notify_enetdown = 1;
>  
>  	ifindex = dev->ifindex;
> @@ -1653,15 +1666,17 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
>  	 * can safely reach the device later, even if a concurrent
>  	 * NETDEV_UNREGISTER has already unlisted it by ifindex.
>  	 */
> -	so->dev = dev;
>  	netdev_hold(so->dev, &so->dev_tracker, GFP_KERNEL);
>  
>  out_put_dev:
> -	/* remove potential reference from dev_get_by_index() */
> +	/* the published so->dev is only valid for a successful bind() */
> +	if (err)
> +		WRITE_ONCE(so->dev, NULL);

[Severity: Medium]
In net/can/isotp.c, isotp_bind() publishes so->dev inside the RCU read-side
critical section before validating can_get_ml_priv(dev) and
READ_ONCE(dev->mtu) < so->ll.mtu:

	rcu_read_lock();
	dev = dev_get_by_index_rcu(net, addr->can_ifindex);
	if (dev) {
		dev_hold(dev);
		WRITE_ONCE(so->dev, dev);
	}
	rcu_read_unlock();
	...
	if (READ_ONCE(dev->mtu) < so->ll.mtu) {
		err = -EINVAL;
		goto out_put_dev;
	}

Unlike raw_bind(), which publishes ro->bind_dev early (used only for
NETDEV_UNREGISTER under lock_sock(sk)) and only sets ro->dev once validation
and filter registration succeed, isotp_notify() checks READ_ONCE(so->dev)
locklessly for NETDEV_DOWN:

	if (READ_ONCE(so->dev) != dev)
		return;

	switch (msg) {
	...
	case NETDEV_DOWN:
		sk->sk_err = ENETDOWN;
		if (!sock_flag(sk, SOCK_DEAD))
			sk_error_report(sk);
		break;
	}

If isotp_bind() fails the MTU check (for example when CAN_ISOTP_LL_OPTS sets
ll.mtu = CANFD_MTU and the target device has dev->mtu = CAN_MTU) while a
concurrent NETDEV_DOWN notifier runs on that device before out_put_dev clears
so->dev, won't isotp_notify() observe READ_ONCE(so->dev) == dev and set
sk->sk_err = ENETDOWN on the still-unbound socket?

Would checking can_get_ml_priv(dev) and READ_ONCE(dev->mtu) < so->ll.mtu
inside rcu_read_lock() before writing so->dev avoid this window?

> +
> +	/* remove reference from the device lookup */
>  	dev_put(dev);
>  out:
>  	release_sock(sk);
> -	rtnl_unlock();
>  
>  	if (notify_enetdown) {
>  		sk->sk_err = ENETDOWN;
> @@ -1864,12 +1879,21 @@ static void isotp_notify(struct isotp_sock *so, unsigned long msg,
>  	if (!net_eq(dev_net(dev), sock_net(sk)))
>  		return;
>  
> -	if (so->dev != dev)
> +	if (msg == NETDEV_DOWN)
> +		smp_mb(); /* pairs with isotp_bind(), see raw_notify() */
> +
> +	/* lockless fast path, see raw_notify() */
> +	if (READ_ONCE(so->dev) != dev)
>  		return;
>  
>  	switch (msg) {
>  	case NETDEV_UNREGISTER:
>  		lock_sock(sk);
> +		if (so->dev != dev) {
> +			release_sock(sk);
> +			break;
> +		}
> +
>  		/* remove current filters & unregister */
>  		if (so->bound) {
>  			if (isotp_register_rxid(so))
> @@ -1885,7 +1909,7 @@ static void isotp_notify(struct isotp_sock *so, unsigned long msg,
>  
>  		so->ifindex = 0;
>  		so->bound  = 0;
> -		so->dev = NULL;
> +		WRITE_ONCE(so->dev, NULL);
>  		release_sock(sk);
>  
>  		sk->sk_err = ENODEV;
> diff --git a/net/can/raw.c b/net/can/raw.c
> index dfea48768b231..d07f3bb68cd5c 100644
> --- a/net/can/raw.c
> +++ b/net/can/raw.c

[ ... ]

> @@ -305,16 +309,31 @@ static void raw_notify(struct raw_sock *ro, unsigned long msg,
>  		       struct net_device *dev)
>  {
>  	struct sock *sk = &ro->sk;
> +	struct net_device *bind_dev;
>  
>  	if (!net_eq(dev_net(dev), sock_net(sk)))
>  		return;
>  
> -	if (ro->dev != dev)
> +	if (msg == NETDEV_DOWN)
> +		smp_mb(); /* IFF_UP is cleared before, pairs with raw_bind() */
> +
> +	/* Lockless fast path, ro->bind_dev covers a concurrent bind().
> +	 * Read it before ro->dev, pairs with smp_store_release() in
> +	 * raw_bind(). A stale ro->bind_dev only costs a lock_sock().
> +	 */
> +	bind_dev = smp_load_acquire(&ro->bind_dev);
> +	if (READ_ONCE(ro->dev) != dev &&
> +	    (msg != NETDEV_UNREGISTER || bind_dev != dev))
>  		return;
>  
>  	switch (msg) {
>  	case NETDEV_UNREGISTER:
>  		lock_sock(sk);
> +		if (ro->dev != dev) {
> +			release_sock(sk);
> +			break;
> +		}
> +
>  		/* remove current filters & unregister */
>  		if (ro->bound) {
>  			raw_disable_allfilters(dev_net(dev), dev, sk);
> @@ -326,7 +345,7 @@ static void raw_notify(struct raw_sock *ro, unsigned long msg,
>  
>  		ro->ifindex = 0;
>  		ro->bound = 0;
> -		ro->dev = NULL;
> +		WRITE_ONCE(ro->dev, NULL);
>  		ro->count = 0;
>  		release_sock(sk);
>  

[ ... ]

> @@ -528,16 +557,24 @@ static int raw_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int len
>  		ro->ifindex = ifindex;
>  		ro->bound = 1;
>  		/* bind() ok -> hold a reference for new ro->dev */
> -		ro->dev = dev;
> +		WRITE_ONCE(ro->dev, dev);
>  		netdev_hold(ro->dev, &ro->dev_tracker, GFP_KERNEL);
> +
> +		if (dev) {
> +			/* Either raw_notify() sees ro->dev on NETDEV_DOWN or
> +			 * we see the cleared IFF_UP, pairs with raw_notify()
> +			 */
> +			smp_mb();
> +			if (!(READ_ONCE(dev->flags) & IFF_UP))
> +				notify_enetdown = 1;
> +		}
>  	}
>  
>  out_put_dev:
> -	/* remove potential reference from dev_get_by_index() */
> +	/* remove reference from the device lookup */
>  	dev_put(dev);
>  out:
>  	release_sock(sk);
> -	rtnl_unlock();
>  
>  	if (notify_enetdown) {
>  		sk->sk_err = ENETDOWN;

[Severity: Medium]
Can raw_bind() and isotp_bind() overwrite sk->sk_err = ENODEV with ENETDOWN
when racing against a concurrent NETDEV_UNREGISTER?

During unregister_netdevice_many_notify() or __dev_change_net_namespace(),
netif_close() clears IFF_UP before unlist_netdevice(dev), synchronize_net(),
and call_netdevice_notifiers(NETDEV_UNREGISTER, dev).

If raw_bind() or isotp_bind() looks up and publishes the device just before
unlist_netdevice(dev), the following sequence can occur:

1. Under lock_sock(sk), raw_bind() or isotp_bind() sees !(READ_ONCE(dev->flags) & IFF_UP)
   and sets notify_enetdown = 1.
2. Concurrent raw_notify() or isotp_notify() for NETDEV_UNREGISTER sees the
   published ro->bind_dev or so->dev and waits on lock_sock(sk).
3. raw_bind() or isotp_bind() calls release_sock(sk).
4. raw_notify() or isotp_notify() acquires lock_sock(sk), removes the
   filters, resets bound = 0 and dev = NULL, calls release_sock(sk), and sets
   sk->sk_err = ENODEV.
5. raw_bind() or isotp_bind() runs its post-unlock notify_enetdown block:

	release_sock(sk);

	if (notify_enetdown) {
		sk->sk_err = ENETDOWN;
		if (!sock_flag(sk, SOCK_DEAD))
			sk_error_report(sk);
	}

This clobbers sk->sk_err = ENODEV with ENETDOWN on a socket that is now
unbound. For CAN_RAW sockets in particular, raw_recvmsg() does not check
ro->bound and relies on sk->sk_err = ENODEV to report that the device went
away, so overwriting ENODEV with ENETDOWN leaves a subsequent blocking
recvmsg() waiting on an unbound socket with no filters.

Should sk->sk_err and sk_error_report(sk) be updated while still holding
lock_sock(sk) in raw_bind(), isotp_bind(), raw_notify(), and isotp_notify()?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021102756.1900679-1-mkl@pengutronix.de?part=11