[RFC PATCH can-next] can: protect the receive lists with a per-device lock

[RFC PATCH can-next] can: protect the receive lists with a per-device lock

[Oliver Hartkopp]

The receive lists of a CAN device are located in the device's
can_ml_priv and travel with the device when it is moved to another
network namespace. But they are protected by the per-netns
rcvlists_lock. So can_rx_unregister() needs dev_net(dev) to pick the
right lock and silently returns when the passed namespace does not
match dev_net(dev) anymore.

isotp_release() and bcm_release() remove the socket from their notifier
list before they take any lock that serializes against the netdev
notifier chain, which runs under RTNL. A socket that leaves the list in
this window is skipped by isotp_notify() / bcm_notify() and has to
remove its CAN filters itself.

This fails when the bound device is moved to another network namespace
in this window: isotp_release() passes sock_net(sk) to
can_rx_unregister() and bcm_rx_unreg() reads dev_net(dev) without
holding RTNL, so can_rx_unregister() does not find the filters. As
can_rx_register() stores rcv->sk without taking a reference, the
remaining receivers point to the freed socket (isotp) or the freed op
(bcm) and travel with the device into the new namespace:

  BUG: KASAN: use-after-free in isotp_rcv+0x1570/0x24d0
  Read of size 1 at addr ffff888118130552 by task isotp_ns_uaf/578
   can_rcv_filter+0x4af/0x8c0
   can_receive+0x28d/0x3c0
   can_rcv+0x2a9/0x310
   __netif_receive_skb_one_core+0x21a/0x260
   process_backlog+0x210/0x760

raw_release() is not affected, as it removes the filters with
dev_net(ro->dev) under RTNL. But raw_release(), raw_setsockopt() and
raw_bind() hold RTNL while removing and adding up to CAN_RAW_FILTER_MAX
filters per socket. As this can be triggered repeatedly by unprivileged
users, e.g. by binding CAN_RAW sockets alternately to different CAN
devices, RTNL becomes a global bottleneck. Extending this RTNL usage to
isotp_release() and bcm_release() is therefore no option.

Instead fix the root cause and add a spinlock to struct
can_dev_rcv_lists, which protects the receive lists of a CAN device or
the 'any' CAN device lists of a namespace. can_rx_unregister() now finds
the subscription independently from the current namespace of the
device. The protocols hold a tracked reference to the device (ro->dev,
so->dev, op->rx_reg_dev), which keeps the device's receive lists
accessible. The per-netns rcvlists_lock only protects the receive list
statistics anymore. The callers pass the namespace that has been used
for can_rx_register() to keep these statistics balanced.

The lock of the CAN device receive lists is initialized in
can_set_ml_priv(), which is used by all CAN network devices to set up
their can_ml_priv.

With this, raw_release(), isotp_release(), bcm_release() and
raw_setsockopt() do not need RTNL anymore. A socket that leaves the
notifier list while its device changes the namespace removes its
filters itself, which now always works. The filters are removed under
lock_sock() only, with cond_resched() between the filters. The
existing busy-wait loops on the *_busy_notifier variables remain
unchanged.

raw_setsockopt() now reads dev->reg_state without RTNL. Use READ_ONCE()
for this check, which is only a shortcut now: When the device is
unregistered concurrently, raw_notify() waits for lock_sock() and
removes the filters that have been set by raw_setsockopt(). The filters
are copied from user space before lock_sock() is taken, so a stalled
copy can not hold lock_sock() and thereby block a concurrent
NETDEV_UNREGISTER that waits for lock_sock() under RTNL in raw_notify().

raw_bind() and isotp_bind() took RTNL to serialize the device lookup
and the binding with NETDEV_UNREGISTER, as raw_notify() and
isotp_notify() checked ro->dev and so->dev without lock_sock(). Now
raw_notify() and isotp_notify() check ro->dev and so->dev under
lock_sock() in the NETDEV_UNREGISTER case, and the device lookup in
raw_bind() and isotp_bind() is done under lock_sock() too. Both on
device unregistration and on a namespace change, unlist_netdevice() and
synchronize_net() are executed before NETDEV_UNREGISTER is sent. So
either the notifier gets lock_sock() first and a subsequent bind() can
not look up the device anymore, or bind() gets lock_sock() first and the
notifier finds the new binding afterwards and removes it. A bind() that
looked up the device just before unlist_netdevice() but takes
lock_sock() only after the notifier has run registers its filters on the
unlisted device; these are removed when netdev_wait_allrefs() rebroadcasts
NETDEV_UNREGISTER while waiting for the tracked device reference. With
this, raw_bind() and isotp_bind() do not need RTNL anymore. raw_bind()
removes the filters of the previously bound device with sock_net(sk),
which has been used for can_rx_register().

raw_notify() and isotp_notify() keep a lockless READ_ONCE() fast path
that skips sockets not bound to the vanishing device, so the notifier
(running under RTNL) only serializes against lock_sock() of the sockets
actually bound to this device, not against every CAN_RAW / CAN_ISOTP
socket in the namespace. ro->dev / so->dev are rechecked under
lock_sock() in the NETDEV_UNREGISTER case; a bind() that sets the
pointer right after the lockless check is caught by the
NETDEV_UNREGISTER rebroadcast described above.

The NETDEV_DOWN case only reports an error to the socket and keeps
checking ro->dev and so->dev locklessly. ro->dev and so->dev are read
with READ_ONCE() and written with WRITE_ONCE() where they are changed
in bind() and in the notifier. dev->flags is read with READ_ONCE() in
bind() as it is read without RTNL now.

