CAN netlayer fixes for stable

PATCH In review Oliver Hartkopp 2 participants 9 messages Sep 29 – Oct 13

Subsystems (4)

Networking Stable branch CAN network drivers CAN network layer

lore

Share & tools

lore

Oliver Hartkopp

author maintainer

2026-09-29 16:34 UTC #0 lore raw

v5 cover CAN netlayer fixes for stable

This patch set aggregates the open stable fixes for the CAN netlayer.

Patch 3 is new and addresses a RPS regression but it needs the other patches as basis.

Changes in patch 3 only.
v2: We need a non-zero UID for the cfecho checks in isotp.c

    So restore the former while (!(skb->hash)) statement

v3: omit the problematic use of skb->hash reported by sashiko bot
v4: address tc mirred or netem duplicate clones suggested by sashiko bot
v5: remove the skb_clone() test speed-up as it introduces a bug

Oliver Hartkopp (3):
can: remove CAN filters independent from namespace
can: convert unreliable ARPHRD_CAN type checks to robust can_get_ml_priv()
can: fix unique skb identifier regression under RPS

 drivers/net/can/vxcan.c  |   3 ++
 include/linux/can/core.h |   3 +-
 include/linux/can/skb.h  |   4 +-
 include/net/can.h        |   2 +
 net/can/af_can.c         |  56 +++++++++++++-------
 net/can/bcm.c            | 107 +++++++++++++++++++++++++++++----------
 net/can/gw.c             |  10 ++--
 net/can/isotp.c          |  28 +++++-----
 net/can/raw.c            |  17 ++++---
 9 files changed, 160 insertions(+), 70 deletions(-)

Signature · 1 line

2.53.0

Oliver Hartkopp

author maintainer

2026-09-29 16:34 UTC #1 lore raw

PATCH v5 1/3 can: remove CAN filters independent from namespace

When the devices namespace is changed the socket namespace and the device namespace might differ. The net_eq(dev_net(dev), sock_net(sk)) check in the CAN protocols netdev notifiers therefore led to skipping the required removal of the CAN filters from the (namespace changed) CAN devices.

This patch removes the namespace equality check in the netdev notifiers for BCM, ISOTP and RAW sockets. Since the struct net_device pointer is globally unique, the notifier should always process the unregister event and remove the CAN filters if it matches the original socket's bound device pointer.

In bcm.c netdevice comparisons were performed by checking the interface index (bo->ifindex and op->ifindex) which is not namespace-safe either. Introduce tracked netdevice pointers (bo->dev and op->tx_dev) for these referenced devices to enable namespace-save device comparisons. Additional put all bo->dev accesses in bcm_notify() under lock_sock().

In isotp.c the two missing can_rx_unregister() calling sites are converted to use dev_net(dev) instead of sock_net(sk) to get the correct namespace.

Fixes 8e8cda6d737d ("can: initial support for network namespaces")

Reported-by Norbert Szetei <[email protected]>

Link https://lore.kernel.org/linux-can/[email protected]/ open here

Cc [email protected]

Signed-off-by Oliver Hartkopp <[email protected]>

---
 net/can/bcm.c   | 100 ++++++++++++++++++++++++++++++++++++------------
 net/can/isotp.c |   7 +---
 net/can/raw.c   |   3 --
 3 files changed, 78 insertions(+), 32 deletions(-)

net/can/bcm.c +76 ~~-24~~

