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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 2 · Low: 1 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] After this patch, CAN_RAW serializes bind/setsockopt/release
  against NETDEV_UNREGISTER only with lock_sock().
- [Medium] can: raw/isotp: race in bind() overwrites ENODEV with ENETDOWN
  on unbind
- [Low] isotp_bind() now publishes so->dev with WRITE_ONCE(so->dev, dev)
  inside the RCU lookup (net/can/isotp.c:1623).

Pre-existing issues:
- [Low] isotp_bind() calls can_rx_register() twice: for rx_id when
  isotp_register_rxid(so) is true (net/can/isotp.c:1648-1650), and for…

--

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

This commit adds a spinlock to struct can_dev_rcv_lists. The receive lists
of a CAN device are now protected per device instead of by the per-netns
rcvlists_lock, so can_rx_unregister() can find subscriptions after the
device has moved to another namespace.

RTNL is dropped from raw_bind(), raw_setsockopt(), raw_release(),
isotp_bind() and isotp_release(). These paths now use lock_sock() and a
device pointer published under RCU to serialize against the netdev
notifier.

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;
>  	}

[Severity: Low]
so->dev is now visible to the lockless check in isotp_notify() before the
remaining checks in isotp_bind() run, and before so->bound is set. The MTU
check can still fail the bind:

	if (READ_ONCE(dev->mtu) < so->ll.mtu) {
		err = -EINVAL;
		goto out_put_dev;
	}

so->dev is only cleared again at out_put_dev.

isotp_bind() no longer holds RTNL. Can a NETDEV_DOWN for the same device
run isotp_notifier()->isotp_notify() inside this window?

The NETDEV_DOWN case only checks READ_ONCE(so->dev) != dev. It does not
look at so->bound or recheck under lock_sock():

	case NETDEV_DOWN:
		sk->sk_err = ENETDOWN;
		if (!sock_flag(sk, SOCK_DEAD))
			sk_error_report(sk);
		break;

Would this set a stale ENETDOWN on a socket that was never bound, whose
bind() then returns -EINVAL? A later successful bind() to another device
(after fixing ll.mtu with CAN_ISOTP_LL_OPTS) would then get that error from
the next recvmsg() or poll().

raw_bind() avoids this. It only publishes ro->bind_dev early, which
raw_notify() uses for NETDEV_UNREGISTER only, and it sets ro->dev after
success. Could isotp use the same approach?

> @@ -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);

[Severity: Low]
This isn't a bug introduced by this patch, but isotp_bind() ignores the
return values of both can_rx_register() calls just above this:

	if (isotp_register_rxid(so))
		can_rx_register(net, dev, rx_id, SINGLE_MASK(rx_id),
				isotp_rcv, sk, sock_i_ino(sk), sk);
	...
	can_rx_register(net, dev, tx_id, SINGLE_MASK(tx_id),
			isotp_rcv_echo, sk, sock_i_ino(sk), sk);

It then sets so->bound = 1 unconditionally.

can_rx_register() returns -ENOMEM if kmem_cache_alloc(rcv_cache,
GFP_KERNEL) fails. Can the socket then report a successful bind() while it
is missing its FC/RX receiver or its echo receiver?

Later, isotp_release() and isotp_notify() would call can_rx_unregister()
for an entry that was never added and hit the "receive list entry not
found" warning. raw_enable_filters() and bcm_rx_setup() check this return
value and unwind.

-ENODEV from the net_eq() check does not look reachable here.
isotp_notify() waits on lock_sock() for NETDEV_UNREGISTER, so dev_net(dev)
cannot change during the registrations. That leaves -ENOMEM as the only
error.

The earlier commit "can: replace ARPHRD_CAN type checks with
can_get_ml_priv()" in this series already notes that isotp_bind() ignores
these return values.

[ ... ]

> 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

[ ... ]