Other users of can_rx_unregister() benefit as well: j1939 removes its
filter with dev_net(ndev). When the device has been moved to another
namespace after can_rx_register(), this picked the rcvlists_lock of the
new namespace to protect the device's receive lists. Now the receive
lists are always protected by the per-device lock. The receive list
statistics of the namespaces may still become imprecise in this case,
as before.

Measured with four CAN_RAW sockets each rebinding CAN_RAW_FILTER_MAX
(512) filters between two vcan devices for 15s, on a kernel with lockdep
and CONFIG_LOCK_STAT (which inflates the absolute numbers, so only the
comparison matters). Without this patch raw_bind() is an rtnl_mutex
contention point and the rebind rate is ~32/s; with it raw_bind() no
longer takes rtnl_mutex, the rate rises to ~99/s and the rtnl_mutex
contention from this workload disappears (rtnl_mutex contentions
1621 -> 213, rtnl_mutex.wait_lock 307 -> 0). An unrelated
ip link add/del running in parallel is no longer stalled behind the
filter churn.

Fixes: 20bab8b88baa ("can: isotp: fix use-after-free race with concurrent NETDEV_UNREGISTER")
Fixes: 8e8cda6d737d ("can: initial support for network namespaces")
Reported-by: Norbert Szetei <norbert@doyensec.com>
Link: https://lore.kernel.org/linux-can/CEA6A38A-2646-4ADA-95B4-CBAE2F301A8E@doyensec.com/
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 include/linux/can/can-ml.h |   3 +
 include/net/netns/can.h    |   2 +-
 net/can/af_can.c           |  37 ++++++----
 net/can/bcm.c              |  49 +++++--------
 net/can/isotp.c            |  24 ++++---
 net/can/raw.c              | 140 ++++++++++++++++++++++++-------------
 6 files changed, 151 insertions(+), 104 deletions(-)

diff --git a/include/linux/can/can-ml.h b/include/linux/can/can-ml.h
index 1e99fda2b380..cfefa4a385be 100644
--- a/include/linux/can/can-ml.h
+++ b/include/linux/can/can-ml.h
@@ -43,10 +43,11 @@
 #define CAN_ML_H
 
 #include <linux/can.h>
 #include <linux/list.h>
 #include <linux/netdevice.h>
+#include <linux/spinlock.h>
 
 /* exposed CAN device capabilities for network layer */
 #define CAN_CAP_CC BIT(0) /* CAN CC aka Classical CAN */
 #define CAN_CAP_FD BIT(1) /* CAN FD */
 #define CAN_CAP_XL BIT(2) /* CAN XL */
@@ -57,10 +58,11 @@
 #define CAN_EFF_RCV_ARRAY_SZ (1 << CAN_EFF_RCV_HASH_BITS)
 
 enum { RX_ERR, RX_ALL, RX_FIL, RX_INV, RX_MAX };
 
 struct can_dev_rcv_lists {
+	spinlock_t lock; /* protects the receive lists and entries */
 	struct hlist_head rx[RX_MAX];
 	struct hlist_head rx_sff[CAN_SFF_RCV_ARRAY_SZ];
 	struct hlist_head rx_eff[CAN_EFF_RCV_ARRAY_SZ];
 	int entries;
 };