index 3d637a1e0ac1..cd3522ec32c0 100644
@@ -128,18 +128,22 @@ struct bcm_op {
 	struct canfd_frame sframe;
 	struct canfd_frame last_sframe;
 	struct sock *sk;
 	struct net_device *rx_reg_dev;
 	netdevice_tracker rx_reg_dev_tracker;
+	struct net_device *tx_dev;
+	netdevice_tracker tx_dev_tracker;
 	spinlock_t bcm_tx_lock; /* protect tx data and timer updates */
 	spinlock_t bcm_rx_update_lock; /* protect filter/timer data updates */
 };
 
 struct bcm_sock {
 	struct sock sk;
 	int bound;
 	int ifindex;
+	struct net_device *dev;
+	netdevice_tracker dev_tracker;
 	struct list_head notifier;
 	struct list_head rx_ops;
 	struct list_head tx_ops;
 	unsigned long dropped_usr_msgs;
 	struct proc_dir_entry *bcm_proc_read;

@@ -933,10 +937,13 @@ static void bcm_free_op_work(struct work_struct *work)
 		kfree(op->frames);
 
 	if ((op->last_frames) && (op->last_frames != &op->last_sframe))
 		kfree(op->last_frames);
 
+	if (op->tx_dev)
+		netdev_put(op->tx_dev, &op->tx_dev_tracker);
+
 	/* the last possible access to op->timer/op->thrtimer has now
 	 * happened above via hrtimer_cancel() - op->sk is no longer
 	 * needed by any pending timer callback, so drop our reference
 	 */
 	sock_put(op->sk);

@@ -1072,10 +1079,11 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
 			int ifindex, struct sock *sk)
 {
 	struct bcm_sock *bo = bcm_sk(sk);
 	struct bcm_op *op;
 	struct canfd_frame *cf;
+	struct net_device *tx_dev;
 	bool add_op_to_list = false;
 	unsigned int i;
 	int err;
 
 	/* we need a real device to send frames */

@@ -1103,10 +1111,26 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
 		 * therefore (complexity / locking) it is not supported.
 		 */
 		if (msg_head->nframes > op->nframes)
 			return -E2BIG;
 
+		/* Re-resolve and re-hold the target device if a concurrent
+		 * NETDEV_UNREGISTER already cleared it (see bcm_notify()).
+		 * op->ifindex and sock_net(sk) is unchanged.
+		 */
+		if (!op->tx_dev) {
+			tx_dev = dev_get_by_index(sock_net(sk), ifindex);
+			if (tx_dev) {
+				op->tx_dev = tx_dev;
+				netdev_hold(tx_dev, &op->tx_dev_tracker,
+					    GFP_KERNEL);
+				dev_put(tx_dev);
+			} else {
+				return -ENODEV;
+			}
+		}
+
 		/* get new CAN frames content into a staging buffer before
 		 * locking: validate and normalize the frames there so that
 		 * bcm_can_tx() / bcm_tx_timeout_handler() never observe a
 		 * partially updated or unvalidated frame in op->frames
 		 */

@@ -1169,10 +1193,22 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
 
 		op = kzalloc(OPSIZ, GFP_KERNEL);
 		if (!op)
 			return -ENOMEM;
 
+		tx_dev = dev_get_by_index(sock_net(sk), ifindex);
+		if (tx_dev) {
+			op->tx_dev = tx_dev;
+			netdev_hold(tx_dev, &op->tx_dev_tracker, GFP_KERNEL);
+			dev_put(tx_dev);
+		} else {
+			/* prepare op->frames for goto free_op */
+			op->frames = &op->sframe;
+			err = -ENODEV;
+			goto free_op;
+		}
+
 		spin_lock_init(&op->bcm_tx_lock);
 		op->can_id = msg_head->can_id;
 		op->cfsiz = CFSIZ(msg_head->flags);
 		op->flags = msg_head->flags;
 		op->nframes = msg_head->nframes;

@@ -1184,12 +1220,14 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
 		if (msg_head->nframes > 1) {
 			op->frames = kmalloc_array(msg_head->nframes,
 						   op->cfsiz,
 						   GFP_KERNEL);
 			if (!op->frames) {
-				kfree(op);
-				return -ENOMEM;
+				/* prepare op->frames for goto free_op */
+				op->frames = &op->sframe;
+				err = -ENOMEM;
+				goto free_op;
 			}
 		} else
 			op->frames = &op->sframe;
 
 		for (i = 0; i < msg_head->nframes; i++) {

@@ -1267,10 +1305,13 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
 		bcm_tx_start_timer(op);
 
 	return msg_head->nframes * op->cfsiz + MHSIZ;
 
 free_op:
+	if (op->tx_dev)
+		netdev_put(op->tx_dev, &op->tx_dev_tracker);
+
 	if (op->frames != &op->sframe)
 		kfree(op->frames);
 	kfree(op);
 	return err;
 }

@@ -1790,46 +1831,47 @@ static int bcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 static void bcm_notify(struct bcm_sock *bo, unsigned long msg,
 		       struct net_device *dev)
 {
 	struct sock *sk = &bo->sk;
 	struct bcm_op *op;
-	int notify_enodev = 0;
+	int sk_err = 0;
 
-	if (!net_eq(dev_net(dev), sock_net(sk)))
-		return;
+	lock_sock(sk);
 
 	switch (msg) {
 
 	case NETDEV_UNREGISTER:
-		lock_sock(sk);
 
 		/* rx_ops: remove device specific receive entries */
 		list_for_each_entry(op, &bo->rx_ops, list) {
 			if (op->rx_reg_dev == dev)
 				bcm_rx_unreg(dev, op);
 
 			/* release an ANYDEV op's claim (see bcm_rx_handler())
 			 * on this now confirmed-gone interface.
 			 */
-			if (!op->ifindex) {
+			if (!op->ifindex && net_eq(dev_net(dev), sock_net(sk))) {
 				spin_lock_bh(&op->bcm_rx_update_lock);
 				if (op->if_detected == dev->ifindex)
 					op->if_detected = 0;
 				spin_unlock_bh(&op->bcm_rx_update_lock);
 			}
 		}
 
 		/* tx_ops: stop device specific cyclic transmissions on the
-		 * vanishing ifindex. Cancelling the timer is enough to stop
+		 * vanishing device. Cancelling the timer is enough to stop
 		 * cyclic bcm_can_tx() calls as there is no re-arming.
 		 */
 		list_for_each_entry(op, &bo->tx_ops, list)
-			if (op->ifindex == dev->ifindex)
+			if (op->tx_dev == dev) {
 				hrtimer_cancel(&op->timer);
+				netdev_put(op->tx_dev, &op->tx_dev_tracker);
+				op->tx_dev = NULL;
+			}
 
 		/* remove device reference, if this is our bound device */
-		if (bo->bound && bo->ifindex == dev->ifindex) {
+		if (bo->bound && bo->dev == dev) {
 #if IS_ENABLED(CONFIG_PROC_FS)
 			if (sock_net(sk)->can.bcmproc_dir && bo->bcm_proc_read) {
 				remove_proc_entry(bo->procname, sock_net(sk)->can.bcmproc_dir);
 				bo->bcm_proc_read = NULL;
 			}

@@ -1839,28 +1881,27 @@ static void bcm_notify(struct bcm_sock *bo, unsigned long msg,
 			 * accessed under lock_sock() so it needs no
 			 * annotation.
 			 */
 			WRITE_ONCE(bo->bound, 0);
 			bo->ifindex = 0;
-			notify_enodev = 1;
-		}
-
-		release_sock(sk);
-
-		if (notify_enodev) {
-			sk->sk_err = ENODEV;
-			if (!sock_flag(sk, SOCK_DEAD))
-				sk_error_report(sk);
+			netdev_put(bo->dev, &bo->dev_tracker);
+			bo->dev = NULL;
+			sk_err = ENODEV;
 		}
 		break;
 
 	case NETDEV_DOWN:
-		if (bo->bound && bo->ifindex == dev->ifindex) {
-			sk->sk_err = ENETDOWN;
-			if (!sock_flag(sk, SOCK_DEAD))
-				sk_error_report(sk);
-		}
+		if (bo->bound && bo->dev == dev)
+			sk_err = ENETDOWN;
+	}
+
+	release_sock(sk);
+
+	if (sk_err) {
+		sk->sk_err = sk_err;
+		if (!sock_flag(sk, SOCK_DEAD))
+			sk_error_report(sk);
 	}
 }
 
 static int bcm_notifier(struct notifier_block *nb, unsigned long msg,
 			void *ptr)

@@ -1982,10 +2023,14 @@ static int bcm_release(struct socket *sock)
 
 	/* remove device reference */
 	if (bo->bound) {
 		WRITE_ONCE(bo->bound, 0);
 		bo->ifindex = 0;
+		if (bo->dev) {
+			netdev_put(bo->dev, &bo->dev_tracker);
+			bo->dev = NULL;
+		}
 	}
 
 	sock_orphan(sk);
 	sock->sk = NULL;

@@ -2029,25 +2074,32 @@ static int bcm_connect(struct socket *sock, struct sockaddr_unsized *uaddr, int
 			ret = -ENODEV;
 			goto fail;
 		}
 
 		bo->ifindex = dev->ifindex;
+		bo->dev = dev;
+		netdev_hold(dev, &bo->dev_tracker, GFP_KERNEL);
 		dev_put(dev);
 
 	} else {
 		/* no interface reference for ifindex = 0 ('any' CAN device) */
 		bo->ifindex = 0;
+		bo->dev = NULL;
 	}
 
 #if IS_ENABLED(CONFIG_PROC_FS)
 	if (net->can.bcmproc_dir) {
 		/* unique socket address as filename */
 		sprintf(bo->procname, "%llu", sock_i_ino(sk));
 		bo->bcm_proc_read = proc_create_net_single(bo->procname, 0644,
 						     net->can.bcmproc_dir,
 						     bcm_proc_show, sk);
 		if (!bo->bcm_proc_read) {
+			if (bo->dev) {
+				netdev_put(bo->dev, &bo->dev_tracker);
+				bo->dev = NULL;
+			}
 			ret = -ENOMEM;
 			goto fail;
 		}
 	}
 #endif /* CONFIG_PROC_FS */

net/can/isotp.c +2 -5

index 155530aedce2..0835a4758a72 100644
@@ -1490,15 +1490,15 @@ static int isotp_release(struct socket *sock)
 	/* remove current filters & unregister
 	 * tracked reference so->dev is taken at bind() time with rtnl_lock
 	 */
 	if (so->bound && so->dev) {
 		if (isotp_register_rxid(so))
-			can_rx_unregister(net, so->dev, so->rxid,
+			can_rx_unregister(dev_net(so->dev), so->dev, so->rxid,
 					  SINGLE_MASK(so->rxid),
 					  isotp_rcv, sk);
 
-		can_rx_unregister(net, so->dev, so->txid,
+		can_rx_unregister(dev_net(so->dev), so->dev, so->txid,
 				  SINGLE_MASK(so->txid),
 				  isotp_rcv_echo, sk);
 		netdev_put(so->dev, &so->dev_tracker);
 	}

@@ -1846,13 +1846,10 @@ static int isotp_getsockopt(struct socket *sock, int level, int optname,
 static void isotp_notify(struct isotp_sock *so, unsigned long msg,
 			 struct net_device *dev)
 {
 	struct sock *sk = &so->sk;
 
-	if (!net_eq(dev_net(dev), sock_net(sk)))
-		return;
-
 	if (so->dev != dev)
 		return;
 
 	switch (msg) {
 	case NETDEV_UNREGISTER:

net/can/raw.c +0 -3

index 82d9c0499c95..c5596fc9aac5 100644
@@ -300,13 +300,10 @@ static int raw_enable_allfilters(struct net *net, struct net_device *dev,
 static void raw_notify(struct raw_sock *ro, unsigned long msg,
 		       struct net_device *dev)
 {
 	struct sock *sk = &ro->sk;
 
-	if (!net_eq(dev_net(dev), sock_net(sk)))
-		return;
-
 	if (ro->dev != dev)
 		return;
 
 	switch (msg) {
 	case NETDEV_UNREGISTER:

Signature · 1 line

2.53.0

[email protected]

AI review

2026-10-03 05:48 UTC #2 lore raw

Re: [PATCH v5 1/3] can: remove CAN filters independent from namespace

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

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

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

- [High] bcm_release() (net/can/bcm.c) unregisters device-bound receive filters without holding RTNL, so a concurrent namespace move can still…
- [Medium] The commit message describes a failure that does not occur in the notifier code.
- [Medium] The patch removes the early net_eq() return in bcm_notify() and moves lock_sock(sk) above the switch (net/can/bcm.c:1838).

can: remove CAN filters independent from namespace

This commit removes the net_eq() namespace check from the BCM, ISOTP and RAW netdev notifiers, so CAN filters are removed on NETDEV_UNREGISTER even when the device namespace differs from the socket namespace. In bcm.c it adds tracked net_device pointers (bo->dev, op->tx_dev) for device comparisons, and bcm_notify() now takes lock_sock() for every event. In isotp_release() the can_rx_unregister() calls now use dev_net(so->dev).

Link https://lore.kernel.org/linux-can/[email protected]/ open here

Quoted 4 lines

When the devices namespace is changed the socket namespace and the device namespace might differ. The net_eq(dev_net(dev), sock_net(sk)) check in the CAN protocols netdev notifiers therefore led to skipping the required removal of the CAN filters from the (namespace changed) CAN devices.

[Severity: Medium]

Is this root cause accurate? In __dev_change_net_namespace(), the UNREGISTER notifiers run while dev_net(dev) still points to the old namespace:

net/core/dev.c:__dev_change_net_namespace() {
    ...
	unlist_netdevice(dev);
    ...
	call_netdevice_notifiers(NETDEV_UNREGISTER, dev);
    ...
	netdev_lock(dev);
	dev_net_set(dev, net);
	netdev_unlock(dev);
	dev->ifindex = new_ifindex;
    ...
}

The bind and registration paths are raw_bind(), isotp_bind(), bcm_connect(), bcm_rx_setup() and bcm_sendmsg(). All of them look up the device with dev_get_by_index(sock_net(sk), ...). can_rx_register() also refuses a device from another namespace:

	if (dev && !net_eq(net, dev_net(dev)))
		return -ENODEV;

For a socket with filters on the moved device, wouldn't the removed net_eq() check in raw_notify(), isotp_notify() and bcm_notify() have passed during the move?

No registration seems able to get in between either:

- raw and isotp register under rtnl.
- bcm_rx_setup() runs under lock_sock(), which bcm_notify() also takes for UNREGISTER.
- Lookups fail after unlist_netdevice().

I couldn't find a path where the notifier sees a bound device in a foreign namespace.

The real bug this patch fixes looks like it is in isotp_release():

1. The socket is unlinked from the notifier list.
2. A namespace move completes.
3. can_rx_unregister(sock_net(sk), so->dev, ...) returns early on its own net_eq() check.

That is a race between release and the move, not the notifier check.

Could the commit message describe the path that was actually hit, or the reproducer from the report? The BCM rework, which is tagged for stable, is justified by this description.

Quoted 4 lines

net/can/bcm.c +0 -0

index 3d637a1e0ac1a..cd3522ec32c0a 100644

[ ... ]

net/can/bcm.c static void bcm_notify(struct bcm_sock *bo, unsigned long msg, · line 1838

Quoted 10 lines

net/can/bcm.c +2 -3

@@ -1792,15 +1833,13 @@ static void bcm_notify(struct bcm_sock *bo, unsigned long msg,
 {
 	struct sock *sk = &bo->sk;
 	struct bcm_op *op;
-	int notify_enodev = 0;
+	int sk_err = 0;
 
-	if (!net_eq(dev_net(dev), sock_net(sk)))
-		return;
+	lock_sock(sk);

[Severity: Medium]

With the early return gone and lock_sock() moved above the switch, bcm_notifier() now takes the lock of every BCM socket in every namespace. It does this for every NETDEV_DOWN and NETDEV_UNREGISTER of any CAN device, while holding RTNL:

net/can/bcm.c:bcm_notifier() {
    ...
	list_for_each_entry(bcm_busy_notifier, &bcm_notifier_list, notifier) {
		spin_unlock(&bcm_notifier_lock);
		bcm_notify(bcm_busy_notifier, msg, dev);
		spin_lock(&bcm_notifier_lock);
	}
    ...
}

Before this change, sockets in other namespaces returned immediately, and NETDEV_DOWN never took lock_sock().

bcm_sendmsg() holds lock_sock() across bcm_tx_setup(), bcm_rx_setup() and bcm_tx_send(), and these call memcpy_from_msg() on the user buffer. Suppose that buffer is on a FUSE-backed mmap that never answers the read. An unprivileged user in a user namespace can set this up. The copy then blocks with the socket lock held.

Can a CAN device event in an unrelated namespace then block in lock_sock() inside bcm_notify() with RTNL held? An example is "ip link set can0 down" in init_net. That would stall network configuration system-wide.

A similar stall was already possible for same-namespace UNREGISTER events. This change extends it to cross-namespace events and to NETDEV_DOWN. Later patches in this series don't appear to change this.

NETDEV_DOWN only needs the bo->dev == dev comparison, which raw_notify() and isotp_notify() already do without the socket lock. Would a lockless check, before lock_sock(), of whether this socket references dev at all avoid this?

Quoted 2 lines

net/can/bcm.c +0 -0
 	switch (msg) {

[ ... ]

net/can/bcm.c static int bcm_release(struct socket *sock) · line 2034

Quoted 11 lines

net/can/bcm.c +4 -0

@@ -1984,6 +2025,10 @@ static int bcm_release(struct socket *sock)
 	if (bo->bound) {
 		WRITE_ONCE(bo->bound, 0);
 		bo->ifindex = 0;
+		if (bo->dev) {
+			netdev_put(bo->dev, &bo->dev_tracker);
+			bo->dev = NULL;
+		}
 	}
 
 	sock_orphan(sk);

[Severity: High]

Does bcm_release() still have the same window between release and a namespace move that this patch closes for isotp?

bcm_release() removes the socket from bcm_notifier_list and then takes only lock_sock(). Unlike raw_release() and isotp_release(), it does not take rtnl_lock():

net/can/bcm.c:bcm_release() {
    ...
	list_del(&bo->notifier);
	spin_unlock(&bcm_notifier_lock);

	lock_sock(sk);
    ...
			if (op->rx_reg_dev)
				bcm_rx_unreg(op->rx_reg_dev, op);
    ...
}

The list_del() can happen before a concurrent move's UNREGISTER walk reaches this socket. In that case bcm_notify() never runs for it, and op->rx_reg_dev stays set.

bcm_rx_unreg() then reads dev_net(dev) without RTNL, and can_rx_unregister() reads it a second time:

net/can/bcm.c:bcm_rx_unreg() {
	if (op->rx_reg_dev == dev) {
		can_rx_unregister(dev_net(dev), 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);
    ...
}

net/can/af_can.c:can_rx_unregister() {
    ...
	if (dev && !net_eq(net, dev_net(dev)))
		return;
    ...
}

The concurrent writer is dev_net_set() in __dev_change_net_namespace(), which runs under RTNL and netdev_lock().

Say the argument is sampled as the old namespace and dev_net_set() runs before the net_eq() check. can_rx_unregister() then returns without unlinking anything. bcm_rx_unreg() still clears op->rx_reg_dev, and bcm_remove_op() later frees op.

Can the next frame received on the device in the new namespace then call bcm_rx_handler() with the freed op?

Now say the check passes instead. hlist_del_rcu() runs under the old namespace's rcvlists_lock, while code in the new namespace may modify the same per-device list under its own lock.

Could this corrupt the receive list?

The isotp_release() change to dev_net(so->dev) in this patch relies on rtnl_lock() being held there. This window existed before this patch, and later patches in this series don't appear to change bcm_release().

For the BCM part of this fix to be complete, does bcm_release() need rtnl_lock() around the unregister step, as raw_release() and isotp_release() take?

Signature · 1 line

Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260929163424.16382-1-socketcan%40hartkopp.net

Oliver Hartkopp

author maintainer

2026-10-13 11:08 UTC #3 lore raw

Re: [PATCH v5 1/3] can: remove CAN filters independent from namespace

I reworked and stripped this patch to only address the race which can indeed be fixed by moving rtnl_lock() as originally suggested by Norbert and Opus-5 ;-)

The new patch is reduced for stable here:

https://lore.kernel.org/linux-can/[email protected]/

Thanks Norbert and thanks for the review!

Best regards,
Oliver

Quoted 237 lines · On 03.10.26 07:48, [email protected] wrote:

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider.
Critical: 0 · High: 1 · Medium: 2 · Low: 0
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).
- [High] bcm_release() (net/can/bcm.c) unregisters device-bound receive filters without holding RTNL, so a concurrent namespace move can still…
- [Medium] The commit message describes a failure that does not occur in the notifier code.
- [Medium] The patch removes the early net_eq() return in bcm_notify() and moves lock_sock(sk) above the switch (net/can/bcm.c:1838).
--
can: remove CAN filters independent from namespace
This commit removes the net_eq() namespace check from the BCM, ISOTP and RAW netdev notifiers, so CAN filters are removed on NETDEV_UNREGISTER even when the device namespace differs from the socket namespace. In bcm.c it adds tracked net_device pointers (bo->dev, op->tx_dev) for device comparisons, and bcm_notify() now takes lock_sock() for every event. In isotp_release() the can_rx_unregister() calls now use dev_net(so->dev).
Link https://lore.kernel.org/linux-can/[email protected]/ open here
Quoted 4 lines
When the devices namespace is changed the socket namespace and the device namespace might differ. The net_eq(dev_net(dev), sock_net(sk)) check in the CAN protocols netdev notifiers therefore led to skipping the required removal of the CAN filters from the (namespace changed) CAN devices.
[Severity: Medium]
Is this root cause accurate? In __dev_change_net_namespace(), the UNREGISTER notifiers run while dev_net(dev) still points to the old namespace:
net/core/dev.c:__dev_change_net_namespace() {
     ...
	unlist_netdevice(dev);
     ...
	call_netdevice_notifiers(NETDEV_UNREGISTER, dev);
     ...
	netdev_lock(dev);
	dev_net_set(dev, net);
	netdev_unlock(dev);
	dev->ifindex = new_ifindex;
     ...
}
The bind and registration paths are raw_bind(), isotp_bind(), bcm_connect(), bcm_rx_setup() and bcm_sendmsg(). All of them look up the device with dev_get_by_index(sock_net(sk), ...). can_rx_register() also refuses a device from another namespace:
	if (dev && !net_eq(net, dev_net(dev)))
		return -ENODEV;
For a socket with filters on the moved device, wouldn't the removed net_eq() check in raw_notify(), isotp_notify() and bcm_notify() have passed during the move?
No registration seems able to get in between either:
- raw and isotp register under rtnl.
- bcm_rx_setup() runs under lock_sock(), which bcm_notify() also takes for UNREGISTER.
- Lookups fail after unlist_netdevice().
I couldn't find a path where the notifier sees a bound device in a foreign namespace.
The real bug this patch fixes looks like it is in isotp_release():
1. The socket is unlinked from the notifier list.
2. A namespace move completes.
3. can_rx_unregister(sock_net(sk), so->dev, ...) returns early on its own net_eq() check.
That is a race between release and the move, not the notifier check.
Could the commit message describe the path that was actually hit, or the reproducer from the report? The BCM rework, which is tagged for stable, is justified by this description.
Quoted 4 lines
net/can/bcm.c +0 -0
index 3d637a1e0ac1a..cd3522ec32c0a 100644
[ ... ]
net/can/bcm.c static void bcm_notify(struct bcm_sock *bo, unsigned long msg, · line 1838
Quoted 10 lines
net/can/bcm.c +2 -3
@@ -1792,15 +1833,13 @@ static void bcm_notify(struct bcm_sock *bo, unsigned long msg,
  {
  	struct sock *sk = &bo->sk;
  	struct bcm_op *op;
-	int notify_enodev = 0;
+	int sk_err = 0;
 
-	if (!net_eq(dev_net(dev), sock_net(sk)))
-		return;
+	lock_sock(sk);
[Severity: Medium]
With the early return gone and lock_sock() moved above the switch, bcm_notifier() now takes the lock of every BCM socket in every namespace. It does this for every NETDEV_DOWN and NETDEV_UNREGISTER of any CAN device, while holding RTNL:
net/can/bcm.c:bcm_notifier() {
     ...
	list_for_each_entry(bcm_busy_notifier, &bcm_notifier_list, notifier) {
		spin_unlock(&bcm_notifier_lock);
		bcm_notify(bcm_busy_notifier, msg, dev);
		spin_lock(&bcm_notifier_lock);
	}
     ...
}
Before this change, sockets in other namespaces returned immediately, and NETDEV_DOWN never took lock_sock().
bcm_sendmsg() holds lock_sock() across bcm_tx_setup(), bcm_rx_setup() and bcm_tx_send(), and these call memcpy_from_msg() on the user buffer. Suppose that buffer is on a FUSE-backed mmap that never answers the read. An unprivileged user in a user namespace can set this up. The copy then blocks with the socket lock held.
Can a CAN device event in an unrelated namespace then block in lock_sock() inside bcm_notify() with RTNL held? An example is "ip link set can0 down" in init_net. That would stall network configuration system-wide.
A similar stall was already possible for same-namespace UNREGISTER events. This change extends it to cross-namespace events and to NETDEV_DOWN. Later patches in this series don't appear to change this.
NETDEV_DOWN only needs the bo->dev == dev comparison, which raw_notify() and isotp_notify() already do without the socket lock. Would a lockless check, before lock_sock(), of whether this socket references dev at all avoid this?
Quoted 2 lines
net/can/bcm.c +0 -0
  	switch (msg) {
[ ... ]
net/can/bcm.c static int bcm_release(struct socket *sock) · line 2034
Quoted 11 lines
net/can/bcm.c +4 -0
@@ -1984,6 +2025,10 @@ static int bcm_release(struct socket *sock)
  	if (bo->bound) {
  		WRITE_ONCE(bo->bound, 0);
  		bo->ifindex = 0;
+		if (bo->dev) {
+			netdev_put(bo->dev, &bo->dev_tracker);
+			bo->dev = NULL;
+		}
  	}
 
  	sock_orphan(sk);
[Severity: High]
Does bcm_release() still have the same window between release and a namespace move that this patch closes for isotp?
bcm_release() removes the socket from bcm_notifier_list and then takes only lock_sock(). Unlike raw_release() and isotp_release(), it does not take rtnl_lock():
net/can/bcm.c:bcm_release() {
     ...
	list_del(&bo->notifier);
	spin_unlock(&bcm_notifier_lock);

	lock_sock(sk);
     ...
			if (op->rx_reg_dev)
				bcm_rx_unreg(op->rx_reg_dev, op);
     ...
}
The list_del() can happen before a concurrent move's UNREGISTER walk reaches this socket. In that case bcm_notify() never runs for it, and op->rx_reg_dev stays set.
bcm_rx_unreg() then reads dev_net(dev) without RTNL, and can_rx_unregister() reads it a second time:
net/can/bcm.c:bcm_rx_unreg() {
	if (op->rx_reg_dev == dev) {
		can_rx_unregister(dev_net(dev), 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);
     ...
}
net/can/af_can.c:can_rx_unregister() {
     ...
	if (dev && !net_eq(net, dev_net(dev)))
		return;
     ...
}
The concurrent writer is dev_net_set() in __dev_change_net_namespace(), which runs under RTNL and netdev_lock().
Say the argument is sampled as the old namespace and dev_net_set() runs before the net_eq() check. can_rx_unregister() then returns without unlinking anything. bcm_rx_unreg() still clears op->rx_reg_dev, and bcm_remove_op() later frees op.
Can the next frame received on the device in the new namespace then call bcm_rx_handler() with the freed op?
Now say the check passes instead. hlist_del_rcu() runs under the old namespace's rcvlists_lock, while code in the new namespace may modify the same per-device list under its own lock.
Could this corrupt the receive list?
The isotp_release() change to dev_net(so->dev) in this patch relies on rtnl_lock() being held there. This window existed before this patch, and later patches in this series don't appear to change bcm_release().
For the BCM part of this fix to be complete, does bcm_release() need rtnl_lock() around the unregister step, as raw_release() and isotp_release() take?

Oliver Hartkopp

author maintainer

2026-10-13 16:45 UTC #4 lore raw

Re: [PATCH v5 1/3] can: remove CAN filters independent from namespace

Hi Marc,

the namespace movement at release time problem also applies to bcm.c and raw.c (less critical). Therefore I sent three patches after all.

I think they won't make it into 7.3 in this cycle - but maybe you can consider them for your can-next PR.

At least they are all checked by Claude Opus-5 so I don't expect any harsh feedback from sashiko-bot :-D

Best regards,
Oliver

Quoted 249 lines · Oliver Hartkopp

I reworked and stripped this patch to only address the race which can indeed be fixed by moving rtnl_lock() as originally suggested by Norbert and Opus-5 ;-)
The new patch is reduced for stable here:
https://lore.kernel.org/linux-can/20261013105909.91715-1- [email protected]/
Thanks Norbert and thanks for the review!
Best regards,
Oliver
Quoted 235 lines · On 03.10.26 07:48, [email protected] wrote:
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider.
Critical: 0 · High: 1 · Medium: 2 · Low: 0
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).
- [High] bcm_release() (net/can/bcm.c) unregisters device-bound receive filters without holding RTNL, so a concurrent namespace move can still…
- [Medium] The commit message describes a failure that does not occur in the notifier code.
- [Medium] The patch removes the early net_eq() return in bcm_notify() and moves lock_sock(sk) above the switch (net/can/bcm.c:1838).
Signature · 11 lines
can: remove CAN filters independent from namespace

This commit removes the net_eq() namespace check from the BCM, ISOTP and
RAW netdev notifiers, so CAN filters are removed on NETDEV_UNREGISTER
even when the device namespace differs from the socket namespace. In
bcm.c it adds tracked net_device pointers (bo->dev, op->tx_dev) for
device comparisons, and bcm_notify() now takes lock_sock() for every
event. In isotp_release() the can_rx_unregister() calls now use
dev_net(so->dev).

Link: https://lore.kernel.org/linux-can/CEA6A38A-2646-4ADA-95B4- [email protected]/
Quoted 4 lines
When the devices namespace is changed the socket namespace and the device namespace might differ. The net_eq(dev_net(dev), sock_net(sk)) check in the CAN protocols netdev notifiers therefore led to skipping the required removal of the CAN filters from the (namespace changed) CAN devices.
[Severity: Medium]
Is this root cause accurate? In __dev_change_net_namespace(), the UNREGISTER notifiers run while dev_net(dev) still points to the old namespace:
net/core/dev.c:__dev_change_net_namespace() {
     ...
    unlist_netdevice(dev);
     ...
    call_netdevice_notifiers(NETDEV_UNREGISTER, dev);
     ...
    netdev_lock(dev);
    dev_net_set(dev, net); netdev_unlock(dev);
    dev->ifindex = new_ifindex;
     ...
}
The bind and registration paths are raw_bind(), isotp_bind(), bcm_connect(), bcm_rx_setup() and bcm_sendmsg(). All of them look up the device with dev_get_by_index(sock_net(sk), ...). can_rx_register() also refuses a device from another namespace:
    if (dev && !net_eq(net, dev_net(dev)))
        return -ENODEV;
For a socket with filters on the moved device, wouldn't the removed net_eq() check in raw_notify(), isotp_notify() and bcm_notify() have passed during the move?
No registration seems able to get in between either:
- raw and isotp register under rtnl.
- bcm_rx_setup() runs under lock_sock(), which bcm_notify() also takes for UNREGISTER.
- Lookups fail after unlist_netdevice().
I couldn't find a path where the notifier sees a bound device in a foreign namespace.
The real bug this patch fixes looks like it is in isotp_release():
1. The socket is unlinked from the notifier list.
2. A namespace move completes.
3. can_rx_unregister(sock_net(sk), so->dev, ...) returns early on its own net_eq() check.
That is a race between release and the move, not the notifier check.
Could the commit message describe the path that was actually hit, or the reproducer from the report? The BCM rework, which is tagged for stable, is justified by this description.
Quoted 4 lines
net/can/bcm.c +0 -0
index 3d637a1e0ac1a..cd3522ec32c0a 100644
[ ... ]
net/can/bcm.c static void bcm_notify(struct bcm_sock *bo, · line 1838
Quoted 10 lines
net/can/bcm.c +2 -3
@@ -1792,15 +1833,13 @@ static void bcm_notify(struct bcm_sock *bo, 
 unsigned long msg,
   {
       struct sock *sk = &bo->sk;
       struct bcm_op *op;
-    int notify_enodev = 0;
+    int sk_err = 0;
-    if (!net_eq(dev_net(dev), sock_net(sk)))
-        return;
+    lock_sock(sk);
[Severity: Medium]
With the early return gone and lock_sock() moved above the switch, bcm_notifier() now takes the lock of every BCM socket in every namespace. It does this for every NETDEV_DOWN and NETDEV_UNREGISTER of any CAN device, while holding RTNL:
net/can/bcm.c:bcm_notifier() {
     ...
    list_for_each_entry(bcm_busy_notifier, &bcm_notifier_list, notifier) { spin_unlock(&bcm_notifier_lock); bcm_notify(bcm_busy_notifier, msg, dev); spin_lock(&bcm_notifier_lock);
    }
     ...
}
Before this change, sockets in other namespaces returned immediately, and NETDEV_DOWN never took lock_sock().
bcm_sendmsg() holds lock_sock() across bcm_tx_setup(), bcm_rx_setup() and bcm_tx_send(), and these call memcpy_from_msg() on the user buffer. Suppose that buffer is on a FUSE-backed mmap that never answers the read. An unprivileged user in a user namespace can set this up. The copy then blocks with the socket lock held.
Can a CAN device event in an unrelated namespace then block in lock_sock() inside bcm_notify() with RTNL held? An example is "ip link set can0 down" in init_net. That would stall network configuration system-wide.
A similar stall was already possible for same-namespace UNREGISTER events. This change extends it to cross-namespace events and to NETDEV_DOWN. Later patches in this series don't appear to change this.
NETDEV_DOWN only needs the bo->dev == dev comparison, which raw_notify() and isotp_notify() already do without the socket lock. Would a lockless check, before lock_sock(), of whether this socket references dev at all avoid this?
Quoted 1 line
net/can/bcm.c +0 -0
       switch (msg) {
[ ... ]
net/can/bcm.c static int bcm_release(struct socket *sock) · line 2033
Quoted 10 lines
net/can/bcm.c +4 -0
@@ -1984,6 +2025,10 @@ static int bcm_release(struct socket *sock)
       if (bo->bound) {
           WRITE_ONCE(bo->bound, 0);
           bo->ifindex = 0;
+        if (bo->dev) {
+            netdev_put(bo->dev, &bo->dev_tracker);
+            bo->dev = NULL;
+        }
       }
       sock_orphan(sk);
[Severity: High]
Does bcm_release() still have the same window between release and a namespace move that this patch closes for isotp?
bcm_release() removes the socket from bcm_notifier_list and then takes only lock_sock(). Unlike raw_release() and isotp_release(), it does not take rtnl_lock():
net/can/bcm.c:bcm_release() {
     ...
    list_del(&bo->notifier);
    spin_unlock(&bcm_notifier_lock);
    lock_sock(sk);
     ...
            if (op->rx_reg_dev)
                bcm_rx_unreg(op->rx_reg_dev, op);
     ...
}
The list_del() can happen before a concurrent move's UNREGISTER walk reaches this socket. In that case bcm_notify() never runs for it, and op->rx_reg_dev stays set.
bcm_rx_unreg() then reads dev_net(dev) without RTNL, and can_rx_unregister() reads it a second time:
net/can/bcm.c:bcm_rx_unreg() {
    if (op->rx_reg_dev == dev) { can_rx_unregister(dev_net(dev), 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);
     ...
}
net/can/af_can.c:can_rx_unregister() {
     ...
    if (dev && !net_eq(net, dev_net(dev)))
        return;
     ...
}
The concurrent writer is dev_net_set() in __dev_change_net_namespace(), which runs under RTNL and netdev_lock().
Say the argument is sampled as the old namespace and dev_net_set() runs before the net_eq() check. can_rx_unregister() then returns without unlinking anything. bcm_rx_unreg() still clears op->rx_reg_dev, and bcm_remove_op() later frees op.
Can the next frame received on the device in the new namespace then call bcm_rx_handler() with the freed op?
Now say the check passes instead. hlist_del_rcu() runs under the old namespace's rcvlists_lock, while code in the new namespace may modify the same per-device list under its own lock.
Could this corrupt the receive list?
The isotp_release() change to dev_net(so->dev) in this patch relies on rtnl_lock() being held there. This window existed before this patch, and later patches in this series don't appear to change bcm_release().
For the BCM part of this fix to be complete, does bcm_release() need rtnl_lock() around the unregister step, as raw_release() and isotp_release() take?

Oliver Hartkopp

author maintainer

2026-09-29 16:34 UTC #5 lore raw

PATCH v5 2/3 can: convert unreliable ARPHRD_CAN type checks to robust can_get_ml_priv()

In reply to Oliver Hartkopp

Commit 4e096a18867a ("net: introduce CAN specific pointer in the struct net_device") introduced an explicit way to assign the midlayer private pointer (dev->ml_priv) to named users like ML_PRIV_CAN.

With this extension the CAN device specific ml_priv assignment became a robust indicator to identify a valid CAN device, when can_get_ml_priv() returns a valid pointer.

This has been used directly by the referenced commit in the CAN specific j1939 and proc code but not in the other parts of the CAN subsystem.

With the TUN/TAP driver a device's ARPHRD type can be controlled by userspace independently of its midlayer private data (ml_priv). The TUNSETLINK ioctl allows a down TUN/TAP device to overwrite its hardware type to become ARPHRD_CAN while dev->ml_priv remains NULL (uninitialized).

Instead of checking dev->type being the unreliable ARPHRD_CAN value convert the missing "valid CAN devices" checks to can_get_ml_priv().

Fixes 4e096a18867a ("net: introduce CAN specific pointer in the struct net_device")

Cc [email protected]

Cc Oleksij Rempel <[email protected]>

Signed-off-by Oliver Hartkopp <[email protected]>

---
 net/can/af_can.c | 12 ++++++------
 net/can/bcm.c    |  7 ++++---
 net/can/gw.c     |  7 ++++---
 net/can/isotp.c  |  5 +++--
 net/can/raw.c    |  4 ++--
 5 files changed, 19 insertions(+), 16 deletions(-)

net/can/af_can.c +6 -6

index 7bc86b176b4d..ef435f22ac93 100644
@@ -224,11 +224,11 @@ int can_send(struct sk_buff *skb, int loop)
 	if (unlikely(skb->len > READ_ONCE(skb->dev->mtu))) {
 		err = -EMSGSIZE;
 		goto inval_skb;
 	}
 
-	if (unlikely(skb->dev->type != ARPHRD_CAN)) {
+	if (unlikely(!can_get_ml_priv(skb->dev))) {
 		err = -EPERM;
 		goto inval_skb;
 	}
 
 	if (unlikely(!(skb->dev->flags & IFF_UP))) {

@@ -450,11 +450,11 @@ int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
 	struct can_dev_rcv_lists *dev_rcv_lists;
 	struct can_rcv_lists_stats *rcv_lists_stats = net->can.rcv_lists_stats;
 
 	/* insert new receiver  (dev,canid,mask) -> (func,data) */
 
-	if (dev && (dev->type != ARPHRD_CAN || !can_get_ml_priv(dev)))
+	if (dev && !can_get_ml_priv(dev))
 		return -ENODEV;
 
 	if (dev && !net_eq(net, dev_net(dev)))
 		return -ENODEV;

@@ -517,11 +517,11 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
 	struct receiver *rcv = NULL;
 	struct hlist_head *rcv_list;
 	struct can_rcv_lists_stats *rcv_lists_stats = net->can.rcv_lists_stats;
 	struct can_dev_rcv_lists *dev_rcv_lists;
 
-	if (dev && dev->type != ARPHRD_CAN)
+	if (dev && !can_get_ml_priv(dev))
 		return;
 
 	if (dev && !net_eq(net, dev_net(dev)))
 		return;

@@ -685,11 +685,11 @@ static void can_receive(struct sk_buff *skb, struct net_device *dev)
 }
 
 static int can_rcv(struct sk_buff *skb, struct net_device *dev,
 		   struct packet_type *pt, struct net_device *orig_dev)
 {
-	if (unlikely(dev->type != ARPHRD_CAN || !can_get_ml_priv(dev) ||
+	if (unlikely(!can_get_ml_priv(dev) ||
 		     !can_skb_ext_find(skb) || !can_is_can_skb(skb))) {
 		pr_warn_once("PF_CAN: dropped non conform CAN skbuff: dev type %d, len %d\n",
 			     dev->type, skb->len);
 
 		kfree_skb_reason(skb, SKB_DROP_REASON_CAN_RX_INVALID_FRAME);

@@ -701,11 +701,11 @@ static int can_rcv(struct sk_buff *skb, struct net_device *dev,
 }
 
 static int canfd_rcv(struct sk_buff *skb, struct net_device *dev,
 		     struct packet_type *pt, struct net_device *orig_dev)
 {
-	if (unlikely(dev->type != ARPHRD_CAN || !can_get_ml_priv(dev) ||
+	if (unlikely(!can_get_ml_priv(dev) ||
 		     !can_skb_ext_find(skb) || !can_is_canfd_skb(skb))) {
 		pr_warn_once("PF_CAN: dropped non conform CAN FD skbuff: dev type %d, len %d\n",
 			     dev->type, skb->len);
 
 		kfree_skb_reason(skb, SKB_DROP_REASON_CANFD_RX_INVALID_FRAME);

@@ -717,11 +717,11 @@ static int canfd_rcv(struct sk_buff *skb, struct net_device *dev,
 }
 
 static int canxl_rcv(struct sk_buff *skb, struct net_device *dev,
 		     struct packet_type *pt, struct net_device *orig_dev)
 {
-	if (unlikely(dev->type != ARPHRD_CAN || !can_get_ml_priv(dev) ||
+	if (unlikely(!can_get_ml_priv(dev) ||
 		     !can_skb_ext_find(skb) || !can_is_canxl_skb(skb))) {
 		pr_warn_once("PF_CAN: dropped non conform CAN XL skbuff: dev type %d, len %d\n",
 			     dev->type, skb->len);
 
 		kfree_skb_reason(skb, SKB_DROP_REASON_CANXL_RX_INVALID_FRAME);

net/can/bcm.c +4 -3

index cd3522ec32c0..940b917e3830 100644
@@ -52,10 +52,11 @@
 #include <linux/netdevice.h>
 #include <linux/socket.h>
 #include <linux/if_arp.h>
 #include <linux/skbuff.h>
 #include <linux/can.h>
+#include <linux/can/can-ml.h>
 #include <linux/can/core.h>
 #include <linux/can/skb.h>
 #include <linux/can/bcm.h>
 #include <linux/slab.h>
 #include <linux/workqueue.h>

@@ -1758,11 +1759,11 @@ static int bcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 			if (!dev) {
 				ret = -ENODEV;
 				goto out_release;
 			}
 
-			if (dev->type != ARPHRD_CAN) {
+			if (!can_get_ml_priv(dev)) {
 				dev_put(dev);
 				ret = -ENODEV;
 				goto out_release;
 			}

@@ -1906,11 +1907,11 @@ static void bcm_notify(struct bcm_sock *bo, unsigned long msg,
 static int bcm_notifier(struct notifier_block *nb, unsigned long msg,
 			void *ptr)
 {
 	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
 
-	if (dev->type != ARPHRD_CAN)
+	if (!can_get_ml_priv(dev))
 		return NOTIFY_DONE;
 	if (msg != NETDEV_UNREGISTER && msg != NETDEV_DOWN)
 		return NOTIFY_DONE;
 	if (unlikely(bcm_busy_notifier)) /* Check for reentrant bug. */
 		return NOTIFY_DONE;

@@ -2067,11 +2068,11 @@ static int bcm_connect(struct socket *sock, struct sockaddr_unsized *uaddr, int
 		dev = dev_get_by_index(net, addr->can_ifindex);
 		if (!dev) {
 			ret = -ENODEV;
 			goto fail;
 		}
-		if (dev->type != ARPHRD_CAN) {
+		if (!can_get_ml_priv(dev)) {
 			dev_put(dev);
 			ret = -ENODEV;
 			goto fail;
 		}

net/can/gw.c +4 -3

index 0ec99f68aa45..d9912dea738b 100644
@@ -50,10 +50,11 @@
 #include <linux/net.h>
 #include <linux/netdevice.h>
 #include <linux/if_arp.h>
 #include <linux/skbuff.h>
 #include <linux/can.h>
+#include <linux/can/can-ml.h>
 #include <linux/can/core.h>
 #include <linux/can/skb.h>
 #include <linux/can/gw.h>
 #include <net/can.h>
 #include <net/rtnetlink.h>

@@ -607,11 +608,11 @@ static int cgw_notifier(struct notifier_block *nb,
 			unsigned long msg, void *ptr)
 {
 	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
 	struct net *net = dev_net(dev);
 
-	if (dev->type != ARPHRD_CAN)
+	if (!can_get_ml_priv(dev))
 		return NOTIFY_DONE;
 
 	if (msg == NETDEV_UNREGISTER) {
 		struct cgw_job *gwj = NULL;
 		struct hlist_node *nx;

@@ -1158,19 +1159,19 @@ static int cgw_create_job(struct sk_buff *skb,  struct nlmsghdr *nlh,
 	gwj->src.dev = __dev_get_by_index(net, gwj->ccgw.src_idx);
 
 	if (!gwj->src.dev)
 		goto out;
 
-	if (gwj->src.dev->type != ARPHRD_CAN)
+	if (!can_get_ml_priv(gwj->src.dev))
 		goto out;
 
 	gwj->dst.dev = __dev_get_by_index(net, gwj->ccgw.dst_idx);
 
 	if (!gwj->dst.dev)
 		goto out;
 
-	if (gwj->dst.dev->type != ARPHRD_CAN)
+	if (!can_get_ml_priv(gwj->dst.dev))
 		goto out;
 
 	/* is sending the skb back to the incoming interface intended? */
 	if (gwj->src.dev == gwj->dst.dev &&
 	    !(gwj->flags & CGW_FLAGS_CAN_IIF_TX_OK)) {

net/can/isotp.c +3 -2

index 0835a4758a72..d3f79efc9c1e 100644
@@ -63,10 +63,11 @@
 #include <linux/netdevice.h>
 #include <linux/socket.h>
 #include <linux/if_arp.h>
 #include <linux/skbuff.h>
 #include <linux/can.h>
+#include <linux/can/can-ml.h>
 #include <linux/can/core.h>
 #include <linux/can/skb.h>
 #include <linux/can/isotp.h>
 #include <linux/slab.h>
 #include <net/can.h>

@@ -1604,11 +1605,11 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
 	dev = dev_get_by_index(net, addr->can_ifindex);
 	if (!dev) {
 		err = -ENODEV;
 		goto out;
 	}
-	if (dev->type != ARPHRD_CAN) {
+	if (!can_get_ml_priv(dev)) {
 		err = -ENODEV;
 		goto out_put_dev;
 	}
 	if (READ_ONCE(dev->mtu) < so->ll.mtu) {
 		err = -EINVAL;

@@ -1888,11 +1889,11 @@ static void isotp_notify(struct isotp_sock *so, unsigned long msg,
 static int isotp_notifier(struct notifier_block *nb, unsigned long msg,
 			  void *ptr)
 {
 	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
 
-	if (dev->type != ARPHRD_CAN)
+	if (!can_get_ml_priv(dev))
 		return NOTIFY_DONE;
 	if (msg != NETDEV_UNREGISTER && msg != NETDEV_DOWN)
 		return NOTIFY_DONE;
 	if (unlikely(isotp_busy_notifier)) /* Check for reentrant bug. */
 		return NOTIFY_DONE;

net/can/raw.c +2 -2

index c5596fc9aac5..7c48ff36e6cd 100644
@@ -339,11 +339,11 @@ static void raw_notify(struct raw_sock *ro, unsigned long msg,
 static int raw_notifier(struct notifier_block *nb, unsigned long msg,
 			void *ptr)
 {
 	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
 
-	if (dev->type != ARPHRD_CAN)
+	if (!can_get_ml_priv(dev))
 		return NOTIFY_DONE;
 	if (msg != NETDEV_UNREGISTER && msg != NETDEV_DOWN)
 		return NOTIFY_DONE;
 	if (unlikely(raw_busy_notifier)) /* Check for reentrant bug. */
 		return NOTIFY_DONE;

@@ -482,11 +482,11 @@ static int raw_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int len
 		dev = dev_get_by_index(sock_net(sk), addr->can_ifindex);
 		if (!dev) {
 			err = -ENODEV;
 			goto out;
 		}
-		if (dev->type != ARPHRD_CAN) {
+		if (!can_get_ml_priv(dev)) {
 			err = -ENODEV;
 			goto out_put_dev;
 		}
 
 		if (!(dev->flags & IFF_UP))

Signature · 1 line

2.53.0

[email protected]

AI review

2026-10-03 05:48 UTC #6 lore raw

Re: [PATCH v5 2/3] can: convert unreliable ARPHRD_CAN type checks to robust can_get_ml_priv()

Thank you for your contribution! Sashiko AI review found more severe issue(s) in other patches of this series. This mail is only an FYI, in case you want to address the nit picks below when you…

Thank you for your contribution! Sashiko AI review found more severe issue(s) in other patches of this series. This mail is only an FYI, in case you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message does not say what user-visible bug this stable-tagged patch fixes.

can: convert unreliable ARPHRD_CAN type checks to robust can_get_ml_priv()

This patch replaces the dev->type != ARPHRD_CAN checks in af_can, bcm, gw, isotp and raw with can_get_ml_priv() checks. The stated reason is that TUNSETLINK lets a down TUN/TAP device change its type to ARPHRD_CAN while dev->ml_priv stays NULL.

Quoted 2 lines

This has been used directly by the referenced commit in the CAN specific j1939 and proc code but not in the other parts of the CAN subsystem.

[Severity: Low]
Is the "not in the other parts of the CAN subsystem" part accurate?

af_can.c already checks can_get_ml_priv() in can_rx_register(), which was added by commit 8aa59e355949 ("can: af_can: fix NULL pointer dereference in can_rx_register()"). It is also checked in can_rcv(), canfd_rcv() and canxl_rcv(). The removed lines in this diff show it:

    if (dev && (dev->type != ARPHRD_CAN || !can_get_ml_priv(dev)))
        return -ENODEV;

Quoted 2 lines

Instead of checking dev->type being the unreliable ARPHRD_CAN value convert the missing "valid CAN devices" checks to can_get_ml_priv().

The patch is tagged for stable, but the message reads like a cleanup. Could it also describe the user-visible bug being fixed?

Before this patch, a NULL pointer dereference looks reachable through CAN_ISOTP. isotp_bind() only checked dev->type, so it accepted a TUN device whose type had been set to ARPHRD_CAN. It also ignores the -ENODEV that can_rx_register() returns:

    if (isotp_register_rxid(so))
        can_rx_register(net, dev, rx_id, SINGLE_MASK(rx_id),
                        isotp_rcv, sk, "isotp", sk);
    ...
    so->bound = 1;
    ...
    so->dev = dev;

On close(), isotp_release() then calls can_rx_unregister() on so->dev. Before this patch, can_rx_unregister() only checked the type:

    if (dev && dev->type != ARPHRD_CAN)
        return;

So it carried on into can_dev_rcv_lists_find(), which has no NULL check:

    struct can_ml_priv *can_ml = can_get_ml_priv(dev);
    return &can_ml->dev_rcv_lists;

dev_rcv_lists is the first member of struct can_ml_priv, so this returns NULL. The call chain is:

close()
isotp_release()
can_rx_unregister()
can_dev_rcv_lists_find() <- returns NULL
can_rcv_list_find()
hlist_for_each_entry_rcu() <- NULL dereference

isotp_notify() for NETDEV_UNREGISTER reaches the same path. Creating the TUN device needs CAP_NET_ADMIN, and an unprivileged user can get that inside a user namespace.

Could the message mention the crash, the affected protocol, and the trigger (TUNSETLINK to ARPHRD_CAN, then binding a CAN_ISOTP socket)? That would make it clear to stable backporters that this is a crash fix.

Signature · 1 line

Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260929163424.16382-1-socketcan%40hartkopp.net

Oliver Hartkopp

author maintainer

2026-09-29 16:34 UTC #7 lore raw

PATCH v5 3/3 can: fix unique skb identifier regression under RPS

In reply to Oliver Hartkopp

Commit d4fb6514ff8e ("can: use skb hash instead of private variable in headroom") moved the per-skb unique identifier used for raw_rcv() duplicate detection into skb->hash.

With RPS enabled, get_rps_cpu() calls skb_get_hash() before the frame reaches the CAN subsystem. Since CAN skbs have no L3/L4 headers, the flow dissector assigns every CAN frame the same non-zero software hash. can_set_skb_uid() only generated a new identifier when skb->hash was 0, so it kept this constant hash. When the SLAB allocator later reused the same skb address, raw_rcv() mistook the next legitimate frame for a duplicate and dropped it.

Fix this by storing the CAN UID in the CAN skb extension (struct can_skb_ext::can_skb_uid) instead of skb->hash, decoupling it from any hash the network stack may compute. can_set_skb_uid() keeps its "assign only if unset" behaviour, which preserves UIDs set before transmission (e.g. by isotp to identify echo frames) across the local loopback path.

Frames whose extension is still shared with another clone (e.g. via tc mirred or netem duplicate) are given a private extension copy before the UID is assigned, so that independently received clones never end up with the same UID.

can-gw and vxcan additionally clear the UID of forwarded/duplicated frames so each newly routed frame gets its own unique identifier.

Fixes d4fb6514ff8e ("can: use skb hash instead of private variable in headroom")

Reported-by Joerg Willmann <[email protected]>

Closes https://lore.kernel.org/linux-can/[email protected]/ open here

Cc [email protected]

Tested-by Oliver Hartkopp <[email protected]>

Signed-off-by Oliver Hartkopp <[email protected]>

---
v2: We need a non-zero UID for the cfecho checks in isotp.c

    So restore the former while (!(skb->hash)) statement
v3: omit the problematic use of skb->hash reported by sashiko bot
v4: address tc mirred or netem duplicate clones suggested by sashiko bot
v5: remove the skb_clone() test speed-up as it introduces a bug
---
 drivers/net/can/vxcan.c  |  3 +++
 include/linux/can/core.h |  3 ++-
 include/linux/can/skb.h  |  4 +++-
 include/net/can.h        |  2 ++
 net/can/af_can.c         | 50 +++++++++++++++++++++++++++++-----------
 net/can/gw.c             |  3 +++
 net/can/isotp.c          | 16 ++++++++-----
 net/can/raw.c            | 10 +++++---
 8 files changed, 66 insertions(+), 25 deletions(-)

drivers/net/can/vxcan.c +3 -0

index 9e2e25d02471..51148a81d1d9 100644
@@ -77,10 +77,13 @@ static netdev_tx_t vxcan_xmit(struct sk_buff *oskb, struct net_device *dev)
 		goto out_unlock;
 	}
 
 	/* reset CAN GW hop counter */
 	csx->can_gw_hops = 0;
+	/* start with new CAN skb UID in the other namespace */
+	csx->can_skb_uid = 0;
+
 	skb->pkt_type   = PACKET_BROADCAST;
 	skb->dev        = peer;
 	skb->ip_summed  = CHECKSUM_UNNECESSARY;
 
 	len = can_skb_get_data_len(skb);

include/linux/can/core.h +2 -1

index 3287232e3cad..2de74c2b78b6 100644
@@ -15,10 +15,11 @@
 #define _CAN_CORE_H
 
 #include <linux/can.h>
 #include <linux/skbuff.h>
 #include <linux/netdevice.h>
+#include <net/can.h>
 
 #define DNAME(dev) ((dev) ? (dev)->name : "any")
 
 /**
  * struct can_proto - CAN protocol structure

@@ -56,9 +57,9 @@ extern 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);
 
 extern int can_send(struct sk_buff *skb, int loop);
-void can_set_skb_uid(struct sk_buff *skb);
+void can_set_skb_uid(struct can_skb_ext *csx);
 void can_sock_destruct(struct sock *sk);
 
 #endif /* !_CAN_CORE_H */

include/linux/can/skb.h +3 -1

index a70a02967071..5d27843862fc 100644
@@ -41,12 +41,14 @@ bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb);
 static inline struct can_skb_ext *can_skb_ext_add(struct sk_buff *skb)
 {
 	struct can_skb_ext *csx = skb_ext_add(skb, SKB_EXT_CAN);
 
 	/* skb_ext_add() returns uninitialized space */
-	if (csx)
+	if (csx) {
 		csx->can_gw_hops = 0;
+		csx->can_skb_uid = 0;
+	}
 
 	return csx;
 }
 
 static inline struct can_skb_ext *can_skb_ext_find(struct sk_buff *skb)

include/net/can.h +2 -0

index 6db9e826f0e0..2b8af2598732 100644
@@ -15,14 +15,16 @@
  * struct can_skb_ext - skb extensions for CAN specific content
  * @can_iif: ifindex of the first interface the CAN frame appeared on
  * @can_framelen: cached echo CAN frame length for bql
  * @can_gw_hops: can-gw CAN frame time-to-live counter
  * @can_ext_flags: CAN skb extensions flags
+ * @can_skb_uid: CAN skb UID for raw_rcv and isotp echo handling
  */
 struct can_skb_ext {
 	int	can_iif;
 	u16	can_framelen;
 	u8	can_gw_hops;
 	u8	can_ext_flags;
+	u32	can_skb_uid;
 };
 
 #endif /* _NET_CAN_H */

net/can/af_can.c +36 ~~-14~~

116 lines not on this page · raw message

net/can/gw.c +3 -0

index d9912dea738b..54bb5bd3242a 100644
@@ -526,10 +526,13 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data)
 	}
 
 	/* put the incremented hop counter in the cloned skb */
 	ncsx->can_gw_hops = csx->can_gw_hops + 1;
 
+	/* force a new CAN UID generation for the routed frame */
+	ncsx->can_skb_uid = 0;
+
 	/* first processing of this CAN frame -> adjust to private hop limit */
 	if (gwj->limit_hops && ncsx->can_gw_hops == 1)
 		ncsx->can_gw_hops = max_hops - gwj->limit_hops + 1;
 
 	nskb->dev = gwj->dst.dev;

net/can/isotp.c +10 -6

index d3f79efc9c1e..860c5221e299 100644
@@ -890,11 +890,11 @@ static void isotp_send_cframe(struct isotp_sock *so)
 	}
 
 	csx->can_iif = dev->ifindex;
 
 	/* set uid in tx skb to identify CF echo frames */
-	can_set_skb_uid(skb);
+	can_set_skb_uid(csx);
 
 	cf = (struct canfd_frame *)skb->data;
 	skb_put_zero(skb, so->ll.mtu);
 
 	/* create consecutive frame */

@@ -916,11 +916,11 @@ static void isotp_send_cframe(struct isotp_sock *so)
 	old_cfecho = READ_ONCE(so->cfecho);
 	if (old_cfecho)
 		pr_notice_once("can-isotp: cfecho is %08X != 0\n", old_cfecho);
 
 	/* set consecutive frame echo tag */
-	WRITE_ONCE(so->cfecho, skb->hash);
+	WRITE_ONCE(so->cfecho, csx->can_skb_uid);
 
 	/* send frame with local echo enabled */
 	can_send_ret = can_send(skb, 1);
 	if (can_send_ret) {
 		pr_notice_once("can-isotp: %s: can_send_ret %pe\n",

@@ -968,22 +968,26 @@ static void isotp_create_fframe(struct canfd_frame *cf, struct isotp_sock *so,
 
 static void isotp_rcv_echo(struct sk_buff *skb, void *data)
 {
 	struct sock *sk = (struct sock *)data;
 	struct isotp_sock *so = isotp_sk(sk);
+	struct can_skb_ext *csx = can_skb_ext_find(skb);
 
 	/* only handle my own local echo CF/SF skb's (no FF!) */
 	if (skb->sk != sk)
 		return;
 
+	if (WARN_ON_ONCE(!csx))
+		return;
+
 	/* unlike isotp_rcv_fc()/isotp_rcv_cf(), not already under so->rx_lock
 	 * (no isotp_rcv() caller here), so take it ourselves
 	 */
 	spin_lock(&so->rx_lock);
 
 	/* so->cfecho may since belong to a new transfer; recheck under lock */
-	if (READ_ONCE(so->cfecho) != skb->hash)
+	if (READ_ONCE(so->cfecho) != csx->can_skb_uid)
 		goto out_unlock;
 
 	/* cancel local echo timeout */
 	hrtimer_cancel(&so->echotimer);

@@ -1221,11 +1225,11 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 	}
 
 	csx->can_iif = dev->ifindex;
 
 	/* set uid in tx skb to identify CF echo frames */
-	can_set_skb_uid(skb);
+	can_set_skb_uid(csx);
 
 	so->tx.len = size;
 	so->tx.idx = 0;
 
 	cf = (struct canfd_frame *)skb->data;

@@ -1260,11 +1264,11 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 			cf->data[SF_PCI_SZ4 + ae] = size;
 		else
 			cf->data[ae] |= size;
 
 		/* set CF echo tag for isotp_rcv_echo() (SF-mode) */
-		WRITE_ONCE(so->cfecho, skb->hash);
+		WRITE_ONCE(so->cfecho, csx->can_skb_uid);
 	} else {
 		/* send first frame */
 
 		isotp_create_fframe(cf, so, ae);

@@ -1277,11 +1281,11 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 
 			/* disable wait for FCs due to activated block size */
 			so->txfc.bs = 0;
 
 			/* set CF echo tag for isotp_rcv_echo() (CF-mode) */
-			WRITE_ONCE(so->cfecho, skb->hash);
+			WRITE_ONCE(so->cfecho, csx->can_skb_uid);
 		} else {
 			/* standard flow control check */
 			new_state = ISOTP_WAIT_FIRST_FC;
 
 			/* start timeout for FC */

net/can/raw.c +7 -3

index 7c48ff36e6cd..7873bfc584ef 100644
@@ -75,11 +75,11 @@ MODULE_ALIAS("can-proto-1");
  * storing the single filter in dfilter, to avoid using dynamic memory.
  */
 
 struct uniqframe {
 	const struct sk_buff *skb;
-	u32 hash;
+	u32 can_skb_uid;
 	unsigned int join_rx_count;
 };
 
 struct raw_sock {
 	struct sock sk;

@@ -131,16 +131,20 @@ static void raw_rcv(struct sk_buff *oskb, void *data)
 	struct sock *sk = (struct sock *)data;
 	struct raw_sock *ro = raw_sk(sk);
 	enum skb_drop_reason reason;
 	struct sockaddr_can *addr;
 	struct sk_buff *skb;
+	struct can_skb_ext *csx = can_skb_ext_find(oskb);
 	unsigned int *pflags;
 
 	/* check the received tx sock reference */
 	if (!ro->recv_own_msgs && oskb->sk == sk)
 		return;
 
+	if (WARN_ON_ONCE(!csx))
+		return;
+
 	/* make sure to not pass oversized frames to the socket */
 	if (!ro->fd_frames && can_is_canfd_skb(oskb))
 		return;
 
 	if (can_is_canxl_skb(oskb)) {

@@ -163,21 +167,21 @@ static void raw_rcv(struct sk_buff *oskb, void *data)
 		}
 	}
 
 	/* eliminate multiple filter matches for the same skb */
 	if (this_cpu_ptr(ro->uniq)->skb == oskb &&
-	    this_cpu_ptr(ro->uniq)->hash == oskb->hash) {
+	    this_cpu_ptr(ro->uniq)->can_skb_uid == csx->can_skb_uid) {
 		if (!ro->join_filters)
 			return;
 
 		this_cpu_inc(ro->uniq->join_rx_count);
 		/* drop frame until all enabled filters matched */
 		if (this_cpu_ptr(ro->uniq)->join_rx_count < ro->count)
 			return;
 	} else {
 		this_cpu_ptr(ro->uniq)->skb = oskb;
-		this_cpu_ptr(ro->uniq)->hash = oskb->hash;
+		this_cpu_ptr(ro->uniq)->can_skb_uid = csx->can_skb_uid;
 		this_cpu_ptr(ro->uniq)->join_rx_count = 1;
 		/* drop first frame to check all enabled filters? */
 		if (ro->join_filters && ro->count > 1)
 			return;
 	}

Signature · 1 line

2.53.0

[email protected]

AI review

2026-10-03 05:48 UTC #8 lore raw

Re: [PATCH v5 3/3] can: fix unique skb identifier regression under RPS

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

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] The commit message says that frames sharing an extension with another clone (tc mirred, netem duplicate) get a private copy before…

can: fix unique skb identifier regression under RPS

This commit moves the CAN per-skb unique identifier that raw_rcv() uses for duplicate detection out of skb->hash and into a new can_skb_uid field in struct can_skb_ext. The receive handlers now give a shared extension a private copy before they assign the UID, and can-gw and vxcan clear the UID on forwarded frames.

Closes https://lore.kernel.org/linux-can/[email protected]/ open here

Quoted 20 lines

net/can/af_can.c +3 -6

index ef435f22ac933..dc27ace43719d 100644
@@ -641,13 +641,10 @@ static int can_rcv_filter(struct can_dev_rcv_lists *dev_rcv_lists, struct sk_buf
 	return matches;
 }
 
-void can_set_skb_uid(struct sk_buff *skb)
+void can_set_skb_uid(struct can_skb_ext *csx)
 {
-	/* create non-zero unique skb identifier together with *skb */
-	while (!(skb->hash))
-		skb->hash = atomic_inc_return(&skbcounter);
-
-	skb->sw_hash = 1;
+	while (!(csx->can_skb_uid))
+		csx->can_skb_uid = atomic_inc_return(&skbcounter);
 }
 EXPORT_SYMBOL(can_set_skb_uid);

[ ... ]

net/can/af_can.c static int can_rcv(struct sk_buff *skb, struct net_device *dev, · line 702

Quoted 12 lines

net/can/af_can.c +8 -0

@@ -696,6 +692,14 @@ static int can_rcv(struct sk_buff *skb, struct net_device *dev,
 		return NET_RX_DROP;
 	}
 
+	/* create unshared CAN skb_extension for netem/mirred skb clones */
+	csx = skb_ext_add(skb, SKB_EXT_CAN);
+	if (unlikely(!csx)) {
+		kfree_skb_reason(skb, SKB_DROP_REASON_NOMEM);
+		return NET_RX_DROP;
+	}
+
+	can_set_skb_uid(csx);

[Severity: Medium]
Do independently received clones get different UIDs if the UID was already set before the clone was made? The same question applies to canfd_rcv() and canxl_rcv().

The commit message says:

skb_ext_add() copies the shared extension through skb_ext_maybe_cow(), which is a memcpy(). Any non-zero can_skb_uid that is already set ends up in the private copy. can_set_skb_uid() then keeps that value, because it only assigns a UID when the value is 0.

isotp sets a non-zero UID before transmission, in both isotp_sendmsg() and isotp_send_cframe():

	/* set uid in tx skb to identify CF echo frames */
	can_set_skb_uid(csx);
	...
	can_send(skb, 1);

Suppose that TX skb is duplicated on egress. netem_enqueue() calls skb2 = skb_clone(skb, GFP_ATOMIC) for duplicates, and mirred egress mirroring also clones. Every copy then has the same UID X. Could the following happen on an IFF_ECHO device such as vcan with echo=1?

vcan_tx(skb)
can_create_echo_skb()
nskb = skb_clone(skb) /* echo e1, UID X */
consume_skb(skb)
netif_rx(e1)
can_rcv(e1)
raw_rcv() /* per-cpu uniq = {e1, X} */
can_receive()
consume_skb(e1) /* e1 slab slot freed */

netem watchdog dequeues skb2
vcan_tx(skb2)
can_create_echo_skb()
nskb = skb_clone(skb2) /* e2 reuses e1's slot, UID X */
netif_rx(e2)
can_rcv(e2)

    raw_rcv()

The check in raw_rcv() would then match:

	if (this_cpu_ptr(ro->uniq)->skb == oskb &&
	    this_cpu_ptr(ro->uniq)->can_skb_uid == csx->can_skb_uid) {
		if (!ro->join_filters)
			return;

The duplicated frame would be silently dropped for every raw socket that already received e1. With join_filters set, join_rx_count would be counted too high instead.

This is the same false duplicate drop that the patch fixes for skb->hash. Here it is limited to frames with a UID set before transmission. The old skb->hash scheme behaved the same way in this case, because skb_clone() copies the hash. can-gw and vxcan clear the UID explicitly, but generic clone paths such as netem and mirred do not.

Clearing the UID whenever the extension is shared would not fix this either. For devices without IFF_ECHO, can_send() does the loopback with newskb = skb_clone(skb) while the original skb is still alive. isotp echo matching depends on that clone keeping the UID.

Should the commit message be toned down for this case, or is there another way to tell these clones apart?

Quoted 3 lines

net/can/af_can.c +0 -0

 	can_receive(skb, dev);
 	return NET_RX_SUCCESS;
 }

Signature · 1 line

Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260929163424.16382-1-socketcan%40hartkopp.net