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

On 21.10.26 22:55, Jakub Kicinski wrote:

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
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/ (local)
Link: https://patch.msgid.link/20261020203257.230955-6-socketcan@hartkopp.net (local)

quoted

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

[ ... ]

quoted

@@ -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?

Yes, changing the order here to

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

is a good hint from sashiko-bot and follows the raw_bind() concept.
@Jakub: I've sent a V6 only for this single patch 11/11 here:

Is this ok for you? I think Marc is offline and can not re-spin the PR right now.

https://lore.kernel.org/netdev/20261021215456.119268-1-socketcan@hartkopp.net/ (local)

quoted

@@ -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.

Right.

This is a pre-existing issue and will be fixed separately. With the
can_get_ml_priv() check in isotp_bind() and the namespace change being
serialized via lock_sock() against isotp_notify(), the only remaining
error from can_rx_register() is -ENOMEM from the GFP_KERNEL allocation
of the receiver. The fix will be addressed in the next net-next cycle.
As this is an improvement for a very unlikely allocation failure, it is
not intended for stable.

[ ... ]

quoted

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

[ ... ]

quoted

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

[ ... ]

quoted

-	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.
+		 */

[ ... ]

quoted

+	}
+
  	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().

Right, the early copy in raw_setsockopt() only covers CAN_RAW_FILTER.
Before this patch, raw_setsockopt() held RTNL itself across all user
space copies, so this patch already improves the current situation. The
complete rework that moves all remaining user space copies in raw.c and
isotp.c out of lock_sock() will be addressed in the next net-next cycle.
This is an improvement and not intended for stable.

Best regards,
Oliver