@@ -79,10 +81,11 @@ static inline struct can_ml_priv *can_get_ml_priv(struct net_device *dev)
 }
 
 static inline void can_set_ml_priv(struct net_device *dev,
 				   struct can_ml_priv *ml_priv)
 {
+	spin_lock_init(&ml_priv->dev_rcv_lists.lock);
 	netdev_set_ml_priv(dev, ml_priv, ML_PRIV_CAN);
 }
 
 static inline bool can_cap_enabled(struct net_device *dev, u32 cap)
 {
diff --git a/include/net/netns/can.h b/include/net/netns/can.h
index 48b79f7e6236..ef7aefdf7b23 100644
--- a/include/net/netns/can.h
+++ b/include/net/netns/can.h
@@ -27,11 +27,11 @@ struct netns_can {
 	struct proc_dir_entry *bcmproc_dir;
 #endif
 
 	/* receive filters subscribed for 'all' CAN devices */
 	struct can_dev_rcv_lists *rx_alldev_list;
-	spinlock_t rcvlists_lock;
+	spinlock_t rcvlists_lock; /* protects rcv_lists_stats */
 	struct timer_list stattimer; /* timer for statistics update */
 	struct can_pkg_stats *pkg_stats;
 	struct can_rcv_lists_stats *rcv_lists_stats;
 
 	/* CAN GW per-net gateway jobs */
diff --git a/net/can/af_can.c b/net/can/af_can.c
index d97f85fc3232..e38a5df3d184 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -460,13 +460,14 @@ int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
 
 	rcv = kmem_cache_alloc(rcv_cache, GFP_KERNEL);
 	if (!rcv)
 		return -ENOMEM;
 
-	spin_lock_bh(&net->can.rcvlists_lock);
-
 	dev_rcv_lists = can_dev_rcv_lists_find(net, dev);
+
+	spin_lock_bh(&dev_rcv_lists->lock);
+
 	rcv_list = can_rcv_list_find(&can_id, &mask, dev_rcv_lists);
 
 	rcv->can_id = can_id;
 	rcv->mask = mask;
 	atomic_long_set(&rcv->matches, 0);
@@ -476,10 +477,13 @@ int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
 	rcv->sk = sk;
 
 	hlist_add_head_rcu(&rcv->list, rcv_list);
 	dev_rcv_lists->entries++;
 
+	spin_unlock_bh(&dev_rcv_lists->lock);
+
+	spin_lock_bh(&net->can.rcvlists_lock);
 	rcv_lists_stats->rcv_entries++;
 	rcv_lists_stats->rcv_entries_max = max(rcv_lists_stats->rcv_entries_max,
 					       rcv_lists_stats->rcv_entries);
 	spin_unlock_bh(&net->can.rcvlists_lock);
 
@@ -507,10 +511,16 @@ static void can_rx_delete_receiver(struct rcu_head *rp)
  * @func: callback function on filter match
  * @data: returned parameter for callback function
  *
  * Description:
  *  Removes subscription entry depending on given (subscription) values.
+ *  The receive lists of a CAN device are protected by a per-device lock,
+ *  so the subscription is found even when the device has been moved to
+ *  another network namespace after can_rx_register(). The caller has to
+ *  ensure that @dev stays valid, e.g. by holding a reference to @dev or
+ *  RTNL. @net is the namespace that has been used for can_rx_register()
+ *  and is only used for the receive list statistics.
  */
 void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
 		       canid_t mask, void (*func)(struct sk_buff *, void *),
 		       void *data)
 {
@@ -520,16 +530,14 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
 	struct can_dev_rcv_lists *dev_rcv_lists;
 
 	if (dev && !can_get_ml_priv(dev))
 		return;
 
-	if (dev && !net_eq(net, dev_net(dev)))
-		return;
+	dev_rcv_lists = can_dev_rcv_lists_find(net, dev);
 
-	spin_lock_bh(&net->can.rcvlists_lock);
+	spin_lock_bh(&dev_rcv_lists->lock);
 
-	dev_rcv_lists = can_dev_rcv_lists_find(net, dev);
 	rcv_list = can_rcv_list_find(&can_id, &mask, dev_rcv_lists);
 
 	/* Search the receiver list for the item to delete.  This should
 	 * exist, since no receiver may be unregistered that hasn't
 	 * been registered before.
@@ -545,30 +553,30 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
 	 * As this case may potentially happen when closing a socket while
 	 * the notifier for removing the CAN netdev is running we just print
 	 * a warning here.
 	 */
 	if (!rcv) {
+		spin_unlock_bh(&dev_rcv_lists->lock);
 		pr_warn("can: receive list entry not found for dev %s, id %03X, mask %03X\n",
 			DNAME(dev), can_id, mask);
-		goto out;
+		return;
 	}
 
 	hlist_del_rcu(&rcv->list);
 	dev_rcv_lists->entries--;
 
+	spin_unlock_bh(&dev_rcv_lists->lock);
+
+	spin_lock_bh(&net->can.rcvlists_lock);
 	if (rcv_lists_stats->rcv_entries > 0)
 		rcv_lists_stats->rcv_entries--;
-
- out:
 	spin_unlock_bh(&net->can.rcvlists_lock);
 
 	/* schedule the receiver item for deletion */
-	if (rcv) {
-		if (rcv->sk)
-			sock_hold(rcv->sk);
-		call_rcu(&rcv->rcu, can_rx_delete_receiver);
-	}
+	if (rcv->sk)
+		sock_hold(rcv->sk);
+	call_rcu(&rcv->rcu, can_rx_delete_receiver);
 }
 EXPORT_SYMBOL(can_rx_unregister);
 
 static inline void deliver(struct sk_buff *skb, struct receiver *rcv)
 {
@@ -821,10 +829,11 @@ static int can_pernet_init(struct net *net)
 {
 	spin_lock_init(&net->can.rcvlists_lock);
 	net->can.rx_alldev_list = kzalloc_obj(*net->can.rx_alldev_list);
 	if (!net->can.rx_alldev_list)
 		goto out;
+	spin_lock_init(&net->can.rx_alldev_list->lock);
 	net->can.pkg_stats = kzalloc_obj(*net->can.pkg_stats);
 	if (!net->can.pkg_stats)
 		goto out_free_rx_alldev_list;
 	net->can.rcv_lists_stats = kzalloc_obj(*net->can.rcv_lists_stats);
 	if (!net->can.rcv_lists_stats)
diff --git a/net/can/bcm.c b/net/can/bcm.c
index 6759662e024c..9853c68f0e2c 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -960,11 +960,11 @@ static void bcm_remove_op(struct bcm_op *op)
 }
 
 static void bcm_rx_unreg(struct net_device *dev, struct bcm_op *op)
 {
 	if (op->rx_reg_dev == dev) {
-		can_rx_unregister(dev_net(dev), dev, op->can_id,
+		can_rx_unregister(sock_net(op->sk), dev, op->can_id,
 				  REGMASK(op->can_id), bcm_rx_handler, op);
 
 		/* mark as removed subscription */
 		op->rx_reg_dev = NULL;
 		netdev_put(dev, &op->rx_reg_dev_tracker);
@@ -986,25 +986,18 @@ static int bcm_delete_rx_op(struct list_head *ops, struct bcm_msg_head *mh,
 		    (op->flags & CAN_FD_FRAME) == (mh->flags & CAN_FD_FRAME)) {
 
 			/* disable automatic timer on frame reception */
 			op->flags |= RX_NO_AUTOTIMER;
 
-			/*
-			 * Don't care if we're bound or not (due to netdev
-			 * problems) can_rx_unregister() is always a save
-			 * thing to do here.
-			 */
 			if (op->ifindex) {
-				/*
-				 * Only remove subscriptions that had not
-				 * been removed due to NETDEV_UNREGISTER
-				 * in bcm_notifier()
-				 *
-				 * op->rx_reg_dev is a tracked reference taken
-				 * when the subscription was registered, so it
-				 * stays valid here even if a concurrent
-				 * NETDEV_UNREGISTER already unlisted the dev.
+				/* Only remove subscriptions that had not been
+				 * removed due to NETDEV_UNREGISTER in
+				 * bcm_notify(). The tracked reference
+				 * op->rx_reg_dev keeps the device's receive
+				 * lists accessible, even when the device has
+				 * been unlisted or moved to another network
+				 * namespace meanwhile.
 				 */
 				if (op->rx_reg_dev)
 					bcm_rx_unreg(op->rx_reg_dev, op);
 			} else
 				can_rx_unregister(sock_net(op->sk), NULL,
@@ -1933,10 +1926,11 @@ static int bcm_release(struct socket *sock)
 		spin_lock(&bcm_notifier_lock);
 	}
 	list_del(&bo->notifier);
 	spin_unlock(&bcm_notifier_lock);
 
+	/* bcm_notify() can not access this socket anymore */
 	lock_sock(sk);
 
 #if IS_ENABLED(CONFIG_PROC_FS)
 	/* remove procfs entry */
 	if (net->can.bcmproc_dir && bo->bcm_proc_read)
@@ -1946,33 +1940,26 @@ static int bcm_release(struct socket *sock)
 	list_for_each_entry_safe(op, next, &bo->tx_ops, list) {
 		list_del_rcu(&op->list);
 		bcm_remove_op(op);
 	}
 
-	list_for_each_entry_safe(op, next, &bo->rx_ops, list) {
-		/*
-		 * Don't care if we're bound or not (due to netdev problems)
-		 * can_rx_unregister() is always a save thing to do here.
-		 */
+	list_for_each_entry(op, &bo->rx_ops, list) {
 		if (op->ifindex) {
-			/*
-			 * Only remove subscriptions that had not
-			 * been removed due to NETDEV_UNREGISTER
-			 * in bcm_notifier()
-			 *
-			 * op->rx_reg_dev is a tracked reference taken
-			 * when the subscription was registered, so it
-			 * stays valid here even if a concurrent
-			 * NETDEV_UNREGISTER already unlisted the device.
+			/* Only remove subscriptions that had not been removed
+			 * due to NETDEV_UNREGISTER in bcm_notify(). The tracked
+			 * reference op->rx_reg_dev keeps the device's receive
+			 * lists accessible, even when the device has been
+			 * moved to another network namespace meanwhile.
 			 */
 			if (op->rx_reg_dev)
 				bcm_rx_unreg(op->rx_reg_dev, op);
-		} else
+		} else {
 			can_rx_unregister(net, NULL, op->can_id,
 					  REGMASK(op->can_id),
 					  bcm_rx_handler, op);
-
+		}
+		cond_resched();
 	}
 
 	synchronize_rcu();
 
 	list_for_each_entry_safe(op, next, &bo->rx_ops, list) {
diff --git a/net/can/isotp.c b/net/can/isotp.c
index 1a272ce6a2bd..fbd92d003f4a 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -1487,15 +1487,16 @@ static int isotp_release(struct socket *sock)
 		spin_lock(&isotp_notifier_lock);
 	}
 	list_del(&so->notifier);
 	spin_unlock(&isotp_notifier_lock);
 
-	rtnl_lock();
 	lock_sock(sk);
 
 	/* remove current filters & unregister
-	 * tracked reference so->dev is taken at bind() time with rtnl_lock
+	 * tracked reference so->dev is taken at bind() time
+	 * and keeps the device's receive lists accessible, even when the
+	 * device has been moved to another network namespace meanwhile
 	 */
 	if (so->bound && so->dev) {
 		if (isotp_register_rxid(so))
 			can_rx_unregister(net, so->dev, so->rxid,
 					  SINGLE_MASK(so->rxid),
@@ -1509,12 +1510,10 @@ static int isotp_release(struct socket *sock)
 
 	so->ifindex = 0;
 	so->bound = 0;
 	so->dev = NULL;
 
-	rtnl_unlock();
-
 	/* 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.
@@ -1578,11 +1577,11 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
 	}
 
 	if (!addr->can_ifindex)
 		return -ENODEV;
 
-	rtnl_lock();
+	/* lock_sock() serializes the binding with isotp_notify() */
 	lock_sock(sk);
 
 	if (so->bound) {
 		err = -EINVAL;
 		goto out;
@@ -1617,11 +1616,11 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
 	}
 	if (READ_ONCE(dev->mtu) < so->ll.mtu) {
 		err = -EINVAL;
 		goto out_put_dev;
 	}
-	if (!(dev->flags & IFF_UP))
+	if (!(READ_ONCE(dev->flags) & IFF_UP))
 		notify_enetdown = 1;
 
 	ifindex = dev->ifindex;
 
 	if (isotp_register_rxid(so))
@@ -1643,19 +1642,18 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
 
 	/* bind() ok -> hold a reference for so->dev so that isotp_release()
 	 * can safely reach the device later, even if a concurrent
 	 * NETDEV_UNREGISTER has already unlisted it by ifindex.
 	 */
-	so->dev = dev;
+	WRITE_ONCE(so->dev, dev);
 	netdev_hold(so->dev, &so->dev_tracker, GFP_KERNEL);
 
 out_put_dev:
 	/* remove potential reference from dev_get_by_index() */
 	dev_put(dev);
 out:
 	release_sock(sk);
-	rtnl_unlock();
 
 	if (notify_enetdown) {
 		sk->sk_err = ENETDOWN;
 		if (!sock_flag(sk, SOCK_DEAD))
 			sk_error_report(sk);
@@ -1854,16 +1852,22 @@ static void isotp_notify(struct isotp_sock *so, unsigned long msg,
 	struct sock *sk = &so->sk;
 
 	if (!net_eq(dev_net(dev), sock_net(sk)))
 		return;
 
-	if (so->dev != dev)
+	/* 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))
 				can_rx_unregister(dev_net(dev), dev, so->rxid,
 						  SINGLE_MASK(so->rxid),
@@ -1875,11 +1879,11 @@ static void isotp_notify(struct isotp_sock *so, unsigned long msg,
 			netdev_put(so->dev, &so->dev_tracker);
 		}
 
 		so->ifindex = 0;
 		so->bound  = 0;
-		so->dev = NULL;
+		WRITE_ONCE(so->dev, NULL);
 		release_sock(sk);
 
 		sk->sk_err = ENODEV;
 		if (!sock_flag(sk, SOCK_DEAD))
 			sk_error_report(sk);
diff --git a/net/can/raw.c b/net/can/raw.c
index 09a5c0275ae6..761d23dae044 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -222,10 +222,11 @@ static int raw_enable_filters(struct net *net, struct net_device *dev,
 {
 	int err = 0;
 	int i;
 
 	for (i = 0; i < count; i++) {
+		cond_resched();
 		err = can_rx_register(net, dev, filter[i].can_id,
 				      filter[i].can_mask,
 				      raw_rcv, sk, sock_i_ino(sk), sk);
 		if (err) {
 			/* clean up successfully registered filters */
@@ -256,13 +257,15 @@ static void raw_disable_filters(struct net *net, struct net_device *dev,
 				struct sock *sk, struct can_filter *filter,
 				int count)
 {
 	int i;
 
-	for (i = 0; i < count; i++)
+	for (i = 0; i < count; i++) {
 		can_rx_unregister(net, dev, filter[i].can_id,
 				  filter[i].can_mask, raw_rcv, sk);
+		cond_resched();
+	}
 }
 
 static inline void raw_disable_errfilter(struct net *net,
 					 struct net_device *dev,
 					 struct sock *sk,
@@ -307,16 +310,28 @@ static void raw_notify(struct raw_sock *ro, unsigned long msg,
 	struct sock *sk = &ro->sk;
 
 	if (!net_eq(dev_net(dev), sock_net(sk)))
 		return;
 
-	if (ro->dev != dev)
+	/* Lockless fast path: only a socket bound to this device needs
+	 * handling, so unrelated sockets are not serialized against the
+	 * netdev notifier (which runs under RTNL) via lock_sock(). A bind()
+	 * that sets ro->dev right after this check is caught by the
+	 * NETDEV_UNREGISTER rebroadcast from netdev_wait_allrefs(); ro->dev
+	 * is rechecked under lock_sock() in the NETDEV_UNREGISTER case.
+	 */
+	if (READ_ONCE(ro->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);
 			netdev_put(dev, &ro->dev_tracker);
 		}
@@ -324,11 +339,11 @@ static void raw_notify(struct raw_sock *ro, unsigned long msg,
 		if (ro->count > 1)
 			kfree(ro->filter);
 
 		ro->ifindex = 0;
 		ro->bound = 0;
-		ro->dev = NULL;
+		WRITE_ONCE(ro->dev, NULL);
 		ro->count = 0;
 		release_sock(sk);
 
 		sk->sk_err = ENODEV;
 		if (!sock_flag(sk, SOCK_DEAD))
@@ -429,17 +444,20 @@ static int raw_release(struct socket *sock)
 		spin_lock(&raw_notifier_lock);
 	}
 	list_del(&ro->notifier);
 	spin_unlock(&raw_notifier_lock);
 
-	rtnl_lock();
 	lock_sock(sk);
 
-	/* remove current filters & unregister */
+	/* remove current filters & unregister
+	 * tracked reference ro->dev keeps the device's receive lists
+	 * accessible, even when the device has been moved to another
+	 * network namespace meanwhile
+	 */
 	if (ro->bound) {
 		if (ro->dev) {
-			raw_disable_allfilters(dev_net(ro->dev), ro->dev, sk);
+			raw_disable_allfilters(net, ro->dev, sk);
 			netdev_put(ro->dev, &ro->dev_tracker);
 		} else {
 			raw_disable_allfilters(net, NULL, sk);
 		}
 	}
@@ -454,11 +472,10 @@ static int raw_release(struct socket *sock)
 
 	sock_orphan(sk);
 	sock->sk = NULL;
 
 	release_sock(sk);
-	rtnl_unlock();
 
 	sock_prot_inuse_add(net, sk->sk_prot, -1);
 	sock_put(sk);
 
 	return 0;
@@ -477,11 +494,11 @@ static int raw_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int len
 	if (len < RAW_MIN_NAMELEN)
 		return -EINVAL;
 	if (addr->can_family != AF_CAN)
 		return -EINVAL;
 
-	rtnl_lock();
+	/* lock_sock() serializes the binding with raw_notify() */
 	lock_sock(sk);
 
 	if (ro->bound && addr->can_ifindex == ro->ifindex)
 		goto out;
 
@@ -494,11 +511,11 @@ static int raw_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int len
 		if (!can_get_ml_priv(dev)) {
 			err = -ENODEV;
 			goto out_put_dev;
 		}
 
-		if (!(dev->flags & IFF_UP))
+		if (!(READ_ONCE(dev->flags) & IFF_UP))
 			notify_enetdown = 1;
 
 		ifindex = dev->ifindex;
 
 		/* filters set by default/setsockopt */
@@ -515,32 +532,31 @@ static int raw_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int len
 
 	if (!err) {
 		if (ro->bound) {
 			/* unregister old filters */
 			if (ro->dev) {
-				raw_disable_allfilters(dev_net(ro->dev),
+				raw_disable_allfilters(sock_net(sk),
 						       ro->dev, sk);
 				/* drop reference to old ro->dev */
 				netdev_put(ro->dev, &ro->dev_tracker);
 			} else {
 				raw_disable_allfilters(sock_net(sk), NULL, sk);
 			}
 		}
 		ro->ifindex = ifindex;
 		ro->bound = 1;
 		/* bind() ok -> hold a reference for new ro->dev */
-		ro->dev = dev;
+		WRITE_ONCE(ro->dev, dev);
 		if (ro->dev)
 			netdev_hold(ro->dev, &ro->dev_tracker, GFP_KERNEL);
 	}
 
 out_put_dev:
 	/* remove potential reference from dev_get_by_index() */
 	dev_put(dev);
 out:
 	release_sock(sk);
-	rtnl_unlock();
 
 	if (notify_enetdown) {
 		sk->sk_err = ENETDOWN;
 		if (!sock_flag(sk, SOCK_DEAD))
 			sk_error_report(sk);
@@ -565,62 +581,49 @@ static int raw_getname(struct socket *sock, struct sockaddr *uaddr,
 
 	return RAW_MIN_NAMELEN;
 }
 
 static int raw_setsockopt_locked(struct socket *sock, int optname,
-				 sockptr_t optval, unsigned int optlen)
+				 sockptr_t optval, unsigned int optlen,
+				 struct can_filter *filter,
+				 struct can_filter *sfilter, int count)
 {
 	struct sock *sk = sock->sk;
 	struct raw_sock *ro = raw_sk(sk);
-	struct can_filter *filter = NULL;  /* dyn. alloc'ed filters */
-	struct can_filter sfilter;         /* single filter */
 	struct net_device *dev = NULL;
 	can_err_mask_t err_mask = 0;
-	int count = 0;
 	int flag;
 	int err = 0;
 
 	switch (optname) {
 	case CAN_RAW_FILTER:
-		if (optlen % sizeof(struct can_filter) != 0)
-			return -EINVAL;
-
-		if (optlen > CAN_RAW_FILTER_MAX * sizeof(struct can_filter))
-			return -EINVAL;
-
-		count = optlen / sizeof(struct can_filter);
-
-		if (count > 1) {
-			/* filter does not fit into dfilter => alloc space */
-			filter = memdup_sockptr(optval, optlen);
-			if (IS_ERR(filter))
-				return PTR_ERR(filter);
-		} else if (count == 1) {
-			if (copy_from_sockptr(&sfilter, optval, sizeof(sfilter)))
-				return -EFAULT;
-		}
-
+		/* The filters have been copied from user space in
+		 * raw_setsockopt() before lock_sock() was taken, so a stalled
+		 * copy can not hold lock_sock() and thereby block a concurrent
+		 * NETDEV_UNREGISTER waiting for it under RTNL in raw_notify().
+		 * On success @filter is linked to the socket, on error the
+		 * caller frees it.
+		 *
+		 * Without RTNL the reg_state check is only a shortcut. When the
+		 * device is unregistered concurrently, raw_notify() waits for
+		 * lock_sock() and removes the filters set here.
+		 */
 		dev = ro->dev;
-		if (ro->bound && dev && dev->reg_state != NETREG_REGISTERED) {
-			if (count > 1)
-				kfree(filter);
+		if (ro->bound && dev &&
+		    READ_ONCE(dev->reg_state) != NETREG_REGISTERED)
 			return -ENODEV;
-		}
 
 		if (ro->bound) {
 			/* (try to) register the new filters */
 			if (count == 1)
 				err = raw_enable_filters(sock_net(sk), dev, sk,
-							 &sfilter, 1);
+							 sfilter, 1);
 			else
 				err = raw_enable_filters(sock_net(sk), dev, sk,
 							 filter, count);
-			if (err) {
-				if (count > 1)
-					kfree(filter);
+			if (err)
 				return err;
-			}
 
 			/* remove old filter registrations */
 			raw_disable_filters(sock_net(sk), dev, sk, ro->filter,
 					    ro->count);
 		}
@@ -630,11 +633,11 @@ static int raw_setsockopt_locked(struct socket *sock, int optname,
 			kfree(ro->filter);
 
 		/* link new filters to the socket */
 		if (count == 1) {
 			/* copy filter data for single filter */
-			ro->dfilter = sfilter;
+			ro->dfilter = *sfilter;
 			filter = &ro->dfilter;
 		}
 		ro->filter = filter;
 		ro->count  = count;
 		break;
@@ -646,12 +649,14 @@ static int raw_setsockopt_locked(struct socket *sock, int optname,
 		if (copy_from_sockptr(&err_mask, optval, optlen))
 			return -EFAULT;
 
 		err_mask &= CAN_ERR_MASK;
 
+		/* lockless shortcut, see CAN_RAW_FILTER */
 		dev = ro->dev;
-		if (ro->bound && dev && dev->reg_state != NETREG_REGISTERED)
+		if (ro->bound && dev &&
+		    READ_ONCE(dev->reg_state) != NETREG_REGISTERED)
 			return -ENODEV;
 
 		/* remove current error mask */
 		if (ro->bound) {
 			/* (try to) register the new err_mask */
@@ -749,22 +754,61 @@ static int raw_setsockopt_locked(struct socket *sock, int optname,
 
 static int raw_setsockopt(struct socket *sock, int level, int optname,
 			  sockptr_t optval, unsigned int optlen)
 {
 	struct sock *sk = sock->sk;
+	struct can_filter *filter = NULL;  /* dyn. alloc'ed filters */
+	struct can_filter sfilter;         /* single filter */
+	int count = 0;
 	int err;
 
 	if (level != SOL_CAN_RAW)
 		return -EINVAL;
 
-	rtnl_lock();
+	if (optname == CAN_RAW_FILTER) {
+		/* Copy the filters from user space before taking lock_sock(),
+		 * so a stalled copy can not hold lock_sock() and thereby block
+		 * a concurrent NETDEV_UNREGISTER that waits for lock_sock()
+		 * under RTNL in raw_notify().
+		 */
+		if (optlen % sizeof(struct can_filter) != 0)
+			return -EINVAL;
+
+		if (optlen > CAN_RAW_FILTER_MAX * sizeof(struct can_filter))
+			return -EINVAL;
+
+		count = optlen / sizeof(struct can_filter);
+
+		if (count > 1) {
+			/* filter does not fit into dfilter => alloc space */
+			filter = memdup_sockptr(optval, optlen);
+			if (IS_ERR(filter))
+				return PTR_ERR(filter);
+		} else if (count == 1) {
+			if (copy_from_sockptr(&sfilter, optval, sizeof(sfilter)))
+				return -EFAULT;
+		}
+	}
+
+	/* The filters of a bound socket are changed under lock_sock() only.
+	 * ro->dev is protected by a tracked reference and a concurrent
+	 * NETDEV_UNREGISTER waits in raw_notify() for lock_sock() and then
+	 * removes the current filters.
+	 */
 	lock_sock(sk);
 
-	err = raw_setsockopt_locked(sock, optname, optval, optlen);
+	err = raw_setsockopt_locked(sock, optname, optval, optlen,
+				    filter, &sfilter, count);
 
 	release_sock(sk);
-	rtnl_unlock();
+
+	/* raw_setsockopt_locked() links filter to the socket on success; on
+	 * any error the caller still owns the memdup'ed buffer. count > 1 is
+	 * only reached for CAN_RAW_FILTER, so it unambiguously marks it.
+	 */
+	if (err && count > 1)
+		kfree(filter);
 
 	return err;
 }
 
 static int raw_getsockopt(struct socket *sock, int level, int optname,
-- 
2.53.0

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

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 7838 bytes --]

On 17.10.2026 22:19:35, Oliver Hartkopp wrote:
> The receive lists of a CAN device are located in the device's
> can_ml_priv and travel with the device when it is moved to another
> network namespace. But they are protected by the per-netns
> rcvlists_lock. So can_rx_unregister() needs dev_net(dev) to pick the
> right lock and silently returns when the passed namespace does not
> match dev_net(dev) anymore.
>
> isotp_release() and bcm_release() remove the socket from their notifier
> list before they take any lock that serializes against the netdev
> notifier chain, which runs under RTNL. A socket that leaves the list in
> this window is skipped by isotp_notify() / bcm_notify() and has to
> remove its CAN filters itself.
>
> This fails when the bound device is moved to another network namespace
> in this window: isotp_release() passes sock_net(sk) to
> can_rx_unregister() and bcm_rx_unreg() reads dev_net(dev) without
> holding RTNL, so can_rx_unregister() does not find the filters. As
> can_rx_register() stores rcv->sk without taking a reference, the
> remaining receivers point to the freed socket (isotp) or the freed op
> (bcm) and travel with the device into the new namespace:
>
>   BUG: KASAN: use-after-free in isotp_rcv+0x1570/0x24d0
>   Read of size 1 at addr ffff888118130552 by task isotp_ns_uaf/578
>    can_rcv_filter+0x4af/0x8c0
>    can_receive+0x28d/0x3c0
>    can_rcv+0x2a9/0x310
>    __netif_receive_skb_one_core+0x21a/0x260
>    process_backlog+0x210/0x760
>
> raw_release() is not affected, as it removes the filters with
> dev_net(ro->dev) under RTNL. But raw_release(), raw_setsockopt() and
> raw_bind() hold RTNL while removing and adding up to CAN_RAW_FILTER_MAX
> filters per socket. As this can be triggered repeatedly by unprivileged
> users, e.g. by binding CAN_RAW sockets alternately to different CAN
> devices, RTNL becomes a global bottleneck. Extending this RTNL usage to
> isotp_release() and bcm_release() is therefore no option.
>
> Instead fix the root cause and add a spinlock to struct
> can_dev_rcv_lists, which protects the receive lists of a CAN device or
> the 'any' CAN device lists of a namespace. can_rx_unregister() now finds
> the subscription independently from the current namespace of the
> device. The protocols hold a tracked reference to the device (ro->dev,
> so->dev, op->rx_reg_dev), which keeps the device's receive lists
> accessible. The per-netns rcvlists_lock only protects the receive list
> statistics anymore. The callers pass the namespace that has been used
> for can_rx_register() to keep these statistics balanced.
>
> The lock of the CAN device receive lists is initialized in
> can_set_ml_priv(), which is used by all CAN network devices to set up
> their can_ml_priv.
>
> With this, raw_release(), isotp_release(), bcm_release() and
> raw_setsockopt() do not need RTNL anymore. A socket that leaves the
> notifier list while its device changes the namespace removes its
> filters itself, which now always works. The filters are removed under
> lock_sock() only, with cond_resched() between the filters. The
> existing busy-wait loops on the *_busy_notifier variables remain
> unchanged.
>
> raw_setsockopt() now reads dev->reg_state without RTNL. Use READ_ONCE()
> for this check, which is only a shortcut now: When the device is
> unregistered concurrently, raw_notify() waits for lock_sock() and
> removes the filters that have been set by raw_setsockopt(). The filters
> are copied from user space before lock_sock() is taken, so a stalled
> copy can not hold lock_sock() and thereby block a concurrent
> NETDEV_UNREGISTER that waits for lock_sock() under RTNL in raw_notify().
>
> raw_bind() and isotp_bind() took RTNL to serialize the device lookup
> and the binding with NETDEV_UNREGISTER, as raw_notify() and
> isotp_notify() checked ro->dev and so->dev without lock_sock(). Now
> raw_notify() and isotp_notify() check ro->dev and so->dev under
> lock_sock() in the NETDEV_UNREGISTER case, and the device lookup in
> raw_bind() and isotp_bind() is done under lock_sock() too. Both on
> device unregistration and on a namespace change, unlist_netdevice() and
> synchronize_net() are executed before NETDEV_UNREGISTER is sent. So
> either the notifier gets lock_sock() first and a subsequent bind() can
> not look up the device anymore, or bind() gets lock_sock() first and the
> notifier finds the new binding afterwards and removes it. A bind() that
> looked up the device just before unlist_netdevice() but takes
> lock_sock() only after the notifier has run registers its filters on the
> unlisted device; these are removed when netdev_wait_allrefs() rebroadcasts
> NETDEV_UNREGISTER while waiting for the tracked device reference. With
> this, raw_bind() and isotp_bind() do not need RTNL anymore. raw_bind()
> removes the filters of the previously bound device with sock_net(sk),
> which has been used for can_rx_register().
>
> raw_notify() and isotp_notify() keep a lockless READ_ONCE() fast path
> that skips sockets not bound to the vanishing device, so the notifier
> (running under RTNL) only serializes against lock_sock() of the sockets
> actually bound to this device, not against every CAN_RAW / CAN_ISOTP
> socket in the namespace. ro->dev / so->dev are rechecked under
> lock_sock() in the NETDEV_UNREGISTER case; a bind() that sets the
> pointer right after the lockless check is caught by the
> NETDEV_UNREGISTER rebroadcast described above.
>
> The NETDEV_DOWN case only reports an error to the socket and keeps
> checking ro->dev and so->dev locklessly. ro->dev and so->dev are read
> with READ_ONCE() and written with WRITE_ONCE() where they are changed
> in bind() and in the notifier. dev->flags is read with READ_ONCE() in
> bind() as it is read without RTNL now.
>
> Other users of can_rx_unregister() benefit as well: j1939 removes its
> filter with dev_net(ndev). When the device has been moved to another
> namespace after can_rx_register(), this picked the rcvlists_lock of the
> new namespace to protect the device's receive lists. Now the receive
> lists are always protected by the per-device lock. The receive list
> statistics of the namespaces may still become imprecise in this case,
> as before.
>
> Measured with four CAN_RAW sockets each rebinding CAN_RAW_FILTER_MAX
> (512) filters between two vcan devices for 15s, on a kernel with lockdep
> and CONFIG_LOCK_STAT (which inflates the absolute numbers, so only the
> comparison matters). Without this patch raw_bind() is an rtnl_mutex
> contention point and the rebind rate is ~32/s; with it raw_bind() no
> longer takes rtnl_mutex, the rate rises to ~99/s and the rtnl_mutex
> contention from this workload disappears (rtnl_mutex contentions
> 1621 -> 213, rtnl_mutex.wait_lock 307 -> 0). An unrelated
> ip link add/del running in parallel is no longer stalled behind the
> filter churn.
>
> Fixes: 20bab8b88baa ("can: isotp: fix use-after-free race with concurrent NETDEV_UNREGISTER")
> Fixes: 8e8cda6d737d ("can: initial support for network namespaces")
> Reported-by: Norbert Szetei <norbert@doyensec.com>
> Link: https://lore.kernel.org/linux-can/CEA6A38A-2646-4ADA-95B4-CBAE2F301A8E@doyensec.com/
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>

Can you make use of Documentation/dev-tools/context-analysis.rst for
this series to document what each lock protects?

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]