> @@ -750,18 +772,47 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
>  			  sockptr_t optval, unsigned int optlen)
>  {
>  	struct sock *sk = sock->sk;

[ ... ]

> -	rtnl_lock();
> +	if (optname == CAN_RAW_FILTER) {
> +		/* Copy the filters before lock_sock(), so a stalled copy can
> +		 * not block raw_notify() waiting for lock_sock() under RTNL.
> +		 */

[ ... ]

> +	}
> +
>  	lock_sock(sk);
>  
> -	err = raw_setsockopt_locked(sock, optname, optval, optlen);
> +	err = raw_setsockopt_locked(sock, optname, optval, optlen,
> +				    filter, &sfilter, count);

[Severity: Medium]
The comment says a stalled copy must not block raw_notify() while it waits
for lock_sock() under RTNL. The early copy only covers CAN_RAW_FILTER,
though. Do the other options still copy from user memory with lock_sock()
held?

In raw_setsockopt_locked(), CAN_RAW_ERR_FILTER still does:

	if (copy_from_sockptr(&err_mask, optval, optlen))
		return -EFAULT;

CAN_RAW_LOOPBACK, CAN_RAW_RECV_OWN_MSGS, CAN_RAW_FD_FRAMES,
CAN_RAW_XL_FRAMES, CAN_RAW_XL_VCID_OPTS and CAN_RAW_JOIN_FILTERS do the
same after lock_sock(sk) is taken here.

raw_getsockopt(CAN_RAW_FILTER) also writes to the user buffer with
lock_sock() held:

	lock_sock(sk);
	...
		if (copy_to_iter(ro->filter, len,
				 &opt->iter_out) != len)
			err = -EFAULT;

Suppose one of these copies stalls on a FUSE-backed mapping while the
socket is bound, or while ro->bind_dev matches the device. Would a
NETDEV_UNREGISTER of that device then leave raw_notify() sleeping in
lock_sock() with RTNL held?

raw_notifier()
  raw_notify()
    lock_sock(sk)   <- waits for the stalled setsockopt/getsockopt

The unregister could come from ip link del of a vcan device in the user's
own network namespace, a namespace move, or a USB unplug.

The new ro->bind_dev hint also adds cases where raw_notify() takes
lock_sock(): a stale bind_dev that matches the unregistered device leads
there too.

Before this patch, raw_setsockopt() held rtnl_lock() itself across these
copies, so overall exposure is lower than before. The new design relies on
lock_sock() never being held across a user copy, though. Should the
remaining copies in raw_setsockopt_locked() and raw_getsockopt() also move
out of the locked section?

isotp_setsockopt() has the same pattern against isotp_notify(), since every
option is copied under lock_sock():

	lock_sock(sk);
	ret = isotp_setsockopt_locked(sock, level, optname, optval, optlen);
	release_sock(sk);

That code predates this patch. With RTNL gone from isotp_bind(), however,
bind relies on lock_sock() alone to serialize with isotp_notify().

[ ... ]

> @@ -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]
With RTNL gone from raw_bind(), can a concurrent NETDEV_UNREGISTER now
leave the socket reporting ENETDOWN instead of ENODEV?

unregister_netdevice_many_notify() and __dev_change_net_namespace()
close the device first, which clears IFF_UP. Only after that do they
call unlist_netdevice(), synchronize_net() and the NETDEV_UNREGISTER
notifier.

Suppose raw_bind() looks up the device before it is unlisted. It then
publishes ro->bind_dev, sees IFF_UP already cleared, and sets
notify_enetdown. raw_notify() for NETDEV_UNREGISTER matches
ro->bind_dev and sleeps in lock_sock() with RTNL held.

Once raw_bind() calls release_sock(), both threads write sk->sk_err
outside the socket lock:

raw_notify()                    raw_bind()
  lock_sock(sk)
  unbind, ro->dev = NULL
  release_sock(sk)
  sk->sk_err = ENODEV
                                  sk->sk_err = ENETDOWN
                                  sk_error_report(sk)

sk_err holds a single value, so the later write wins and the ENODEV is
lost. Userspace then sees ENETDOWN on a socket that is no longer bound
and has ro->count == 0 with no filters registered.

An application that treats ENETDOWN as "interface is down, wait for it
to come back" keeps blocking in recvmsg(). It never learns that the
device is gone and that it has to rebind.

The old code also wrote sk_err after rtnl_unlock(). There,
NETDEV_UNREGISTER could only start after bind() dropped RTNL, and the
synchronize_net() before the notifier made this ordering practically
unreachable. With this patch the notifier is already queued on
lock_sock() and runs as soon as bind() releases it.

isotp_bind() and isotp_notify() have the same pattern. so->dev is
published early and IFF_UP is already clear, so notify_enetdown gets
set. isotp_notify() then writes ENODEV after its release_sock(), while
isotp_bind() writes ENETDOWN after its own.

Would it work to set sk_err and call sk_error_report() before
release_sock() in both bind() paths and in the NETDEV_UNREGISTER case
of raw_notify() and isotp_notify()? The reported error would then
follow lock_sock() ownership. Or do you prefer another way to keep the
ENODEV from being clobbered?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261021102756.1900679-2-mkl%40pengutronix.de