[PATCH net-next 0/11] pull-request: can-next 2026-10-21

[PATCH net-next 0/11] pull-request: can-next 2026-10-21

[Marc Kleine-Budde]

Hello netdev-team,

this is a pull request of 11 patches for net-next/main and obsoletes
pull-request: can-next 2026-10-19.

The first patch is by Vincent Mailhol and drops CAN-XL frames on non
CAN-XL devices.

A patch by Sang-Heon Jeon removes a redundant NULL check before
netdev_hold() in the CAN RAW protocol.

A patch by Kaixuan Li for the CAN ISOTP protocol improves the check
for the correct CAN CC/FD frames.

The next 3 patches are by Cunhao Lu and make CAN skb freeing safe in
any context in several CAN helper functions.

A series by Oliver Hartkopp et al fixes several problems in the CAN
net-layer:

3 patches target the CAN procfs: A patch by Runyu Xiao resets the CAN
pkg_stats using atomics, a patch by Oliver Hartkopp fixes the stats
calculation after a statistics reset. Followed by a patch to remove
kernel pointers from the output.

The last 2 patches are by Oliver Hartkopp as well, they convert the
unreliable ARPHRD_CAN type check to the robust can_get_ml_priv() and
protect the CAN receive lists with a per-device lock.

regards,
Marc

---

The following changes since commit 7dca8514c51ea5219daf7f28bc1a0536b907e611:

  Merge branch 'vsock-so_inq-for-stream-sockets' (2026-10-21 11:58:24 +0200)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git tags/linux-can-next-for-7.4-20261021

for you to fetch changes up to 26e74534310157d284fe9f2518c1503e055b6210:

  Merge patch series "can: protect the receive lists with a per-device lock" (2026-10-21 12:19:14 +0200)

----------------------------------------------------------------
linux-can-next-for-7.4-20261021

----------------------------------------------------------------
Cunhao Lu (3):
      can: skb: make echo skb freeing safe in any IRQ context
      can: skb: make CAN skb allocation failure paths IRQ-safe
      can: dev: can_put_echo_skb(): free skb on invalid echo index

Kaixuan Li (1):
      can: isotp: check the frame type, not just the length

Marc Kleine-Budde (2):
      Merge patch series "can: skb: make echo skb freeing safe in any IRQ context"
      Merge patch series "can: protect the receive lists with a per-device lock"

Oliver Hartkopp (4):
      can: proc: skip stats calculation after reset in can_stat_update()
      can: proc: remove pointers from CAN specific proc output
      can: replace ARPHRD_CAN type checks with can_get_ml_priv()
      can: protect the receive lists with a per-device lock

Runyu Xiao (1):
      can: proc: reset pkg_stats atomics individually

Sang-Heon Jeon (1):
      can: raw: remove redundant NULL check before netdev_hold()

Vincent Mailhol (1):
      can: dev: can_dropped_invalid_skb: drop CAN XL frames on non-CAN XL devices

 Documentation/networking/can.rst |  16 ++--
 drivers/net/can/dev/skb.c        |  16 ++--
 include/linux/can/can-ml.h       |   3 +
 include/linux/can/core.h         |   2 +-
 include/linux/can/dev.h          |   2 +-
 include/linux/can/skb.h          |   4 +-
 include/net/netns/can.h          |   2 +-
 net/can/af_can.c                 |  62 ++++++++-------
 net/can/af_can.h                 |   2 +-
 net/can/bcm.c                    |  64 ++++++---------
 net/can/gw.c                     |   9 ++-
 net/can/isotp.c                  |  65 +++++++++++----
 net/can/j1939/main.c             |   2 +-
 net/can/proc.c                   |  95 +++++++++++++---------
 net/can/raw.c                    | 166 +++++++++++++++++++++++++--------------
 15 files changed, 306 insertions(+), 204 deletions(-)

[PATCH net-next 01/11] can: dev: can_dropped_invalid_skb: drop CAN XL frames on non-CAN XL devices

[Marc Kleine-Budde]

From: Vincent Mailhol <mailhol@kernel.org>

Sending a PF_PACKET bypasses the CAN framework logic and can directly
reach a CAN driver's xmit() function. The PF_PACKET framework only
checks that skb->len does not exceed the net_device MTU.

For a CAN device that is not CAN XL capable, anything above CANFD_MTU
(72 bytes) is therefore dropped before it reaches the driver. However,
CAN XL frames are variable length. can_is_canxl_skb() accepts lengths in
the range CANXL_HDR_SIZE + CANXL_MIN_DLEN up to CANXL_MTU, i.e. 13 to
2060 bytes.

As a result, an ETH_P_CANXL skb with a length between 13 and 72 bytes
can pass both the MTU and the can_dropped_invalid_skb() checks.

A driver that does not support CAN XL will interpret canxl_frame->flags
as a length because of the overlap with can_frame->len. And because
CANXL_XLF is set, the resulting length is between 128 and 255. For
drivers that do not check can_frame->len before copying can_frame->data,
as most drivers do, this results in a buffer overflow of up to 247
bytes.

Drop ETH_P_CANXL skbs if the device does not have the CAN_CAP_XL
capability. Keep can_is_canxl_skb() for the validation of CAN XL skbs.

Closes: https://sashiko.dev/#/patchset/20260731-master-v5-0-5b27029dee20@qq.com?part=1
Fixes: fb08cba12b52 ("can: canxl: update CAN infrastructure for CAN XL frames")
Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
Link: https://patch.msgid.link/20260731-drop_canxl_frames-v1-1-7387b70353b3@kernel.org
Cc: stable@kernel.org
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/dev/skb.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
index 14edec5afb57..bc0787535b52 100644
--- a/drivers/net/can/dev/skb.c
+++ b/drivers/net/can/dev/skb.c
@@ -4,6 +4,7 @@
  * Copyright (C) 2008-2009 Wolfgang Grandegger <wg@grandegger.com>
  */
 
+#include <linux/can/can-ml.h>
 #include <linux/can/dev.h>
 #include <linux/module.h>
 #include <net/can.h>
@@ -388,7 +389,7 @@ bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb)
 		break;
 
 	case ETH_P_CANXL:
-		if (!can_is_canxl_skb(skb))
+		if (!can_cap_enabled(dev, CAN_CAP_XL) || !can_is_canxl_skb(skb))
 			goto inval_skb;
 		break;
 

base-commit: 7dca8514c51ea5219daf7f28bc1a0536b907e611
-- 
2.53.0

Re: [PATCH net-next 01/11] can: dev: can_dropped_invalid_skb: drop CAN XL frames on non-CAN XL devices

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

[PATCH net-next 02/11] can: skb: make echo skb freeing safe in any IRQ context

[Marc Kleine-Budde]

From: Cunhao Lu <1579567540@qq.com>

can_put_echo_skb() can be called with hardware interrupts disabled. Its
direct drop paths use kfree_skb(), while can_create_echo_skb() uses
kfree_skb() when cloning fails and consume_skb() after a successful clone.
None of these helpers is safe in every IRQ context.

Use dev_kfree_skb_any() for all drop paths and dev_consume_skb_any() when
consuming a successfully cloned skb. This preserves the respective skb drop
and consumed semantics regardless of the caller IRQ context.

Signed-off-by: Cunhao Lu <1579567540@qq.com>
Link: https://patch.msgid.link/tencent_E84809CF236D0137885E7E4E4D58340B3208@qq.com
Cc: stable@kernel.org
Fixes: ae64438be192 ("can: dev: fix skb drop check")
Fixes: 0ae89beb283a ("can: add destructor for self generated skbs")
Fixes: 3ccd4c6167d3 ("can: Unify droping of invalid tx skbs and netdev stats")
Fixes: 39549eef3587 ("can: CAN Network device driver and Netlink interface")
[mkl: also convert can_dropped_invalid_skb(), can_dev_dropped_skb()]
[mkl: add stable on Cc + add Fixes tags]
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>

---

FAQ:

Q: What about the comment in drivers/net/can/virtio_can.c
   virtio_can_start_xmit()? The error handling for virtqueue_add_sgs()
   mentions that can_put_echo_skb() consumes the skb via consume_skb().

A: The comment in virtio_can.c will be updated in a separate series, to
   ease backporting of this patch.

[mkl: add FAQ section]
---
 drivers/net/can/dev/skb.c | 6 +++---
 include/linux/can/dev.h   | 2 +-
 include/linux/can/skb.h   | 4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
index bc0787535b52..46c151eece8f 100644
--- a/drivers/net/can/dev/skb.c
+++ b/drivers/net/can/dev/skb.c
@@ -63,7 +63,7 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
 	    (skb->protocol != htons(ETH_P_CAN) &&
 	     skb->protocol != htons(ETH_P_CANFD) &&
 	     skb->protocol != htons(ETH_P_CANXL))) {
-		kfree_skb(skb);
+		dev_kfree_skb_any(skb);
 		return 0;
 	}
 
@@ -91,7 +91,7 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
 	} else {
 		/* locking problem with netif_stop_queue() ?? */
 		netdev_err(dev, "%s: BUG! echo_skb %d is occupied!\n", __func__, idx);
-		kfree_skb(skb);
+		dev_kfree_skb_any(skb);
 		return -EBUSY;
 	}
 
@@ -403,7 +403,7 @@ bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb)
 	return false;
 
 inval_skb:
-	kfree_skb(skb);
+	dev_kfree_skb_any(skb);
 	dev->stats.tx_dropped++;
 	return true;
 }
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 6d0710d6f571..346faddee02e 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -176,7 +176,7 @@ static inline bool can_dev_dropped_skb(struct net_device *dev, struct sk_buff *s
 	return can_dropped_invalid_skb(dev, skb);
 
 invalid_skb:
-	kfree_skb(skb);
+	dev_kfree_skb_any(skb);
 	dev->stats.tx_dropped++;
 	return true;
 }
diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
index 5d27843862fc..87ace09c0d8b 100644
--- a/include/linux/can/skb.h
+++ b/include/linux/can/skb.h
@@ -78,12 +78,12 @@ static inline struct sk_buff *can_create_echo_skb(struct sk_buff *skb)
 
 	nskb = skb_clone(skb, GFP_ATOMIC);
 	if (unlikely(!nskb)) {
-		kfree_skb(skb);
+		dev_kfree_skb_any(skb);
 		return NULL;
 	}
 
 	can_skb_set_owner(nskb, skb->sk);
-	consume_skb(skb);
+	dev_consume_skb_any(skb);
 	return nskb;
 }
 
-- 
2.53.0

Re: [PATCH net-next 02/11] can: skb: make echo skb freeing safe in any IRQ context

[netdev-bot+sashiko]

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] After this patch, the comment in virtio_can_start_xmit()
  (drivers/net/can/virtio_can.c) still says "can_put_echo_skb() already…

Pre-existing issues:
- [Medium] At this commit, can_put_echo_skb() in
  drivers/net/can/dev/skb.c handles skb ownership differently depending…

--

can: skb: make echo skb freeing safe in any IRQ context

This patch replaces kfree_skb() with dev_kfree_skb_any() on the drop
paths of can_put_echo_skb(), can_dropped_invalid_skb(),
can_dev_dropped_skb() and can_create_echo_skb(). After a successful clone,
can_create_echo_skb() now releases the original skb with
dev_consume_skb_any() instead of consume_skb(). With these changes the
helpers are safe to call with hardware interrupts disabled.

Link: https://patch.msgid.link/tencent_E84809CF236D0137885E7E4E4D58340B3208@qq.com

> diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
> index bc0787535b524..46c151eece8f7 100644
> --- a/drivers/net/can/dev/skb.c
> +++ b/drivers/net/can/dev/skb.c
> @@ -63,7 +63,7 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
>  	    (skb->protocol != htons(ETH_P_CAN) &&
>  	     skb->protocol != htons(ETH_P_CANFD) &&
>  	     skb->protocol != htons(ETH_P_CANXL))) {
> -		kfree_skb(skb);
> +		dev_kfree_skb_any(skb);
>  		return 0;
>  	}
>  
> @@ -91,7 +91,7 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
>  	} else {
>  		/* locking problem with netif_stop_queue() ?? */
>  		netdev_err(dev, "%s: BUG! echo_skb %d is occupied!\n", __func__, idx);
> -		kfree_skb(skb);
> +		dev_kfree_skb_any(skb);
>  		return -EBUSY;
>  	}

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch. Since
these error paths in can_put_echo_skb() are being changed anyway, should
the out-of-bounds index path free the skb as well?

At this commit that path returns without freeing or storing the skb:

drivers/net/can/dev/skb.c:can_put_echo_skb() {
	if (idx >= priv->echo_skb_max) {
		netdev_err(dev, "%s: BUG! Trying to access can_priv::echo_skb out of bounds (%u/max %u)\n",
			   __func__, idx, priv->echo_skb_max);
		return -EINVAL;
	}
    ...
}

The -EBUSY, -ENOMEM and IFF_ECHO/protocol drop paths all free the skb,
and callers depend on that. For example, virtio_can_start_xmit() does:

	err = can_put_echo_skb(skb, dev, can_tx_msg->putidx, 0);
	if (unlikely(err)) {
		/* skb was already freed by can_put_echo_skb() on error */
		...
		goto kick;
	}

Won't the skb leak if a driver passes idx >= priv->echo_skb_max?

The -EINVAL branch comes from commit 6411959c10fe. A later patch in this
series, "can: dev: can_put_echo_skb(): free skb on invalid echo index",
adds dev_kfree_skb_any(skb) before the return -EINVAL. By the end of the
series, every return path consumes the skb.

[ ... ]

> diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
> index 5d27843862fca..87ace09c0d8bd 100644
> --- a/include/linux/can/skb.h
> +++ b/include/linux/can/skb.h
> @@ -78,12 +78,12 @@ static inline struct sk_buff *can_create_echo_skb(struct sk_buff *skb)
>  
>  	nskb = skb_clone(skb, GFP_ATOMIC);
>  	if (unlikely(!nskb)) {
> -		kfree_skb(skb);
> +		dev_kfree_skb_any(skb);
>  		return NULL;
>  	}
>  
>  	can_skb_set_owner(nskb, skb->sk);
> -	consume_skb(skb);
> +	dev_consume_skb_any(skb);

[Severity: Low]
This isn't a bug, but the comment in virtio_can_start_xmit() still names
the old helper:

drivers/net/can/virtio_can.c:virtio_can_start_xmit() {
    ...
		/*
		 * can_put_echo_skb() already consumed skb via consume_skb(),
		 * so returning NETDEV_TX_BUSY would cause the stack to requeue
		 * a freed pointer. Drop the frame and return OK instead.
		 */
    ...
}

What the comment says about ownership is still correct.
dev_consume_skb_any() gives up the skb whether it frees it directly or
defers the free. Only the helper name is out of date.

The commit message FAQ says this comment will be updated in a separate
series to make backporting easier, and it is still unchanged at the end of
this series. Should it say dev_consume_skb_any() instead?

>  	return nskb;
>  }

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

[PATCH net-next 03/11] can: raw: remove redundant NULL check before netdev_hold()

[Marc Kleine-Budde]

From: Sang-Heon Jeon <ekffu200098@gmail.com>

netdev_hold() does nothing if dev is NULL, so the check before the call
is redundant.

So remove it. No functional change.

This is the result of running the Coccinelle script from
scripts/coccinelle/free/ifnulldev_put.cocci after commit f83b8a58695c
("coccinelle: ifnulldev_put: update outdated helper names").

Signed-off-by: Sang-Heon Jeon <ekffu200098@gmail.com>
Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>
Link: https://patch.msgid.link/20260831052031.902699-1-ekffu200098@gmail.com
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 net/can/raw.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/can/raw.c b/net/can/raw.c
index 0acd4f6c6dd6..6944ebab3feb 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -529,8 +529,7 @@ static int raw_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int len
 		ro->bound = 1;
 		/* bind() ok -> hold a reference for new ro->dev */
 		ro->dev = dev;
-		if (ro->dev)
-			netdev_hold(ro->dev, &ro->dev_tracker, GFP_KERNEL);
+		netdev_hold(ro->dev, &ro->dev_tracker, GFP_KERNEL);
 	}
 
 out_put_dev:
-- 
2.53.0

[PATCH net-next 04/11] can: skb: make CAN skb allocation failure paths IRQ-safe

[Marc Kleine-Budde]

From: Cunhao Lu <1579567540@qq.com>

The CAN skb allocation helpers are used from hardware interrupt receive
handlers. If can_skb_ext_add() fails, they release the newly allocated skb
with kfree_skb(), which is not safe in hardware interrupt context.

Use dev_kfree_skb_any() for the allocation failure paths in
alloc_can_skb(), alloc_canfd_skb(), and alloc_canxl_skb().

Fixes: 96ea3a1e2d31 ("can: add CAN skb extension infrastructure")
Signed-off-by: Cunhao Lu <1579567540@qq.com>
Link: https://patch.msgid.link/tencent_C825C17D442F801351CE2FBC4984064B4605@qq.com
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/dev/skb.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
index 46c151eece8f..3e36c0368bae 100644
--- a/drivers/net/can/dev/skb.c
+++ b/drivers/net/can/dev/skb.c
@@ -228,7 +228,7 @@ struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf)
 
 	csx = can_skb_ext_add(skb);
 	if (!csx) {
-		kfree_skb(skb);
+		dev_kfree_skb_any(skb);
 		goto out_error_cc;
 	}
 
@@ -259,7 +259,7 @@ struct sk_buff *alloc_canfd_skb(struct net_device *dev,
 
 	csx = can_skb_ext_add(skb);
 	if (!csx) {
-		kfree_skb(skb);
+		dev_kfree_skb_any(skb);
 		goto out_error_fd;
 	}
 
@@ -297,7 +297,7 @@ struct sk_buff *alloc_canxl_skb(struct net_device *dev,
 
 	csx = can_skb_ext_add(skb);
 	if (!csx) {
-		kfree_skb(skb);
+		dev_kfree_skb_any(skb);
 		goto out_error_xl;
 	}
 
-- 
2.53.0

[PATCH net-next 05/11] can: isotp: check the frame type, not just the length

[Marc Kleine-Budde]

From: Kaixuan Li <kaixuanli0131@gmail.com>

isotp_rcv() separates Classic CAN from CAN FD by skb->len alone:

	if (skb->len != so->ll.mtu)
		return;

	cf = (struct canfd_frame *)skb->data;

A CAN XL frame with cxl->len 4 is CAN_MTU bytes, so it passes, and is then
read as a canfd_frame whose len comes out of canxl_frame.flags: at least
0x80.

Of the paths that follow, only the flow control one uses that length
without bounding it first, so check_pad() walks to 255 over a 16-byte
frame and the caller reports EBADMSG on an unrelated socket.

bcm_rx_handler(), j1939_can_recv(), can_can_gw_rcv() and raw_rcv() check
the frame type here, and can_dropped_invalid_skb() switches on
skb->protocol on the transmit side. isotp_rcv() is the gap.

Fixes: fb08cba12b52 ("can: canxl: update CAN infrastructure for CAN XL frames")
Signed-off-by: Kaixuan Li <kaixuanli0131@gmail.com>
Reviewed-by: Oliver Hartkopp <socketcan@hartkopp.net>
Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>
Reviewed-by: Quchaosheng <quchaosheng000406@163.com>
Link: https://patch.msgid.link/20260920035626.2581040-1-kaixuanli0131@gmail.com
Cc: stable@vger.kernel.org
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 net/can/isotp.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/net/can/isotp.c b/net/can/isotp.c
index f5dc9d04bd68..d3e43ab48425 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -755,6 +755,14 @@ static void isotp_rcv(struct sk_buff *skb, void *data)
 	if (skb->len != so->ll.mtu)
 		return;
 
+	/* check for correct CAN CC/FD frame content */
+	if (so->ll.mtu == CAN_MTU) {
+		if (!can_is_can_skb(skb))
+			return;
+	} else if (!can_is_canfd_skb(skb)) {
+		return;
+	}
+
 	cf = (struct canfd_frame *)skb->data;
 
 	/* if enabled: check reception of my configured extended address */
-- 
2.53.0

[PATCH net-next 06/11] can: proc: reset pkg_stats atomics individually

[Marc Kleine-Budde]

From: Runyu Xiao <runyu.xiao@seu.edu.cn>

Commit 80b5f90158d1 ("can: statistics: use atomic access in hot path")
converted several members of struct can_pkg_stats to atomic_long_t and
updated the hot TX/RX and procfs read paths to use atomic_long_*()
helpers. However, can_init_stats() still clears the whole struct with
memset().

can_init_stats() is reached from can_stat_update() in timer context and
also from the procfs reset path. Those paths can run while the TX/RX hot
paths are concurrently updating rx_frames, tx_frames, matches and their
*_delta counters. Hitting the whole-struct memset() in that window
performs plain writes to fields that otherwise follow an atomic_long_t
access contract, which can lose or mix live statistics updates.

This issue was found by source-level API-misuse analysis looking for
whole-object resets left behind after atomic_long_t conversions, then
manually audited on Linux v6.18.21.

Replace the whole-struct memset() with a helper that resets the
atomic_long_t counters via atomic_long_set() and clears the derived
scalar statistics explicitly. This preserves the existing reset
semantics for scalar fields while restoring atomic access discipline for
the live counters.

Build-tested by compiling net/can/proc.o on x86_64 netdev/main.
Runtime-tested with a QEMU + vcan setup on Linux v6.18.21 by driving
concurrent traffic and reset_stats reads, which reproduced inconsistent
exported statistics before the fix.

[Hartkopp] can_stat_update() also read the *_delta counters at the
beginning and cleared them with atomic_long_set() at the end. Increments
from the hot path in between were lost for both the current and the next
interval. Use atomic_long_xchg() to fetch and clear the *_delta counters
in one atomic step instead.

Fixes: 80b5f90158d1 ("can: statistics: use atomic access in hot path")
Cc: stable@vger.kernel.org
Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>
Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
Link: https://patch.msgid.link/20261020203257.230955-2-socketcan@hartkopp.net
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 net/can/proc.c | 39 +++++++++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 10 deletions(-)

diff --git a/net/can/proc.c b/net/can/proc.c
index de4d05ae3459..70104f13077e 100644
--- a/net/can/proc.c
+++ b/net/can/proc.c
@@ -76,16 +76,39 @@ static const char rx_list_name[][8] = {
  * af_can statistics stuff
  */
 
+static void can_reset_pkg_stats(struct can_pkg_stats *pkg_stats)
+{
+	atomic_long_set(&pkg_stats->rx_frames, 0);
+	atomic_long_set(&pkg_stats->tx_frames, 0);
+	atomic_long_set(&pkg_stats->matches, 0);
+
+	pkg_stats->total_rx_rate = 0;
+	pkg_stats->total_tx_rate = 0;
+	pkg_stats->total_rx_match_ratio = 0;
+
+	pkg_stats->current_rx_rate = 0;
+	pkg_stats->current_tx_rate = 0;
+	pkg_stats->current_rx_match_ratio = 0;
+
+	pkg_stats->max_rx_rate = 0;
+	pkg_stats->max_tx_rate = 0;
+	pkg_stats->max_rx_match_ratio = 0;
+
+	atomic_long_set(&pkg_stats->rx_frames_delta, 0);
+	atomic_long_set(&pkg_stats->tx_frames_delta, 0);
+	atomic_long_set(&pkg_stats->matches_delta, 0);
+}
+
 static void can_init_stats(struct net *net)
 {
 	struct can_pkg_stats *pkg_stats = net->can.pkg_stats;
 	struct can_rcv_lists_stats *rcv_lists_stats = net->can.rcv_lists_stats;
 	/*
-	 * This memset function is called from a timer context (when
+	 * This stats reset is called from a timer context (when
 	 * can_stattimer is active which is the default) OR in a process
 	 * context (reading the proc_fs when can_stattimer is disabled).
 	 */
-	memset(pkg_stats, 0, sizeof(struct can_pkg_stats));
+	can_reset_pkg_stats(pkg_stats);
 	pkg_stats->jiffies_init = jiffies;
 
 	rcv_lists_stats->stats_reset++;
@@ -121,9 +144,10 @@ void can_stat_update(struct timer_list *t)
 	long rx_frames = atomic_long_read(&pkg_stats->rx_frames);
 	long tx_frames = atomic_long_read(&pkg_stats->tx_frames);
 	long matches = atomic_long_read(&pkg_stats->matches);
-	long rx_frames_delta = atomic_long_read(&pkg_stats->rx_frames_delta);
-	long tx_frames_delta = atomic_long_read(&pkg_stats->tx_frames_delta);
-	long matches_delta = atomic_long_read(&pkg_stats->matches_delta);
+	/* fetch and clear values for 'current rate' calculation */
+	long rx_frames_delta = atomic_long_xchg(&pkg_stats->rx_frames_delta, 0);
+	long tx_frames_delta = atomic_long_xchg(&pkg_stats->tx_frames_delta, 0);
+	long matches_delta = atomic_long_xchg(&pkg_stats->matches_delta, 0);
 
 	/* restart counting in timer context on user request */
 	if (user_reset)
@@ -172,11 +196,6 @@ void can_stat_update(struct timer_list *t)
 	if (pkg_stats->max_rx_match_ratio < pkg_stats->current_rx_match_ratio)
 		pkg_stats->max_rx_match_ratio = pkg_stats->current_rx_match_ratio;
 
-	/* clear values for 'current rate' calculation */
-	atomic_long_set(&pkg_stats->tx_frames_delta, 0);
-	atomic_long_set(&pkg_stats->rx_frames_delta, 0);
-	atomic_long_set(&pkg_stats->matches_delta, 0);
-
 	/* restart timer (one second) */
 	mod_timer(&net->can.stattimer, round_jiffies(jiffies + HZ));
 }
-- 
2.53.0

[PATCH net-next 07/11] can: proc: skip stats calculation after reset in can_stat_update()

[Marc Kleine-Budde]

From: Oliver Hartkopp <socketcan@hartkopp.net>

Commit 80b5f90158d1 ("can: statistics: use atomic access in hot path")
introduced local snapshots of the statistics counters at the beginning
of can_stat_update(). When can_init_stats() resets the statistics, these
stale pre-reset snapshots are still used for the following total/current
/max calculations and overwrite the freshly reset values. Additionally a
jiffies tick between the snapshot and the reset could trigger a second
can_init_stats() call.

Merge the reset conditions into a single check and skip the calculation
for the current interval when the statistics have been reset.

Fixes: 80b5f90158d1 ("can: statistics: use atomic access in hot path")
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
Link: https://patch.msgid.link/20261020203257.230955-3-socketcan@hartkopp.net
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 net/can/proc.c | 30 ++++++++++++------------------
 1 file changed, 12 insertions(+), 18 deletions(-)

diff --git a/net/can/proc.c b/net/can/proc.c
index 70104f13077e..fb144d2adc35 100644
--- a/net/can/proc.c
+++ b/net/can/proc.c
@@ -149,25 +149,18 @@ void can_stat_update(struct timer_list *t)
 	long tx_frames_delta = atomic_long_xchg(&pkg_stats->tx_frames_delta, 0);
 	long matches_delta = atomic_long_xchg(&pkg_stats->matches_delta, 0);
 
-	/* restart counting in timer context on user request */
-	if (user_reset)
-		can_init_stats(net);
-
-	/* restart counting on jiffies overflow */
-	if (j < pkg_stats->jiffies_init)
-		can_init_stats(net);
-
-	/* prevent overflow in calc_rate() */
-	if (rx_frames > (LONG_MAX / HZ))
-		can_init_stats(net);
-
-	/* prevent overflow in calc_rate() */
-	if (tx_frames > (LONG_MAX / HZ))
-		can_init_stats(net);
-
-	/* matches overflow - very improbable */
-	if (matches > (LONG_MAX / 100))
+	/*
+	 * Restart counting in timer context on user request, on jiffies
+	 * overflow or to prevent overflows in calc_rate() and in the
+	 * match ratio calculation. The local snapshots are stale after
+	 * the reset, so skip the calculation for this interval.
+	 */
+	if (user_reset || j < pkg_stats->jiffies_init ||
+	    rx_frames > (LONG_MAX / HZ) || tx_frames > (LONG_MAX / HZ) ||
+	    matches > (LONG_MAX / 100)) {
 		can_init_stats(net);
+		goto out;
+	}
 
 	/* calc total values */
 	if (rx_frames)
@@ -196,6 +189,7 @@ void can_stat_update(struct timer_list *t)
 	if (pkg_stats->max_rx_match_ratio < pkg_stats->current_rx_match_ratio)
 		pkg_stats->max_rx_match_ratio = pkg_stats->current_rx_match_ratio;
 
+out:
 	/* restart timer (one second) */
 	mod_timer(&net->can.stattimer, round_jiffies(jiffies + HZ));
 }
-- 
2.53.0

[PATCH net-next 08/11] can: proc: remove pointers from CAN specific proc output

[Marc Kleine-Budde]

From: Oliver Hartkopp <socketcan@hartkopp.net>

The proc content in /proc/net/can/ and /proc/net/can-bcm/ is intended to
check the internal filter lists (af_can, can_raw) and the efficiency and
functionality of can_bcm jobs. While exposing kernel addresses was
acceptable when this code was written the times have changed and multiple
attempts have been made to hash or remove such now sensitive data.

This patch removes the disclosure of pointers and instead provides the
function names and sock inode numbers when available. When the function
name can not be resolved (e.g. KALLSYMS=n or stripped module symbols)
%ps would fall back to the raw text address. Therefore (unknown) is
printed instead in this case. As there's no known tooling around the CAN
specific proc output breaking the ABI with this rework creates no issue.

Suggested-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
Link: https://patch.msgid.link/20261020203257.230955-4-socketcan@hartkopp.net
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 Documentation/networking/can.rst | 16 ++++++++++------
 include/linux/can/core.h         |  2 +-
 net/can/af_can.c                 |  6 +++---
 net/can/af_can.h                 |  2 +-
 net/can/bcm.c                    | 14 +++++++-------
 net/can/gw.c                     |  2 +-
 net/can/isotp.c                  |  4 ++--
 net/can/j1939/main.c             |  2 +-
 net/can/proc.c                   | 26 ++++++++++++++++----------
 net/can/raw.c                    |  4 ++--
 10 files changed, 44 insertions(+), 34 deletions(-)

diff --git a/Documentation/networking/can.rst b/Documentation/networking/can.rst
index 536ff411da1d..51bc70748f75 100644
--- a/Documentation/networking/can.rst
+++ b/Documentation/networking/can.rst
@@ -1040,17 +1040,21 @@ As described in :ref:`socketcan-receive-lists` the SocketCAN core uses several f
 lists to deliver received CAN frames to CAN protocol modules. These
 receive lists, their filters and the count of filter matches can be
 checked in the appropriate receive list. All entries contain the
-device and a protocol module identifier::
+CAN network device, the filter (can_id/can_mask), the number of filter
+matches, the inode number of the subscribing socket and the name of the
+receive function. A sock_inode of 0 denotes a kernel internal subscriber
+without a socket (e.g. CAN gateway or J1939). When the function name can
+not be resolved (e.g. with CONFIG_KALLSYMS=n) "(unknown)" is printed::
 
     foo@bar:~$ cat /proc/net/can/rcvlist_all
 
     receive list 'rx_all':
-      (vcan3: no entry)
-      (vcan2: no entry)
-      (vcan1: no entry)
-      device   can_id   can_mask  function  userdata   matches  ident
-       vcan0     000    00000000  f88e6370  f6c6f400         0  raw
       (any: no entry)
+      device   can_id   can_mask   matches  sock_inode  function
+       vcan0     000    00000000         0       63586  raw_rcv [can_raw]
+      (vcan1: no entry)
+      (vcan2: no entry)
+      (vcan3: no entry)
 
 In this example an application requests any CAN traffic from vcan0::
 
diff --git a/include/linux/can/core.h b/include/linux/can/core.h
index 2de74c2b78b6..effb5c31ef40 100644
--- a/include/linux/can/core.h
+++ b/include/linux/can/core.h
@@ -51,7 +51,7 @@ extern void can_proto_unregister(const struct can_proto *cp);
 int can_rx_register(struct net *net, struct net_device *dev,
 		    canid_t can_id, canid_t mask,
 		    void (*func)(struct sk_buff *, void *),
-		    void *data, char *ident, struct sock *sk);
+		    void *data, u64 ino, struct sock *sk);
 
 extern void can_rx_unregister(struct net *net, struct net_device *dev,
 			      canid_t can_id, canid_t mask,
diff --git a/net/can/af_can.c b/net/can/af_can.c
index 34fe3b28d576..6ebe9d3689ac 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -418,7 +418,7 @@ static struct hlist_head *can_rcv_list_find(canid_t *can_id, canid_t *mask,
  * @mask: CAN mask (see description)
  * @func: callback function on filter match
  * @data: returned parameter for callback function
- * @ident: string for calling module identification
+ * @ino: inode number of sock (0 = unknown)
  * @sk: socket pointer (might be NULL)
  *
  * Description:
@@ -443,7 +443,7 @@ static struct hlist_head *can_rcv_list_find(canid_t *can_id, canid_t *mask,
  */
 int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
 		    canid_t mask, void (*func)(struct sk_buff *, void *),
-		    void *data, char *ident, struct sock *sk)
+		    void *data, u64 ino, struct sock *sk)
 {
 	struct receiver *rcv;
 	struct hlist_head *rcv_list;
@@ -472,7 +472,7 @@ int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
 	atomic_long_set(&rcv->matches, 0);
 	rcv->func = func;
 	rcv->data = data;
-	rcv->ident = ident;
+	rcv->ino = ino;
 	rcv->sk = sk;
 
 	hlist_add_head_rcu(&rcv->list, rcv_list);
diff --git a/net/can/af_can.h b/net/can/af_can.h
index 87887014f562..6e593ed5db5f 100644
--- a/net/can/af_can.h
+++ b/net/can/af_can.h
@@ -55,7 +55,7 @@ struct receiver {
 	atomic_long_t matches;
 	void (*func)(struct sk_buff *skb, void *data);
 	void *data;
-	char *ident;
+	u64 ino;
 	struct sock *sk;
 	struct rcu_head rcu;
 };
diff --git a/net/can/bcm.c b/net/can/bcm.c
index 3d637a1e0ac1..6c85a76477c3 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -220,9 +220,7 @@ static int bcm_proc_show(struct seq_file *m, void *v)
 	struct bcm_sock *bo = bcm_sk(sk);
 	struct bcm_op *op;
 
-	seq_printf(m, ">>> socket %pK", sk->sk_socket);
-	seq_printf(m, " / sk %pK", sk);
-	seq_printf(m, " / bo %pK", bo);
+	seq_printf(m, ">>> sock inode %s", bo->procname);
 	seq_printf(m, " / dropped %lu", bo->dropped_usr_msgs);
 	seq_printf(m, " / bound %s", bcm_proc_getifname(net, ifname, bo->ifindex));
 	seq_printf(m, " <<<\n");
@@ -1536,7 +1534,7 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
 						      op->can_id,
 						      REGMASK(op->can_id),
 						      bcm_rx_handler, op,
-						      "bcm", sk);
+						      sock_i_ino(sk), sk);
 
 				/* keep a tracked reference so that a later
 				 * unregister can safely reach the device even
@@ -1560,7 +1558,8 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
 		} else {
 			err = can_rx_register(sock_net(sk), NULL, op->can_id,
 					      REGMASK(op->can_id),
-					      bcm_rx_handler, op, "bcm", sk);
+					      bcm_rx_handler, op,
+					      sock_i_ino(sk), sk);
 		}
 
 		if (err) {
@@ -2040,8 +2039,9 @@ static int bcm_connect(struct socket *sock, struct sockaddr_unsized *uaddr, int
 
 #if IS_ENABLED(CONFIG_PROC_FS)
 	if (net->can.bcmproc_dir) {
-		/* unique socket address as filename */
-		sprintf(bo->procname, "%llu", sock_i_ino(sk));
+		/* use unique socket inode number as filename */
+		snprintf(bo->procname, sizeof(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);
diff --git a/net/can/gw.c b/net/can/gw.c
index 5793cb2420ed..36f9aa0f7026 100644
--- a/net/can/gw.c
+++ b/net/can/gw.c
@@ -579,7 +579,7 @@ static inline int cgw_register_filter(struct net *net, struct cgw_job *gwj)
 {
 	return can_rx_register(net, gwj->src.dev, gwj->ccgw.filter.can_id,
 			       gwj->ccgw.filter.can_mask, can_can_gw_rcv,
-			       gwj, "gw", NULL);
+			       gwj, 0, NULL);
 }
 
 static inline void cgw_unregister_filter(struct net *net, struct cgw_job *gwj)
diff --git a/net/can/isotp.c b/net/can/isotp.c
index d3e43ab48425..64ff203a37f4 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -1633,14 +1633,14 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
 
 	if (isotp_register_rxid(so))
 		can_rx_register(net, dev, rx_id, SINGLE_MASK(rx_id),
-				isotp_rcv, sk, "isotp", sk);
+				isotp_rcv, sk, sock_i_ino(sk), sk);
 
 	/* no consecutive frame echo skb in flight */
 	WRITE_ONCE(so->cfecho, 0);
 
 	/* register for echo skb's */
 	can_rx_register(net, dev, tx_id, SINGLE_MASK(tx_id),
-			isotp_rcv_echo, sk, "isotpe", sk);
+			isotp_rcv_echo, sk, sock_i_ino(sk), sk);
 
 	/* switch to new settings */
 	so->ifindex = ifindex;
diff --git a/net/can/j1939/main.c b/net/can/j1939/main.c
index 5e5e6c228f22..d9384cf0e356 100644
--- a/net/can/j1939/main.c
+++ b/net/can/j1939/main.c
@@ -184,7 +184,7 @@ static int j1939_can_rx_register(struct j1939_priv *priv)
 
 	j1939_priv_get(priv);
 	ret = can_rx_register(dev_net(ndev), ndev, J1939_CAN_ID, J1939_CAN_MASK,
-			      j1939_can_recv, priv, "j1939", NULL);
+			      j1939_can_recv, priv, 0, NULL);
 	if (ret < 0) {
 		j1939_priv_put(priv);
 		return ret;
diff --git a/net/can/proc.c b/net/can/proc.c
index fb144d2adc35..b7496d2a37cd 100644
--- a/net/can/proc.c
+++ b/net/can/proc.c
@@ -204,26 +204,32 @@ static void can_print_rcvlist(struct seq_file *m, struct hlist_head *rx_list,
 	struct receiver *r;
 
 	hlist_for_each_entry_rcu(r, rx_list, list) {
+		char symcheck[4];
 		char *fmt = (r->can_id & CAN_EFF_FLAG)?
-			"   %-5s  %08x  %08x  %pK  %pK  %8ld  %s\n" :
-			"   %-5s     %03x    %08x  %pK  %pK  %8ld  %s\n";
+			"  %6s  %08x  %08x  %8ld  %10llu" :
+			"  %6s     %03x    %08x  %8ld  %10llu";
 
 		seq_printf(m, fmt, DNAME(dev), r->can_id, r->mask,
-			   r->func, r->data, atomic_long_read(&r->matches),
-			   r->ident);
+			   atomic_long_read(&r->matches), r->ino);
+
+		/* %ps falls back to the raw text address when the symbol
+		 * can not be resolved (KALLSYMS=n, stripped module symbols)
+		 */
+		snprintf(symcheck, sizeof(symcheck), "%ps", r->func);
+		if (strncmp(symcheck, "0x", 2))
+			seq_printf(m, "  %ps\n", r->func);
+		else
+			seq_puts(m, "  (unknown)\n");
 	}
 }
 
 static void can_print_recv_banner(struct seq_file *m)
 {
 	/*
-	 *                  can1.  00000000  00000000  00000000
-	 *                 .......          0  tp20
+	 *    device   can_id   can_mask   matches  sock_inode  function
+	 *     vcan0     000    00000000         0       43798  raw_rcv [can_raw]
 	 */
-	if (IS_ENABLED(CONFIG_64BIT))
-		seq_puts(m, "  device   can_id   can_mask      function          userdata       matches  ident\n");
-	else
-		seq_puts(m, "  device   can_id   can_mask  function  userdata   matches  ident\n");
+	seq_puts(m, "  device   can_id   can_mask   matches  sock_inode  function\n");
 }
 
 static int can_stats_proc_show(struct seq_file *m, void *v)
diff --git a/net/can/raw.c b/net/can/raw.c
index 6944ebab3feb..3e57f72a3afe 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -226,7 +226,7 @@ static int raw_enable_filters(struct net *net, struct net_device *dev,
 	for (i = 0; i < count; i++) {
 		err = can_rx_register(net, dev, filter[i].can_id,
 				      filter[i].can_mask,
-				      raw_rcv, sk, "raw", sk);
+				      raw_rcv, sk, sock_i_ino(sk), sk);
 		if (err) {
 			/* clean up successfully registered filters */
 			while (--i >= 0)
@@ -247,7 +247,7 @@ static int raw_enable_errfilter(struct net *net, struct net_device *dev,
 
 	if (err_mask)
 		err = can_rx_register(net, dev, 0, err_mask | CAN_ERR_FLAG,
-				      raw_rcv, sk, "raw", sk);
+				      raw_rcv, sk, sock_i_ino(sk), sk);
 
 	return err;
 }
-- 
2.53.0

[PATCH net-next 09/11] can: replace ARPHRD_CAN type checks with can_get_ml_priv()

[Marc Kleine-Budde]

From: Oliver Hartkopp <socketcan@hartkopp.net>

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. A valid pointer
returned by can_get_ml_priv() therefore identifies a real CAN device.

The referenced commit only converted the j1939 and proc code to this
check, while the other parts of the CAN subsystem still rely on
dev->type == ARPHRD_CAN.

The TUNSETLINK ioctl allows a down TUN/TAP device to set its hardware
type to ARPHRD_CAN while dev->ml_priv remains NULL. Such a repurposed
netdevice passes the ARPHRD_CAN checks.

Commit 8aa59e355949 ("can: af_can: fix NULL pointer dereference in
can_rx_register()") already made can_rx_register() reject such devices.
But can_rx_unregister() still only checks dev->type and dereferences
the NULL ml_priv when it walks the receive lists of the device.

CAN_ISOTP reaches this path: isotp_bind() ignores the return values of
can_rx_register() and binds the socket anyway. When the socket is
released or the device is unregistered, isotp_release() and
isotp_notify() remove the filters that have never been registered and
trigger the NULL pointer dereference in can_rx_unregister(). This only
requires owning a TUN/TAP device or CAP_NET_ADMIN in an unprivileged
user namespace.

Convert the remaining ARPHRD_CAN checks in af_can, bcm, gw, isotp and
raw to can_get_ml_priv().

Fixes: 4e096a18867a ("net: introduce CAN specific pointer in the struct net_device")
Cc: stable@kernel.org
Cc: Oleksij Rempel <o.rempel@pengutronix.de>
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
Link: https://patch.msgid.link/20261012164802.94113-1-socketcan@hartkopp.net
Link: https://patch.msgid.link/20261020203257.230955-5-socketcan@hartkopp.net
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 net/can/af_can.c | 15 ++++++---------
 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(+), 19 deletions(-)

diff --git a/net/can/af_can.c b/net/can/af_can.c
index 6ebe9d3689ac..d97f85fc3232 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -226,7 +226,7 @@ int can_send(struct sk_buff *skb, int loop)
 		goto inval_skb;
 	}
 
-	if (unlikely(skb->dev->type != ARPHRD_CAN)) {
+	if (unlikely(!can_get_ml_priv(skb->dev))) {
 		err = -EPERM;
 		goto inval_skb;
 	}
@@ -452,7 +452,7 @@ int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
 
 	/* 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)))
@@ -519,7 +519,7 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
 	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)))
@@ -684,8 +684,7 @@ static int can_rcv(struct sk_buff *skb, struct net_device *dev,
 {
 	struct can_skb_ext *csx = can_skb_ext_find(skb);
 
-	if (unlikely(dev->type != ARPHRD_CAN || !can_get_ml_priv(dev) ||
-		     !csx || !can_is_can_skb(skb))) {
+	if (unlikely(!can_get_ml_priv(dev) || !csx || !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);
 
@@ -710,8 +709,7 @@ static int canfd_rcv(struct sk_buff *skb, struct net_device *dev,
 {
 	struct can_skb_ext *csx = can_skb_ext_find(skb);
 
-	if (unlikely(dev->type != ARPHRD_CAN || !can_get_ml_priv(dev) ||
-		     !csx || !can_is_canfd_skb(skb))) {
+	if (unlikely(!can_get_ml_priv(dev) || !csx || !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);
 
@@ -736,8 +734,7 @@ static int canxl_rcv(struct sk_buff *skb, struct net_device *dev,
 {
 	struct can_skb_ext *csx = can_skb_ext_find(skb);
 
-	if (unlikely(dev->type != ARPHRD_CAN || !can_get_ml_priv(dev) ||
-		     !csx || !can_is_canxl_skb(skb))) {
+	if (unlikely(!can_get_ml_priv(dev) || !csx || !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);
 
diff --git a/net/can/bcm.c b/net/can/bcm.c
index 6c85a76477c3..6759662e024c 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -54,6 +54,7 @@
 #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>
@@ -1718,7 +1719,7 @@ static int bcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 				goto out_release;
 			}
 
-			if (dev->type != ARPHRD_CAN) {
+			if (!can_get_ml_priv(dev)) {
 				dev_put(dev);
 				ret = -ENODEV;
 				goto out_release;
@@ -1866,7 +1867,7 @@ static int bcm_notifier(struct notifier_block *nb, unsigned long msg,
 {
 	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;
@@ -2023,7 +2024,7 @@ static int bcm_connect(struct socket *sock, struct sockaddr_unsized *uaddr, int
 			ret = -ENODEV;
 			goto fail;
 		}
-		if (dev->type != ARPHRD_CAN) {
+		if (!can_get_ml_priv(dev)) {
 			dev_put(dev);
 			ret = -ENODEV;
 			goto fail;
diff --git a/net/can/gw.c b/net/can/gw.c
index 36f9aa0f7026..8a5327d8a00e 100644
--- a/net/can/gw.c
+++ b/net/can/gw.c
@@ -52,6 +52,7 @@
 #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>
@@ -612,7 +613,7 @@ static int cgw_notifier(struct notifier_block *nb,
 	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) {
@@ -1163,7 +1164,7 @@ static int cgw_create_job(struct sk_buff *skb,  struct nlmsghdr *nlh,
 	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);
@@ -1171,7 +1172,7 @@ static int cgw_create_job(struct sk_buff *skb,  struct nlmsghdr *nlh,
 	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? */
diff --git a/net/can/isotp.c b/net/can/isotp.c
index 64ff203a37f4..5e1c3971ebfc 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -65,6 +65,7 @@
 #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>
@@ -1618,7 +1619,7 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
 		err = -ENODEV;
 		goto out;
 	}
-	if (dev->type != ARPHRD_CAN) {
+	if (!can_get_ml_priv(dev)) {
 		err = -ENODEV;
 		goto out_put_dev;
 	}
@@ -1905,7 +1906,7 @@ static int isotp_notifier(struct notifier_block *nb, unsigned long msg,
 {
 	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;
diff --git a/net/can/raw.c b/net/can/raw.c
index 3e57f72a3afe..dfea48768b23 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -348,7 +348,7 @@ static int raw_notifier(struct notifier_block *nb, unsigned long msg,
 {
 	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;
@@ -491,7 +491,7 @@ static int raw_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int len
 			err = -ENODEV;
 			goto out;
 		}
-		if (dev->type != ARPHRD_CAN) {
+		if (!can_get_ml_priv(dev)) {
 			err = -ENODEV;
 			goto out_put_dev;
 		}
-- 
2.53.0

[PATCH net-next 10/11] can: dev: can_put_echo_skb(): free skb on invalid echo index

[Marc Kleine-Budde]

From: Cunhao Lu <1579567540@qq.com>

can_put_echo_skb() consumes the skb on all paths except when the echo
index is out of bounds. This leaves ownership with the caller on -EINVAL,
unlike the other error paths, and can leak the skb if the caller expects
consistent semantics.

Free the skb before returning -EINVAL so that all return paths consume it.

Fixes: 6411959c10fe ("can: dev: can_put_echo_skb(): don't crash kernel if can_priv::echo_skb is accessed out of bounds")
Cc: stable@vger.kernel.org
Reviewed-by: Vincent Mailhol <mailhol@kernel.org>
Signed-off-by: Cunhao Lu <1579567540@qq.com>
Link: https://patch.msgid.link/tencent_683AA16E643DE00211CD2FB62991264DC605@qq.com
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>

---

FAQ:

Q: `can_skb_init_valid()` directly modifies `skb->data` without checking if
   the SKB is cloned or shared. Is this violating SKB shared buffer rules and
   causing data corruption?

A: The finding about can_skb_init_valid() modifying skb->data on a
   potentially shared buffer is not relevant for this (correct) patch.

   The FDF-flag write in can_skb_init_valid() only matters for PF_PACKET
   use: PF_CAN allocators already set CANFD_FDF. The write exists to
   normalize frames for PF_PACKET observers (tcpdump/Wireshark) so they can
   distinguish classic CAN from CAN FD at the netdev level, and to
   compensate for PF_PACKET senders that omit the bit. That normalization
   is functionally required.

   The shared-buffer concern only becomes realistic through a specific
   chain: a PF_PACKET sender injects a CANFD-sized frame, the CAN driver's
   loopback puts an echo skb back into can_rcv(), and cgw (without
   modfuncs) clones it for forwarding to another interface. Only on that
   second can_skb_init_valid() call is the skb cloned. By then the bit was
   already set on the exclusive skb during the initial xmit path, so the
   flags |= CANFD_FDF write is strictly idempotent for any parallel
   consumer of the shared buffer - no memory-safety or
   information-disclosure consequence. It's a formal violation of the skb
   sharing rules without an observable effect, so the current code can stay
   as-is.

Link: https://lore.kernel.org/all/54a3cc01-abcf-4a33-b932-39bb1f68cdd5@hartkopp.net/
[mkl: convert Oliver's mail to FAQ section]
---
 drivers/net/can/dev/skb.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
index 3e36c0368bae..738d5968dfa6 100644
--- a/drivers/net/can/dev/skb.c
+++ b/drivers/net/can/dev/skb.c
@@ -55,6 +55,7 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
 	if (idx >= priv->echo_skb_max) {
 		netdev_err(dev, "%s: BUG! Trying to access can_priv::echo_skb out of bounds (%u/max %u)\n",
 			   __func__, idx, priv->echo_skb_max);
+		dev_kfree_skb_any(skb);
 		return -EINVAL;
 	}
 
-- 
2.53.0

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

[Marc Kleine-Budde]

From: Oliver Hartkopp <socketcan@hartkopp.net>

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() leave their notifier list before they
take any lock that serializes against the netdev notifier, which runs
under RTNL. When the bound device is moved to another namespace in this
window, the notifier skips the socket and the release path has to remove
the filters itself. isotp_release() passes the socket's namespace, which
does not match dev_net(dev) anymore. bcm_rx_unreg() passes dev_net(dev),
which can_rx_unregister() reads again for its check, so a namespace
change between both reads makes the check fail as well. In both cases
can_rx_unregister() silently returns. As can_rx_register() stores
rcv->sk without a reference, the remaining receivers point to the freed
socket (isotp) or the freed op (bcm):

  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

Besides this, the lists of a device are protected by the rcvlists_lock
of the namespace that is passed in. Callers which pass dev_net(dev) read
it at different times, so the same lists may be modified under the locks
of different namespaces while the device is moved.

Taking RTNL in these release paths is no option: raw_bind(),
raw_setsockopt() and raw_release() already hold RTNL while adding and
removing up to CAN_RAW_FILTER_MAX filters per socket, which unprivileged
users can trigger repeatedly and which makes RTNL a global bottleneck.

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. It is initialized in
can_set_ml_priv(), which is used by all CAN network devices. With this,
can_rx_unregister() finds the subscription independently of the
current namespace of the device. The protocols keep the device valid
with their tracked references. The per-netns rcvlists_lock only protects
the receive list statistics anymore, and CAN_RAW, CAN_BCM and CAN_ISOTP
pass the namespace that has been used for can_rx_register().

So isotp_release() and bcm_release() now remove their filters correctly
without RTNL, and raw_release(), raw_setsockopt(), raw_bind() and
isotp_bind() do not need RTNL anymore and add and remove their filters
under lock_sock(). To serialize bind() with NETDEV_UNREGISTER, bind()
publishes the device inside the RCU section of its lookup, and
raw_notify() and isotp_notify() recheck the bound device under lock_sock().
As RTNL does not keep NETDEV_DOWN out of bind() anymore, bind() checks
IFF_UP after publishing the bound device, with a barrier pair against
raw_notify() and isotp_notify(), so a down transition during bind() is
still reported with ENETDOWN.

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-5-5
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
Link: https://patch.msgid.link/20261020203257.230955-6-socketcan@hartkopp.net
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 include/linux/can/can-ml.h |   3 +
 include/net/netns/can.h    |   2 +-
 net/can/af_can.c           |  41 ++++++----
 net/can/bcm.c              |  43 +++-------
 net/can/isotp.c            |  48 +++++++++---
 net/can/raw.c              | 155 ++++++++++++++++++++++++-------------
 6 files changed, 181 insertions(+), 111 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
@@ -45,6 +45,7 @@
 #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 */
@@ -59,6 +60,7 @@
 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];
@@ -81,6 +83,7 @@ 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);
 }
 
diff --git a/include/net/netns/can.h b/include/net/netns/can.h
index 48b79f7e6236..54f23681e0d4 100644
--- a/include/net/netns/can.h
+++ b/include/net/netns/can.h
@@ -29,7 +29,7 @@ struct netns_can {
 
 	/* receive filters subscribed for 'all' CAN devices */
 	struct can_dev_rcv_lists *rx_alldev_list;
-	spinlock_t rcvlists_lock;
+	spinlock_t rcvlists_lock; /* protects the rcv_entries(_max) stats */
 	struct timer_list stattimer; /* timer for statistics update */
 	struct can_pkg_stats *pkg_stats;
 	struct can_rcv_lists_stats *rcv_lists_stats;
diff --git a/net/can/af_can.c b/net/can/af_can.c
index d97f85fc3232..0f3051a61aab 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -462,9 +462,10 @@ int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
 	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;
@@ -478,6 +479,9 @@ int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
 	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);
@@ -509,6 +513,14 @@ static void can_rx_delete_receiver(struct rcu_head *rp)
  *
  * 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 should be the namespace that has been used for
+ *  can_rx_register(). It selects the 'any' CAN device lists for
+ *  @dev == NULL and is otherwise only used for the receive list
+ *  statistics, which become imprecise when another namespace is passed.
  */
 void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
 		       canid_t mask, void (*func)(struct sk_buff *, void *),
@@ -522,12 +534,10 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
 	if (dev && !can_get_ml_priv(dev))
 		return;
 
-	if (dev && !net_eq(net, dev_net(dev)))
-		return;
-
-	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);
 
 	/* Search the receiver list for the item to delete.  This should
@@ -547,26 +557,26 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
 	 * 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);
 
@@ -823,6 +833,7 @@ static int can_pernet_init(struct net *net)
 	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;
diff --git a/net/can/bcm.c b/net/can/bcm.c
index 6759662e024c..7bd2aa39777a 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -962,7 +962,7 @@ 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 */
@@ -988,21 +988,10 @@ static int bcm_delete_rx_op(struct list_head *ops, struct bcm_msg_head *mh,
 			/* 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 in bcm_notify(). op->rx_reg_dev keeps
+				 * the device's receive lists accessible.
 				 */
 				if (op->rx_reg_dev)
 					bcm_rx_unreg(op->rx_reg_dev, op);
@@ -1935,6 +1924,7 @@ static int bcm_release(struct socket *sock)
 	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)
@@ -1948,29 +1938,20 @@ static int bcm_release(struct socket *sock)
 		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
+			 * in bcm_notify(). op->rx_reg_dev keeps the device's
+			 * receive lists accessible.
 			 */
 			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();
diff --git a/net/can/isotp.c b/net/can/isotp.c
index 5e1c3971ebfc..8ec37d252557 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -1497,11 +1497,11 @@ static int isotp_release(struct socket *sock)
 	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
+	 * the tracked reference so->dev keeps the device's receive lists
+	 * accessible, even after a namespace change
 	 */
 	if (so->bound && so->dev) {
 		if (isotp_register_rxid(so))
@@ -1519,8 +1519,6 @@ static int isotp_release(struct socket *sock)
 	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
@@ -1588,7 +1586,7 @@ 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) {
@@ -1614,7 +1612,17 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
 		goto out;
 	}
 
-	dev = dev_get_by_index(net, addr->can_ifindex);
+	/* Publish the device inside the RCU section of the lookup: When it
+	 * is unlisted concurrently, either isotp_notify() sees so->dev after
+	 * synchronize_net() or the lookup fails.
+	 */
+	rcu_read_lock();
+	dev = dev_get_by_index_rcu(net, addr->can_ifindex);
+	if (dev) {
+		dev_hold(dev);
+		WRITE_ONCE(so->dev, dev);
+	}
+	rcu_read_unlock();
 	if (!dev) {
 		err = -ENODEV;
 		goto out;
@@ -1627,7 +1635,12 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
 		err = -EINVAL;
 		goto out_put_dev;
 	}
-	if (!(dev->flags & IFF_UP))
+
+	/* Either isotp_notify() sees so->dev on NETDEV_DOWN or we see the
+	 * cleared IFF_UP, pairs with isotp_notify()
+	 */
+	smp_mb();
+	if (!(READ_ONCE(dev->flags) & IFF_UP))
 		notify_enetdown = 1;
 
 	ifindex = dev->ifindex;
@@ -1653,15 +1666,17 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
 	 * can safely reach the device later, even if a concurrent
 	 * NETDEV_UNREGISTER has already unlisted it by ifindex.
 	 */
-	so->dev = dev;
 	netdev_hold(so->dev, &so->dev_tracker, GFP_KERNEL);
 
 out_put_dev:
-	/* remove potential reference from dev_get_by_index() */
+	/* the published so->dev is only valid for a successful bind() */
+	if (err)
+		WRITE_ONCE(so->dev, NULL);
+
+	/* remove reference from the device lookup */
 	dev_put(dev);
 out:
 	release_sock(sk);
-	rtnl_unlock();
 
 	if (notify_enetdown) {
 		sk->sk_err = ENETDOWN;
@@ -1864,12 +1879,21 @@ static void isotp_notify(struct isotp_sock *so, unsigned long msg,
 	if (!net_eq(dev_net(dev), sock_net(sk)))
 		return;
 
-	if (so->dev != dev)
+	if (msg == NETDEV_DOWN)
+		smp_mb(); /* pairs with isotp_bind(), see raw_notify() */
+
+	/* lockless fast path, see raw_notify() */
+	if (READ_ONCE(so->dev) != dev)
 		return;
 
 	switch (msg) {
 	case NETDEV_UNREGISTER:
 		lock_sock(sk);
+		if (so->dev != dev) {
+			release_sock(sk);
+			break;
+		}
+
 		/* remove current filters & unregister */
 		if (so->bound) {
 			if (isotp_register_rxid(so))
@@ -1885,7 +1909,7 @@ static void isotp_notify(struct isotp_sock *so, unsigned long msg,
 
 		so->ifindex = 0;
 		so->bound  = 0;
-		so->dev = NULL;
+		WRITE_ONCE(so->dev, NULL);
 		release_sock(sk);
 
 		sk->sk_err = ENODEV;
diff --git a/net/can/raw.c b/net/can/raw.c
index dfea48768b23..d07f3bb68cd5 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -85,6 +85,7 @@ struct raw_sock {
 	struct sock sk;
 	struct net_device *dev;
 	netdevice_tracker dev_tracker;
+	struct net_device *bind_dev; /* hint for raw_notify(), no reference */
 	struct list_head notifier;
 	int ifindex;
 	unsigned int bound:1;
@@ -224,6 +225,7 @@ static int raw_enable_filters(struct net *net, struct net_device *dev,
 	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);
@@ -258,9 +260,11 @@ static void raw_disable_filters(struct net *net, struct net_device *dev,
 {
 	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,
@@ -305,16 +309,31 @@ static void raw_notify(struct raw_sock *ro, unsigned long msg,
 		       struct net_device *dev)
 {
 	struct sock *sk = &ro->sk;
+	struct net_device *bind_dev;
 
 	if (!net_eq(dev_net(dev), sock_net(sk)))
 		return;
 
-	if (ro->dev != dev)
+	if (msg == NETDEV_DOWN)
+		smp_mb(); /* IFF_UP is cleared before, pairs with raw_bind() */
+
+	/* Lockless fast path, ro->bind_dev covers a concurrent bind().
+	 * Read it before ro->dev, pairs with smp_store_release() in
+	 * raw_bind(). A stale ro->bind_dev only costs a lock_sock().
+	 */
+	bind_dev = smp_load_acquire(&ro->bind_dev);
+	if (READ_ONCE(ro->dev) != dev &&
+	    (msg != NETDEV_UNREGISTER || bind_dev != dev))
 		return;
 
 	switch (msg) {
 	case NETDEV_UNREGISTER:
 		lock_sock(sk);
+		if (ro->dev != dev) {
+			release_sock(sk);
+			break;
+		}
+
 		/* remove current filters & unregister */
 		if (ro->bound) {
 			raw_disable_allfilters(dev_net(dev), dev, sk);
@@ -326,7 +345,7 @@ static void raw_notify(struct raw_sock *ro, unsigned long msg,
 
 		ro->ifindex = 0;
 		ro->bound = 0;
-		ro->dev = NULL;
+		WRITE_ONCE(ro->dev, NULL);
 		ro->count = 0;
 		release_sock(sk);
 
@@ -381,6 +400,7 @@ static int raw_init(struct sock *sk)
 	ro->bound            = 0;
 	ro->ifindex          = 0;
 	ro->dev              = NULL;
+	ro->bind_dev         = NULL;
 
 	/* set default filter to single entry dfilter */
 	ro->dfilter.can_id   = 0;
@@ -431,13 +451,15 @@ static int raw_release(struct socket *sock)
 	list_del(&ro->notifier);
 	spin_unlock(&raw_notifier_lock);
 
-	rtnl_lock();
 	lock_sock(sk);
 
-	/* remove current filters & unregister */
+	/* remove current filters & unregister
+	 * the tracked reference ro->dev keeps the device's receive lists
+	 * accessible, even after a namespace change
+	 */
 	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);
@@ -456,7 +478,6 @@ static int raw_release(struct socket *sock)
 	sock->sk = NULL;
 
 	release_sock(sk);
-	rtnl_unlock();
 
 	sock_prot_inuse_add(net, sk->sk_prot, -1);
 	sock_put(sk);
@@ -479,14 +500,25 @@ static int raw_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int len
 	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;
 
 	if (addr->can_ifindex) {
-		dev = dev_get_by_index(sock_net(sk), addr->can_ifindex);
+		/* Publish the device inside the RCU section of the lookup:
+		 * When it is unlisted concurrently, either raw_notify() sees
+		 * ro->bind_dev after synchronize_net() or the lookup fails.
+		 */
+		rcu_read_lock();
+		dev = dev_get_by_index_rcu(sock_net(sk), addr->can_ifindex);
+		if (dev) {
+			dev_hold(dev);
+			/* pairs with smp_load_acquire() in raw_notify() */
+			smp_store_release(&ro->bind_dev, dev);
+		}
+		rcu_read_unlock();
 		if (!dev) {
 			err = -ENODEV;
 			goto out;
@@ -496,9 +528,6 @@ static int raw_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int len
 			goto out_put_dev;
 		}
 
-		if (!(dev->flags & IFF_UP))
-			notify_enetdown = 1;
-
 		ifindex = dev->ifindex;
 
 		/* filters set by default/setsockopt */
@@ -517,7 +546,7 @@ static int raw_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int len
 		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);
@@ -528,16 +557,24 @@ static int raw_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int len
 		ro->ifindex = ifindex;
 		ro->bound = 1;
 		/* bind() ok -> hold a reference for new ro->dev */
-		ro->dev = dev;
+		WRITE_ONCE(ro->dev, dev);
 		netdev_hold(ro->dev, &ro->dev_tracker, GFP_KERNEL);
+
+		if (dev) {
+			/* Either raw_notify() sees ro->dev on NETDEV_DOWN or
+			 * we see the cleared IFF_UP, pairs with raw_notify()
+			 */
+			smp_mb();
+			if (!(READ_ONCE(dev->flags) & IFF_UP))
+				notify_enetdown = 1;
+		}
 	}
 
 out_put_dev:
-	/* remove potential reference from dev_get_by_index() */
+	/* remove reference from the device lookup */
 	dev_put(dev);
 out:
 	release_sock(sk);
-	rtnl_unlock();
 
 	if (notify_enetdown) {
 		sk->sk_err = ENETDOWN;
@@ -566,58 +603,41 @@ static int raw_getname(struct socket *sock, struct sockaddr *uaddr,
 }
 
 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;
-		}
-
+		/* @filter has been copied by raw_setsockopt(). It is linked
+		 * to the socket on success and freed by the caller on error.
+		 *
+		 * The reg_state check is only a shortcut: On a concurrent
+		 * unregistration or namespace change 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,
@@ -631,7 +651,7 @@ static int raw_setsockopt_locked(struct socket *sock, int optname,
 		/* 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;
@@ -647,8 +667,10 @@ static int raw_setsockopt_locked(struct socket *sock, int optname,
 
 		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 */
@@ -750,18 +772,47 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
 			  sockptr_t optval, unsigned int optlen)
 {
 	struct sock *sk = sock->sk;
+	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 before lock_sock(), so a stalled copy can
+		 * not block raw_notify() waiting for lock_sock() under RTNL.
+		 */
+		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;
+		}
+	}
+
 	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();
+
+	/* on error free the filters allocated for CAN_RAW_FILTER above */
+	if (err && count > 1)
+		kfree(filter);
 
 	return err;
 }
-- 
2.53.0

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

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
can: protect the receive lists with a per-device lock

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

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

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

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

[ ... ]

> @@ -1614,7 +1612,17 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
>  		goto out;
>  	}
>  
> -	dev = dev_get_by_index(net, addr->can_ifindex);
> +	/* Publish the device inside the RCU section of the lookup: When it
> +	 * is unlisted concurrently, either isotp_notify() sees so->dev after
> +	 * synchronize_net() or the lookup fails.
> +	 */
> +	rcu_read_lock();
> +	dev = dev_get_by_index_rcu(net, addr->can_ifindex);
> +	if (dev) {
> +		dev_hold(dev);
> +		WRITE_ONCE(so->dev, dev);
> +	}
> +	rcu_read_unlock();
>  	if (!dev) {
>  		err = -ENODEV;
>  		goto out;
> @@ -1627,7 +1635,12 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
>  		err = -EINVAL;
>  		goto out_put_dev;
>  	}

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

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

so->dev is only cleared again at out_put_dev.

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

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

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

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

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

> @@ -1653,15 +1666,17 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
>  	 * can safely reach the device later, even if a concurrent
>  	 * NETDEV_UNREGISTER has already unlisted it by ifindex.
>  	 */
> -	so->dev = dev;
>  	netdev_hold(so->dev, &so->dev_tracker, GFP_KERNEL);

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

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

It then sets so->bound = 1 unconditionally.

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

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

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

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

[ ... ]

> diff --git a/net/can/raw.c b/net/can/raw.c
> index dfea48768b231..d07f3bb68cd5c 100644
> --- a/net/can/raw.c
> +++ b/net/can/raw.c

[ ... ]

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

[ ... ]

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

[ ... ]

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

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

In raw_setsockopt_locked(), CAN_RAW_ERR_FILTER still does:

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

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

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

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

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

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

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

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

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

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

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

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

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

[Oliver Hartkopp]

On 21.10.26 22:55, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> can: protect the receive lists with a per-device lock
> 
> This commit adds a spinlock to struct can_dev_rcv_lists. The receive lists
> of a CAN device are now protected per device instead of by the per-netns
> rcvlists_lock, so can_rx_unregister() can find subscriptions after the
> device has moved to another namespace.
> 
> RTNL is dropped from raw_bind(), raw_setsockopt(), raw_release(),
> isotp_bind() and isotp_release(). These paths now use lock_sock() and a
> device pointer published under RCU to serialize against the netdev
> notifier.
> 
> Link: https://lore.kernel.org/linux-can/CEA6A38A-2646-4ADA-95B4-CBAE2F301A8E@doyensec.com/
> Link: https://patch.msgid.link/20261020203257.230955-6-socketcan@hartkopp.net
> 
>> diff --git a/net/can/isotp.c b/net/can/isotp.c
>> index 5e1c3971ebfce..8ec37d252557e 100644
>> --- a/net/can/isotp.c
>> +++ b/net/can/isotp.c
> 
> [ ... ]
> 
>> @@ -1614,7 +1612,17 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
>>   		goto out;
>>   	}
>>   
>> -	dev = dev_get_by_index(net, addr->can_ifindex);
>> +	/* Publish the device inside the RCU section of the lookup: When it
>> +	 * is unlisted concurrently, either isotp_notify() sees so->dev after
>> +	 * synchronize_net() or the lookup fails.
>> +	 */
>> +	rcu_read_lock();
>> +	dev = dev_get_by_index_rcu(net, addr->can_ifindex);
>> +	if (dev) {
>> +		dev_hold(dev);
>> +		WRITE_ONCE(so->dev, dev);
>> +	}
>> +	rcu_read_unlock();
>>   	if (!dev) {
>>   		err = -ENODEV;
>>   		goto out;
>> @@ -1627,7 +1635,12 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
>>   		err = -EINVAL;
>>   		goto out_put_dev;
>>   	}
> 
> [Severity: Low]
> so->dev is now visible to the lockless check in isotp_notify() before the
> remaining checks in isotp_bind() run, and before so->bound is set. The MTU
> check can still fail the bind:
> 
> 	if (READ_ONCE(dev->mtu) < so->ll.mtu) {
> 		err = -EINVAL;
> 		goto out_put_dev;
> 	}
> 
> so->dev is only cleared again at out_put_dev.
> 
> isotp_bind() no longer holds RTNL. Can a NETDEV_DOWN for the same device
> run isotp_notifier()->isotp_notify() inside this window?
> 
> The NETDEV_DOWN case only checks READ_ONCE(so->dev) != dev. It does not
> look at so->bound or recheck under lock_sock():
> 
> 	case NETDEV_DOWN:
> 		sk->sk_err = ENETDOWN;
> 		if (!sock_flag(sk, SOCK_DEAD))
> 			sk_error_report(sk);
> 		break;
> 
> Would this set a stale ENETDOWN on a socket that was never bound, whose
> bind() then returns -EINVAL? A later successful bind() to another device
> (after fixing ll.mtu with CAN_ISOTP_LL_OPTS) would then get that error from
> the next recvmsg() or poll().
> 
> raw_bind() avoids this. It only publishes ro->bind_dev early, which
> raw_notify() uses for NETDEV_UNREGISTER only, and it sets ro->dev after
> success. Could isotp use the same approach?
> 

Yes, changing the order here to

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

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

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

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

>> @@ -1653,15 +1666,17 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
>>   	 * can safely reach the device later, even if a concurrent
>>   	 * NETDEV_UNREGISTER has already unlisted it by ifindex.
>>   	 */
>> -	so->dev = dev;
>>   	netdev_hold(so->dev, &so->dev_tracker, GFP_KERNEL);
> 
> [Severity: Low]
> This isn't a bug introduced by this patch, but isotp_bind() ignores the
> return values of both can_rx_register() calls just above this:
> 
> 	if (isotp_register_rxid(so))
> 		can_rx_register(net, dev, rx_id, SINGLE_MASK(rx_id),
> 				isotp_rcv, sk, sock_i_ino(sk), sk);
> 	...
> 	can_rx_register(net, dev, tx_id, SINGLE_MASK(tx_id),
> 			isotp_rcv_echo, sk, sock_i_ino(sk), sk);
> 
> It then sets so->bound = 1 unconditionally.
> 
> can_rx_register() returns -ENOMEM if kmem_cache_alloc(rcv_cache,
> GFP_KERNEL) fails. Can the socket then report a successful bind() while it
> is missing its FC/RX receiver or its echo receiver?
> 
> Later, isotp_release() and isotp_notify() would call can_rx_unregister()
> for an entry that was never added and hit the "receive list entry not
> found" warning. raw_enable_filters() and bcm_rx_setup() check this return
> value and unwind.
> 
> -ENODEV from the net_eq() check does not look reachable here.
> isotp_notify() waits on lock_sock() for NETDEV_UNREGISTER, so dev_net(dev)
> cannot change during the registrations. That leaves -ENOMEM as the only
> error.
> 
> The earlier commit "can: replace ARPHRD_CAN type checks with
> can_get_ml_priv()" in this series already notes that isotp_bind() ignores
> these return values.

Right.

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

> 
> [ ... ]
> 
>> diff --git a/net/can/raw.c b/net/can/raw.c
>> index dfea48768b231..d07f3bb68cd5c 100644
>> --- a/net/can/raw.c
>> +++ b/net/can/raw.c
> 
> [ ... ]
> 
>> @@ -750,18 +772,47 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
>>   			  sockptr_t optval, unsigned int optlen)
>>   {
>>   	struct sock *sk = sock->sk;
> 
> [ ... ]
> 
>> -	rtnl_lock();
>> +	if (optname == CAN_RAW_FILTER) {
>> +		/* Copy the filters before lock_sock(), so a stalled copy can
>> +		 * not block raw_notify() waiting for lock_sock() under RTNL.
>> +		 */
> 
> [ ... ]
> 
>> +	}
>> +
>>   	lock_sock(sk);
>>   
>> -	err = raw_setsockopt_locked(sock, optname, optval, optlen);
>> +	err = raw_setsockopt_locked(sock, optname, optval, optlen,
>> +				    filter, &sfilter, count);
> 
> [Severity: Medium]
> The comment says a stalled copy must not block raw_notify() while it waits
> for lock_sock() under RTNL. The early copy only covers CAN_RAW_FILTER,
> though. Do the other options still copy from user memory with lock_sock()
> held?
> 
> In raw_setsockopt_locked(), CAN_RAW_ERR_FILTER still does:
> 
> 	if (copy_from_sockptr(&err_mask, optval, optlen))
> 		return -EFAULT;
> 
> CAN_RAW_LOOPBACK, CAN_RAW_RECV_OWN_MSGS, CAN_RAW_FD_FRAMES,
> CAN_RAW_XL_FRAMES, CAN_RAW_XL_VCID_OPTS and CAN_RAW_JOIN_FILTERS do the
> same after lock_sock(sk) is taken here.
> 
> raw_getsockopt(CAN_RAW_FILTER) also writes to the user buffer with
> lock_sock() held:
> 
> 	lock_sock(sk);
> 	...
> 		if (copy_to_iter(ro->filter, len,
> 				 &opt->iter_out) != len)
> 			err = -EFAULT;
> 
> Suppose one of these copies stalls on a FUSE-backed mapping while the
> socket is bound, or while ro->bind_dev matches the device. Would a
> NETDEV_UNREGISTER of that device then leave raw_notify() sleeping in
> lock_sock() with RTNL held?
> 
> raw_notifier()
>    raw_notify()
>      lock_sock(sk)   <- waits for the stalled setsockopt/getsockopt
> 
> The unregister could come from ip link del of a vcan device in the user's
> own network namespace, a namespace move, or a USB unplug.
> 
> The new ro->bind_dev hint also adds cases where raw_notify() takes
> lock_sock(): a stale bind_dev that matches the unregistered device leads
> there too.
> 
> Before this patch, raw_setsockopt() held rtnl_lock() itself across these
> copies, so overall exposure is lower than before. The new design relies on
> lock_sock() never being held across a user copy, though. Should the
> remaining copies in raw_setsockopt_locked() and raw_getsockopt() also move
> out of the locked section?
> 
> isotp_setsockopt() has the same pattern against isotp_notify(), since every
> option is copied under lock_sock():
> 
> 	lock_sock(sk);
> 	ret = isotp_setsockopt_locked(sock, level, optname, optval, optlen);
> 	release_sock(sk);
> 
> That code predates this patch. With RTNL gone from isotp_bind(), however,
> bind relies on lock_sock() alone to serialize with isotp_notify().

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

Best regards,
Oliver

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

[Marc Kleine-Budde]

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

On 21.10.2026 23:59:17, Oliver Hartkopp wrote:
[...]>
> is a good hint from sashiko-bot and follows the raw_bind() concept.
> @Jakub: I've sent a V6 only for this single patch 11/11 here:
>
> Is this ok for you? I think Marc is offline and can not re-spin the PR right
> now.
>
> https://lore.kernel.org/netdev/20261021215456.119268-1-socketcan@hartkopp.net/

Give me a sec, I'm preparing the new PR.

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 --]

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

[Oliver Hartkopp]

On 22.10.26 01:05, Marc Kleine-Budde wrote:
> On 21.10.2026 23:59:17, Oliver Hartkopp wrote:
> [...]>
>> is a good hint from sashiko-bot and follows the raw_bind() concept.
>> @Jakub: I've sent a V6 only for this single patch 11/11 here:
>>
>> Is this ok for you? I think Marc is offline and can not re-spin the PR right
>> now.
>>
>> https://lore.kernel.org/netdev/20261021215456.119268-1-socketcan@hartkopp.net/
> 
> Give me a sec, I'm preparing the new PR.
> 
> Marc
> 
Thanks Marc!

PR can-next 2026-10-22 is here:

https://lore.kernel.org/netdev/20261021231130.2085690-1-mkl@pengutronix.de/

Many thanks,
Oliver

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

[Marc Kleine-Budde]

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

On 21.10.2026 23:59:17, Oliver Hartkopp wrote:
> Yes, changing the order here to
>
>         rcu_read_lock();
>         dev = dev_get_by_index_rcu(net, addr->can_ifindex);
>         if (!dev || !can_get_ml_priv(dev))
>                 err = -ENODEV;
>         else if (READ_ONCE(dev->mtu) < so->ll.mtu)
>                 err = -EINVAL;
>         if (!err) {
>                 dev_hold(dev);
>                 WRITE_ONCE(so->dev, dev);
>         }
>         rcu_read_unlock();
>         if (err)
>                 goto out;
>
> is a good hint from sashiko-bot and follows the raw_bind() concept.
> @Jakub: I've sent a V6 only for this single patch 11/11 here:
>
> Is this ok for you? I think Marc is offline and can not re-spin the PR right
> now.
>
> https://lore.kernel.org/netdev/20261021215456.119268-1-socketcan@hartkopp.net/

pw-bot: cr

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 --]

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

[sashiko-bot]

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

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

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

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

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

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

[ ... ]

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

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

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

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

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

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

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

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

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

[ ... ]

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

[ ... ]

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

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

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

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

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

	release_sock(sk);

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

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

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

[ ... ]

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

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

[netdev-bot+sashiko]

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

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

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

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

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

--

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

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

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

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

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

[ ... ]

> @@ -1614,7 +1612,17 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
>  		goto out;
>  	}
>  
> -	dev = dev_get_by_index(net, addr->can_ifindex);
> +	/* Publish the device inside the RCU section of the lookup: When it
> +	 * is unlisted concurrently, either isotp_notify() sees so->dev after
> +	 * synchronize_net() or the lookup fails.
> +	 */
> +	rcu_read_lock();
> +	dev = dev_get_by_index_rcu(net, addr->can_ifindex);
> +	if (dev) {
> +		dev_hold(dev);
> +		WRITE_ONCE(so->dev, dev);
> +	}
> +	rcu_read_unlock();
>  	if (!dev) {
>  		err = -ENODEV;
>  		goto out;
> @@ -1627,7 +1635,12 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
>  		err = -EINVAL;
>  		goto out_put_dev;
>  	}

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

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

so->dev is only cleared again at out_put_dev.

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

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

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

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

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

> @@ -1653,15 +1666,17 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
>  	 * can safely reach the device later, even if a concurrent
>  	 * NETDEV_UNREGISTER has already unlisted it by ifindex.
>  	 */
> -	so->dev = dev;
>  	netdev_hold(so->dev, &so->dev_tracker, GFP_KERNEL);

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

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

It then sets so->bound = 1 unconditionally.

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

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

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

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

[ ... ]

> diff --git a/net/can/raw.c b/net/can/raw.c
> index dfea48768b231..d07f3bb68cd5c 100644
> --- a/net/can/raw.c
> +++ b/net/can/raw.c

[ ... ]

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

[ ... ]

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

[ ... ]

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

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

In raw_setsockopt_locked(), CAN_RAW_ERR_FILTER still does:

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

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

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

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

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

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

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

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

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

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

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

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

[ ... ]

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

[Severity: Medium]
With RTNL gone from raw_bind(), can a concurrent NETDEV_UNREGISTER now
leave the socket reporting ENETDOWN instead of ENODEV?

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

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

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

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

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

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

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

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

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

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

Re: [PATCH net-next 0/11] pull-request: can-next 2026-10-21

[Paolo Abeni]

On 10/21/26 12:24, Marc Kleine-Budde wrote:
> this is a pull request of 11 patches for net-next/main and obsoletes
> pull-request: can-next 2026-10-19.

This is a bit tricky. We are already into the merge window, we should
defer all 'next' material posted after the merge window start.

/P

Re: [PATCH net-next 0/11] pull-request: can-next 2026-10-21

[Oliver Hartkopp]

Hi Paolo,

the original PR is pull-request: can-next 2026-10-18

https://lore.kernel.org/linux-can/20261018164417.1540449-1-mkl@pengutronix.de/

and I had to fix some sashiko-bots reports that didn't show up in 
previous postings on the CAN mailing list and its LMM.

So its really just a fixes version of can-next 2026-10-18.

Sorry for the re-sending :-/
My faults.

Best regards,
Oliver

On 21.10.26 13:19, Paolo Abeni wrote:
> On 10/21/26 12:24, Marc Kleine-Budde wrote:
>> this is a pull request of 11 patches for net-next/main and obsoletes
>> pull-request: can-next 2026-10-19.
> 
> This is a bit tricky. We are already into the merge window, we should
> defer all 'next' material posted after the merge window start.
> 
> /P
> 
> 

Re: [PATCH net-next 0/11] pull-request: can-next 2026-10-21

[Jakub Kicinski]

On Wed, 21 Oct 2026 13:33:38 +0200 Oliver Hartkopp wrote:
> the original PR is pull-request: can-next 2026-10-18
> 
> https://lore.kernel.org/linux-can/20261018164417.1540449-1-mkl@pengutronix.de/
> 
> and I had to fix some sashiko-bots reports that didn't show up in 
> previous postings on the CAN mailing list and its LMM.
> 
> So its really just a fixes version of can-next 2026-10-18.

Right, looks like fixes mostly. Could you please quickly look over 
the feedback on patch 11?

Re: [PATCH net-next 0/11] pull-request: can-next 2026-10-21

[Oliver Hartkopp]

On 21.10.26 22:56, Jakub Kicinski wrote:
> On Wed, 21 Oct 2026 13:33:38 +0200 Oliver Hartkopp wrote:
>> the original PR is pull-request: can-next 2026-10-18
>>
>> https://lore.kernel.org/linux-can/20261018164417.1540449-1-mkl@pengutronix.de/
>>
>> and I had to fix some sashiko-bots reports that didn't show up in
>> previous postings on the CAN mailing list and its LMM.
>>
>> So its really just a fixes version of can-next 2026-10-18.
> 
> Right, looks like fixes mostly. Could you please quickly look over
> the feedback on patch 11?
> 

Yes. I'm working on it.

Thanks!

[PATCH net-next 0/11] pull-request: can-next 2026-10-22

[Marc Kleine-Budde]

Hello netdev-team,

this is a pull request of 11 patches for net-next/main and obsoletes
pull-request: linux-can-next-for-7.4-20261021.

The first patch is by Vincent Mailhol and drops CAN-XL frames on non
CAN-XL devices.

A patch by Sang-Heon Jeon removes a redundant NULL check before
netdev_hold() in the CAN RAW protocol.

A patch by Kaixuan Li for the CAN ISOTP protocol improves the check
for the correct CAN CC/FD frames.

The next 3 patches are by Cunhao Lu and make CAN skb freeing safe in
any context in several CAN helper functions.

A series by Oliver Hartkopp et al fixes several problems in the CAN
net-layer:

3 patches target the CAN procfs: A patch by Runyu Xiao resets the CAN
pkg_stats using atomics, a patch by Oliver Hartkopp fixes the stats
calculation after a statistics reset. Followed by a patch to remove
kernel pointers from the output.

The last 2 patches are by Oliver Hartkopp as well, they convert the
unreliable ARPHRD_CAN type check to the robust can_get_ml_priv() and
protect the CAN receive lists with a per-device lock.

regards,
Marc

---

The following changes since commit 7dca8514c51ea5219daf7f28bc1a0536b907e611:

  Merge branch 'vsock-so_inq-for-stream-sockets' (2026-10-21 11:58:24 +0200)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git tags/linux-can-next-for-7.4-20261022

for you to fetch changes up to 80cea9d8546f7533e9161c7f5b156ecd066647be:

  Merge patch series "can: protect the receive lists with a per-device lock" (2026-10-22 01:04:07 +0200)

----------------------------------------------------------------
linux-can-next-for-7.4-20261022

----------------------------------------------------------------
Cunhao Lu (3):
      can: skb: make echo skb freeing safe in any IRQ context
      can: skb: make CAN skb allocation failure paths IRQ-safe
      can: dev: can_put_echo_skb(): free skb on invalid echo index

Kaixuan Li (1):
      can: isotp: check the frame type, not just the length

Marc Kleine-Budde (2):
      Merge patch series "can: skb: make echo skb freeing safe in any IRQ context"
      Merge patch series "can: protect the receive lists with a per-device lock"

Oliver Hartkopp (4):
      can: proc: skip stats calculation after reset in can_stat_update()
      can: proc: remove pointers from CAN specific proc output
      can: replace ARPHRD_CAN type checks with can_get_ml_priv()
      can: protect the receive lists with a per-device lock

Runyu Xiao (1):
      can: proc: reset pkg_stats atomics individually

Sang-Heon Jeon (1):
      can: raw: remove redundant NULL check before netdev_hold()

Vincent Mailhol (1):
      can: dev: can_dropped_invalid_skb: drop CAN XL frames on non-CAN XL devices

 Documentation/networking/can.rst |  16 ++--
 drivers/net/can/dev/skb.c        |  16 ++--
 include/linux/can/can-ml.h       |   3 +
 include/linux/can/core.h         |   2 +-
 include/linux/can/dev.h          |   2 +-
 include/linux/can/skb.h          |   4 +-
 include/net/netns/can.h          |   2 +-
 net/can/af_can.c                 |  62 ++++++++-------
 net/can/af_can.h                 |   2 +-
 net/can/bcm.c                    |  64 ++++++---------
 net/can/gw.c                     |   9 ++-
 net/can/isotp.c                  |  73 +++++++++++------
 net/can/j1939/main.c             |   2 +-
 net/can/proc.c                   |  95 +++++++++++++---------
 net/can/raw.c                    | 166 +++++++++++++++++++++++++--------------
 15 files changed, 305 insertions(+), 213 deletions(-)

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

[Marc Kleine-Budde]

From: Oliver Hartkopp <socketcan@hartkopp.net>

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() leave their notifier list before they
take any lock that serializes against the netdev notifier, which runs
under RTNL. When the bound device is moved to another namespace in this
window, the notifier skips the socket and the release path has to remove
the filters itself. isotp_release() passes the socket's namespace, which
does not match dev_net(dev) anymore. bcm_rx_unreg() passes dev_net(dev),
which can_rx_unregister() reads again for its check, so a namespace
change between both reads makes the check fail as well. In both cases
can_rx_unregister() silently returns. As can_rx_register() stores
rcv->sk without a reference, the remaining receivers point to the freed
socket (isotp) or the freed op (bcm):

  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

Besides this, the lists of a device are protected by the rcvlists_lock
of the namespace that is passed in. Callers which pass dev_net(dev) read
it at different times, so the same lists may be modified under the locks
of different namespaces while the device is moved.

Taking RTNL in these release paths is no option: raw_bind(),
raw_setsockopt() and raw_release() already hold RTNL while adding and
removing up to CAN_RAW_FILTER_MAX filters per socket, which unprivileged
users can trigger repeatedly and which makes RTNL a global bottleneck.

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. It is initialized in
can_set_ml_priv(), which is used by all CAN network devices. With this,
can_rx_unregister() finds the subscription independently of the
current namespace of the device. The protocols keep the device valid
with their tracked references. The per-netns rcvlists_lock only protects
the receive list statistics anymore, and CAN_RAW, CAN_BCM and CAN_ISOTP
pass the namespace that has been used for can_rx_register().

So isotp_release() and bcm_release() now remove their filters correctly
without RTNL, and raw_release(), raw_setsockopt(), raw_bind() and
isotp_bind() do not need RTNL anymore and add and remove their filters
under lock_sock(). To serialize bind() with NETDEV_UNREGISTER, bind()
publishes the device inside the RCU section of its lookup, and
raw_notify() and isotp_notify() recheck the bound device under lock_sock().
As RTNL does not keep NETDEV_DOWN out of bind() anymore, bind() checks
IFF_UP after publishing the bound device, with a barrier pair against
raw_notify() and isotp_notify(), so a down transition during bind() is
still reported with ENETDOWN.

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-5-5
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
Link: https://patch.msgid.link/20261021215456.119268-1-socketcan@hartkopp.net
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 include/linux/can/can-ml.h |   3 +
 include/net/netns/can.h    |   2 +-
 net/can/af_can.c           |  41 ++++++----
 net/can/bcm.c              |  43 +++-------
 net/can/isotp.c            |  58 ++++++++------
 net/can/raw.c              | 155 ++++++++++++++++++++++++-------------
 6 files changed, 181 insertions(+), 121 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
@@ -45,6 +45,7 @@
 #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 */
@@ -59,6 +60,7 @@
 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];
@@ -81,6 +83,7 @@ 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);
 }
 
diff --git a/include/net/netns/can.h b/include/net/netns/can.h
index 48b79f7e6236..54f23681e0d4 100644
--- a/include/net/netns/can.h
+++ b/include/net/netns/can.h
@@ -29,7 +29,7 @@ struct netns_can {
 
 	/* receive filters subscribed for 'all' CAN devices */
 	struct can_dev_rcv_lists *rx_alldev_list;
-	spinlock_t rcvlists_lock;
+	spinlock_t rcvlists_lock; /* protects the rcv_entries(_max) stats */
 	struct timer_list stattimer; /* timer for statistics update */
 	struct can_pkg_stats *pkg_stats;
 	struct can_rcv_lists_stats *rcv_lists_stats;
diff --git a/net/can/af_can.c b/net/can/af_can.c
index d97f85fc3232..0f3051a61aab 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -462,9 +462,10 @@ int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
 	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;
@@ -478,6 +479,9 @@ int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
 	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);
@@ -509,6 +513,14 @@ static void can_rx_delete_receiver(struct rcu_head *rp)
  *
  * 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 should be the namespace that has been used for
+ *  can_rx_register(). It selects the 'any' CAN device lists for
+ *  @dev == NULL and is otherwise only used for the receive list
+ *  statistics, which become imprecise when another namespace is passed.
  */
 void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
 		       canid_t mask, void (*func)(struct sk_buff *, void *),
@@ -522,12 +534,10 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
 	if (dev && !can_get_ml_priv(dev))
 		return;
 
-	if (dev && !net_eq(net, dev_net(dev)))
-		return;
-
-	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);
 
 	/* Search the receiver list for the item to delete.  This should
@@ -547,26 +557,26 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
 	 * 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);
 
@@ -823,6 +833,7 @@ static int can_pernet_init(struct net *net)
 	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;
diff --git a/net/can/bcm.c b/net/can/bcm.c
index 6759662e024c..7bd2aa39777a 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -962,7 +962,7 @@ 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 */
@@ -988,21 +988,10 @@ static int bcm_delete_rx_op(struct list_head *ops, struct bcm_msg_head *mh,
 			/* 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 in bcm_notify(). op->rx_reg_dev keeps
+				 * the device's receive lists accessible.
 				 */
 				if (op->rx_reg_dev)
 					bcm_rx_unreg(op->rx_reg_dev, op);
@@ -1935,6 +1924,7 @@ static int bcm_release(struct socket *sock)
 	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)
@@ -1948,29 +1938,20 @@ static int bcm_release(struct socket *sock)
 		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
+			 * in bcm_notify(). op->rx_reg_dev keeps the device's
+			 * receive lists accessible.
 			 */
 			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();
diff --git a/net/can/isotp.c b/net/can/isotp.c
index 5e1c3971ebfc..9386389cd702 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -1497,11 +1497,11 @@ static int isotp_release(struct socket *sock)
 	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
+	 * the tracked reference so->dev keeps the device's receive lists
+	 * accessible, even after a namespace change
 	 */
 	if (so->bound && so->dev) {
 		if (isotp_register_rxid(so))
@@ -1519,8 +1519,6 @@ static int isotp_release(struct socket *sock)
 	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
@@ -1588,7 +1586,7 @@ 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) {
@@ -1614,20 +1612,30 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
 		goto out;
 	}
 
-	dev = dev_get_by_index(net, addr->can_ifindex);
-	if (!dev) {
+	/* Publish the device inside the RCU section of the lookup: When it
+	 * is unlisted concurrently, either isotp_notify() sees so->dev after
+	 * synchronize_net() or the lookup fails. Check the device before, so
+	 * that bind() can not fail after so->dev has been published.
+	 */
+	rcu_read_lock();
+	dev = dev_get_by_index_rcu(net, addr->can_ifindex);
+	if (!dev || !can_get_ml_priv(dev))
 		err = -ENODEV;
-		goto out;
-	}
-	if (!can_get_ml_priv(dev)) {
-		err = -ENODEV;
-		goto out_put_dev;
-	}
-	if (READ_ONCE(dev->mtu) < so->ll.mtu) {
+	else if (READ_ONCE(dev->mtu) < so->ll.mtu)
 		err = -EINVAL;
-		goto out_put_dev;
+	if (!err) {
+		dev_hold(dev);
+		WRITE_ONCE(so->dev, dev);
 	}
-	if (!(dev->flags & IFF_UP))
+	rcu_read_unlock();
+	if (err)
+		goto out;
+
+	/* Either isotp_notify() sees so->dev on NETDEV_DOWN or we see the
+	 * cleared IFF_UP, pairs with isotp_notify()
+	 */
+	smp_mb();
+	if (!(READ_ONCE(dev->flags) & IFF_UP))
 		notify_enetdown = 1;
 
 	ifindex = dev->ifindex;
@@ -1653,15 +1661,12 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
 	 * can safely reach the device later, even if a concurrent
 	 * NETDEV_UNREGISTER has already unlisted it by ifindex.
 	 */
-	so->dev = dev;
 	netdev_hold(so->dev, &so->dev_tracker, GFP_KERNEL);
 
-out_put_dev:
-	/* remove potential reference from dev_get_by_index() */
+	/* remove reference from the device lookup */
 	dev_put(dev);
 out:
 	release_sock(sk);
-	rtnl_unlock();
 
 	if (notify_enetdown) {
 		sk->sk_err = ENETDOWN;
@@ -1864,12 +1869,21 @@ static void isotp_notify(struct isotp_sock *so, unsigned long msg,
 	if (!net_eq(dev_net(dev), sock_net(sk)))
 		return;
 
-	if (so->dev != dev)
+	if (msg == NETDEV_DOWN)
+		smp_mb(); /* pairs with isotp_bind(), see raw_notify() */
+
+	/* lockless fast path, see raw_notify() */
+	if (READ_ONCE(so->dev) != dev)
 		return;
 
 	switch (msg) {
 	case NETDEV_UNREGISTER:
 		lock_sock(sk);
+		if (so->dev != dev) {
+			release_sock(sk);
+			break;
+		}
+
 		/* remove current filters & unregister */
 		if (so->bound) {
 			if (isotp_register_rxid(so))
@@ -1885,7 +1899,7 @@ static void isotp_notify(struct isotp_sock *so, unsigned long msg,
 
 		so->ifindex = 0;
 		so->bound  = 0;
-		so->dev = NULL;
+		WRITE_ONCE(so->dev, NULL);
 		release_sock(sk);
 
 		sk->sk_err = ENODEV;
diff --git a/net/can/raw.c b/net/can/raw.c
index dfea48768b23..d07f3bb68cd5 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -85,6 +85,7 @@ struct raw_sock {
 	struct sock sk;
 	struct net_device *dev;
 	netdevice_tracker dev_tracker;
+	struct net_device *bind_dev; /* hint for raw_notify(), no reference */
 	struct list_head notifier;
 	int ifindex;
 	unsigned int bound:1;
@@ -224,6 +225,7 @@ static int raw_enable_filters(struct net *net, struct net_device *dev,
 	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);
@@ -258,9 +260,11 @@ static void raw_disable_filters(struct net *net, struct net_device *dev,
 {
 	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,
@@ -305,16 +309,31 @@ static void raw_notify(struct raw_sock *ro, unsigned long msg,
 		       struct net_device *dev)
 {
 	struct sock *sk = &ro->sk;
+	struct net_device *bind_dev;
 
 	if (!net_eq(dev_net(dev), sock_net(sk)))
 		return;
 
-	if (ro->dev != dev)
+	if (msg == NETDEV_DOWN)
+		smp_mb(); /* IFF_UP is cleared before, pairs with raw_bind() */
+
+	/* Lockless fast path, ro->bind_dev covers a concurrent bind().
+	 * Read it before ro->dev, pairs with smp_store_release() in
+	 * raw_bind(). A stale ro->bind_dev only costs a lock_sock().
+	 */
+	bind_dev = smp_load_acquire(&ro->bind_dev);
+	if (READ_ONCE(ro->dev) != dev &&
+	    (msg != NETDEV_UNREGISTER || bind_dev != dev))
 		return;
 
 	switch (msg) {
 	case NETDEV_UNREGISTER:
 		lock_sock(sk);
+		if (ro->dev != dev) {
+			release_sock(sk);
+			break;
+		}
+
 		/* remove current filters & unregister */
 		if (ro->bound) {
 			raw_disable_allfilters(dev_net(dev), dev, sk);
@@ -326,7 +345,7 @@ static void raw_notify(struct raw_sock *ro, unsigned long msg,
 
 		ro->ifindex = 0;
 		ro->bound = 0;
-		ro->dev = NULL;
+		WRITE_ONCE(ro->dev, NULL);
 		ro->count = 0;
 		release_sock(sk);
 
@@ -381,6 +400,7 @@ static int raw_init(struct sock *sk)
 	ro->bound            = 0;
 	ro->ifindex          = 0;
 	ro->dev              = NULL;
+	ro->bind_dev         = NULL;
 
 	/* set default filter to single entry dfilter */
 	ro->dfilter.can_id   = 0;
@@ -431,13 +451,15 @@ static int raw_release(struct socket *sock)
 	list_del(&ro->notifier);
 	spin_unlock(&raw_notifier_lock);
 
-	rtnl_lock();
 	lock_sock(sk);
 
-	/* remove current filters & unregister */
+	/* remove current filters & unregister
+	 * the tracked reference ro->dev keeps the device's receive lists
+	 * accessible, even after a namespace change
+	 */
 	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);
@@ -456,7 +478,6 @@ static int raw_release(struct socket *sock)
 	sock->sk = NULL;
 
 	release_sock(sk);
-	rtnl_unlock();
 
 	sock_prot_inuse_add(net, sk->sk_prot, -1);
 	sock_put(sk);
@@ -479,14 +500,25 @@ static int raw_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int len
 	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;
 
 	if (addr->can_ifindex) {
-		dev = dev_get_by_index(sock_net(sk), addr->can_ifindex);
+		/* Publish the device inside the RCU section of the lookup:
+		 * When it is unlisted concurrently, either raw_notify() sees
+		 * ro->bind_dev after synchronize_net() or the lookup fails.
+		 */
+		rcu_read_lock();
+		dev = dev_get_by_index_rcu(sock_net(sk), addr->can_ifindex);
+		if (dev) {
+			dev_hold(dev);
+			/* pairs with smp_load_acquire() in raw_notify() */
+			smp_store_release(&ro->bind_dev, dev);
+		}
+		rcu_read_unlock();
 		if (!dev) {
 			err = -ENODEV;
 			goto out;
@@ -496,9 +528,6 @@ static int raw_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int len
 			goto out_put_dev;
 		}
 
-		if (!(dev->flags & IFF_UP))
-			notify_enetdown = 1;
-
 		ifindex = dev->ifindex;
 
 		/* filters set by default/setsockopt */
@@ -517,7 +546,7 @@ static int raw_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int len
 		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);
@@ -528,16 +557,24 @@ static int raw_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int len
 		ro->ifindex = ifindex;
 		ro->bound = 1;
 		/* bind() ok -> hold a reference for new ro->dev */
-		ro->dev = dev;
+		WRITE_ONCE(ro->dev, dev);
 		netdev_hold(ro->dev, &ro->dev_tracker, GFP_KERNEL);
+
+		if (dev) {
+			/* Either raw_notify() sees ro->dev on NETDEV_DOWN or
+			 * we see the cleared IFF_UP, pairs with raw_notify()
+			 */
+			smp_mb();
+			if (!(READ_ONCE(dev->flags) & IFF_UP))
+				notify_enetdown = 1;
+		}
 	}
 
 out_put_dev:
-	/* remove potential reference from dev_get_by_index() */
+	/* remove reference from the device lookup */
 	dev_put(dev);
 out:
 	release_sock(sk);
-	rtnl_unlock();
 
 	if (notify_enetdown) {
 		sk->sk_err = ENETDOWN;
@@ -566,58 +603,41 @@ static int raw_getname(struct socket *sock, struct sockaddr *uaddr,
 }
 
 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;
-		}
-
+		/* @filter has been copied by raw_setsockopt(). It is linked
+		 * to the socket on success and freed by the caller on error.
+		 *
+		 * The reg_state check is only a shortcut: On a concurrent
+		 * unregistration or namespace change 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,
@@ -631,7 +651,7 @@ static int raw_setsockopt_locked(struct socket *sock, int optname,
 		/* 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;
@@ -647,8 +667,10 @@ static int raw_setsockopt_locked(struct socket *sock, int optname,
 
 		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 */
@@ -750,18 +772,47 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
 			  sockptr_t optval, unsigned int optlen)
 {
 	struct sock *sk = sock->sk;
+	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 before lock_sock(), so a stalled copy can
+		 * not block raw_notify() waiting for lock_sock() under RTNL.
+		 */
+		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;
+		}
+	}
+
 	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();
+
+	/* on error free the filters allocated for CAN_RAW_FILTER above */
+	if (err && count > 1)
+		kfree(filter);
 
 	return err;
 }
-- 
2.53.0

[PATCH net-next 0/11] pull-request: can-next 2026-10-19

[Marc Kleine-Budde]

Hello netdev-team,

this is a pull request of 11 patches for net-next/main and obsoletes
pull-request: can-next 2026-10-18.

The first patch is by Vincent Mailhol and drops CAN-XL frames on non
CAN-XL devices.

A patch by Sang-Heon Jeon removes a redundant NULL check before
netdev_hold() in the CAN RAW protocol.

A patch by Kaixuan Li for the CAN ISOTP protocol improves the check
for the correct CAN CC/FD frames.

The next 3 patches are by Cunhao Lu and make CAN skb freeing safe in
any context in several CAN helper functions.

A series by Oliver Hartkopp et al fixes several problems in the CAN
net-layer:

3 patches target the CAN procfs: A patch by Runyu Xiao resets the CAN
pkg_stats using atomics, a patch by Oliver Hartkopp fixes the stats
calculation after a statistics reset. Followed by a patch to remove
kernel pointers from the output.

The last s patches are by Oliver Hartkopp as well, they convert the
unreliable ARPHRD_CAN type check to the robust can_get_ml_priv() and
protect the CAN receive lists with a per-device lock.

regards,
Marc

---

The following changes since commit 98c2ad4f5d805dbc1ea436a334ee59311806a48c:

  Merge branch 'ipv4-remove-netns-argument-from-fib_table_lookup' (2026-10-16 18:52:36 -0700)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git tags/linux-can-next-for-7.4-20261019

for you to fetch changes up to 94f33aab0beb5c7eba56be9fab2032b82fa78b86:

  Merge patch series "can: protect the receive lists with a per-device lock" (2026-10-19 18:46:56 +0200)

----------------------------------------------------------------
linux-can-next-for-7.4-20261019

----------------------------------------------------------------
Cunhao Lu (3):
      can: skb: make echo skb freeing safe in any IRQ context
      can: skb: make CAN skb allocation failure paths IRQ-safe
      can: dev: can_put_echo_skb(): free skb on invalid echo index

Kaixuan Li (1):
      can: isotp: check the frame type, not just the length

Marc Kleine-Budde (2):
      Merge patch series "can: skb: make echo skb freeing safe in any IRQ context"
      Merge patch series "can: protect the receive lists with a per-device lock"

Oliver Hartkopp (4):
      can: proc: skip stats calculation after reset in can_stat_update()
      can: proc: remove pointers from CAN specific proc output
      can: replace ARPHRD_CAN type checks with can_get_ml_priv()
      can: protect the receive lists with a per-device lock

Runyu Xiao (1):
      can: proc: reset pkg_stats atomics individually

Sang-Heon Jeon (1):
      can: raw: remove redundant NULL check before netdev_hold()

Vincent Mailhol (1):
      can: dev: can_dropped_invalid_skb: drop CAN XL frames on non-CAN XL devices

 Documentation/networking/can.rst |  16 ++--
 drivers/net/can/dev/skb.c        |  16 ++--
 include/linux/can/can-ml.h       |   3 +
 include/linux/can/core.h         |   2 +-
 include/linux/can/dev.h          |   2 +-
 include/linux/can/skb.h          |   4 +-
 include/net/netns/can.h          |   2 +-
 net/can/af_can.c                 |  61 +++++++++-------
 net/can/af_can.h                 |   2 +-
 net/can/bcm.c                    |  64 ++++++----------
 net/can/gw.c                     |   9 ++-
 net/can/isotp.c                  |  57 +++++++++++----
 net/can/j1939/main.c             |   2 +-
 net/can/proc.c                   |  95 ++++++++++++++----------
 net/can/raw.c                    | 153 +++++++++++++++++++++++++--------------
 15 files changed, 286 insertions(+), 202 deletions(-)

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

[Marc Kleine-Budde]

From: Oliver Hartkopp <socketcan@hartkopp.net>

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() leave their notifier list before they
take any lock that serializes against the netdev notifier, which runs
under RTNL. When the bound device is moved to another namespace in this
window, the notifier skips the socket and the release path has to remove
the filters itself. isotp_release() passes the socket's namespace, which
does not match dev_net(dev) anymore. bcm_rx_unreg() passes dev_net(dev),
which can_rx_unregister() reads again for its check, so a namespace
change between both reads makes the check fail as well. In both cases
can_rx_unregister() silently returns. As can_rx_register() stores
rcv->sk without a reference, the remaining receivers point to the freed
socket (isotp) or the freed op (bcm):

  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

Besides this, the lists of a device are protected by the rcvlists_lock
of the namespace that is passed in. Callers which pass dev_net(dev) read
it at different times, so the same lists may be modified under the locks
of different namespaces while the device is moved.

Taking RTNL in these release paths is no option: raw_bind(),
raw_setsockopt() and raw_release() already hold RTNL while adding and
removing up to CAN_RAW_FILTER_MAX filters per socket, which unprivileged
users can trigger repeatedly and which makes RTNL a global bottleneck.

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. It is initialized in
can_set_ml_priv(), which is used by all CAN network devices. With this,
can_rx_unregister() finds the subscription independently of the
current namespace of the device. The protocols keep the device valid
with their tracked references. The per-netns rcvlists_lock only protects
the receive list statistics anymore, and the callers pass the namespace
that has been used for can_rx_register().

So isotp_release() and bcm_release() now remove their filters correctly
without RTNL, and raw_release(), raw_setsockopt(), raw_bind() and
isotp_bind() do not need RTNL anymore and add and remove their filters
under lock_sock(). To serialize bind() with NETDEV_UNREGISTER, bind()
publishes the device inside the RCU section of its lookup, and
raw_notify() and isotp_notify() recheck the bound device under lock_sock().

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-5-5
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
Link: https://patch.msgid.link/20261019152741.54854-6-socketcan@hartkopp.net
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 include/linux/can/can-ml.h |   3 +
 include/net/netns/can.h    |   2 +-
 net/can/af_can.c           |  40 +++++++----
 net/can/bcm.c              |  43 ++++-------
 net/can/isotp.c            |  40 +++++++----
 net/can/raw.c              | 142 ++++++++++++++++++++++++-------------
 6 files changed, 161 insertions(+), 109 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
@@ -45,6 +45,7 @@
 #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 */
@@ -59,6 +60,7 @@
 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];
@@ -81,6 +83,7 @@ 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);
 }
 
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
@@ -29,7 +29,7 @@ struct netns_can {
 
 	/* 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;
diff --git a/net/can/af_can.c b/net/can/af_can.c
index d97f85fc3232..bf612c4b983d 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -462,9 +462,10 @@ int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
 	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;
@@ -478,6 +479,9 @@ int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
 	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);
@@ -509,6 +513,13 @@ static void can_rx_delete_receiver(struct rcu_head *rp)
  *
  * 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().
+ *  It selects the 'any' CAN device lists for @dev == NULL and is 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 *),
@@ -522,12 +533,10 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
 	if (dev && !can_get_ml_priv(dev))
 		return;
 
-	if (dev && !net_eq(net, dev_net(dev)))
-		return;
-
-	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);
 
 	/* Search the receiver list for the item to delete.  This should
@@ -547,26 +556,26 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
 	 * 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);
 
@@ -823,6 +832,7 @@ static int can_pernet_init(struct net *net)
 	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;
diff --git a/net/can/bcm.c b/net/can/bcm.c
index 6759662e024c..7bd2aa39777a 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -962,7 +962,7 @@ 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 */
@@ -988,21 +988,10 @@ static int bcm_delete_rx_op(struct list_head *ops, struct bcm_msg_head *mh,
 			/* 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 in bcm_notify(). op->rx_reg_dev keeps
+				 * the device's receive lists accessible.
 				 */
 				if (op->rx_reg_dev)
 					bcm_rx_unreg(op->rx_reg_dev, op);
@@ -1935,6 +1924,7 @@ static int bcm_release(struct socket *sock)
 	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)
@@ -1948,29 +1938,20 @@ static int bcm_release(struct socket *sock)
 		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
+			 * in bcm_notify(). op->rx_reg_dev keeps the device's
+			 * receive lists accessible.
 			 */
 			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();
diff --git a/net/can/isotp.c b/net/can/isotp.c
index 5e1c3971ebfc..5ae55ed28d57 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -1497,11 +1497,11 @@ static int isotp_release(struct socket *sock)
 	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
+	 * the tracked reference so->dev keeps the device's receive lists
+	 * accessible, even after a namespace change
 	 */
 	if (so->bound && so->dev) {
 		if (isotp_register_rxid(so))
@@ -1519,8 +1519,6 @@ static int isotp_release(struct socket *sock)
 	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
@@ -1588,7 +1586,7 @@ 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) {
@@ -1614,7 +1612,17 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
 		goto out;
 	}
 
-	dev = dev_get_by_index(net, addr->can_ifindex);
+	/* Publish the device inside the RCU section of the lookup: When it
+	 * is unlisted concurrently, either isotp_notify() sees so->dev after
+	 * synchronize_net() or the lookup fails.
+	 */
+	rcu_read_lock();
+	dev = dev_get_by_index_rcu(net, addr->can_ifindex);
+	if (dev) {
+		dev_hold(dev);
+		WRITE_ONCE(so->dev, dev);
+	}
+	rcu_read_unlock();
 	if (!dev) {
 		err = -ENODEV;
 		goto out;
@@ -1627,7 +1635,7 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
 		err = -EINVAL;
 		goto out_put_dev;
 	}
-	if (!(dev->flags & IFF_UP))
+	if (!(READ_ONCE(dev->flags) & IFF_UP))
 		notify_enetdown = 1;
 
 	ifindex = dev->ifindex;
@@ -1653,15 +1661,17 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
 	 * can safely reach the device later, even if a concurrent
 	 * NETDEV_UNREGISTER has already unlisted it by ifindex.
 	 */
-	so->dev = dev;
 	netdev_hold(so->dev, &so->dev_tracker, GFP_KERNEL);
 
 out_put_dev:
-	/* remove potential reference from dev_get_by_index() */
+	/* the published so->dev is only valid for a successful bind() */
+	if (err)
+		WRITE_ONCE(so->dev, NULL);
+
+	/* remove reference from the device lookup */
 	dev_put(dev);
 out:
 	release_sock(sk);
-	rtnl_unlock();
 
 	if (notify_enetdown) {
 		sk->sk_err = ENETDOWN;
@@ -1864,12 +1874,18 @@ static void isotp_notify(struct isotp_sock *so, unsigned long msg,
 	if (!net_eq(dev_net(dev), sock_net(sk)))
 		return;
 
-	if (so->dev != dev)
+	/* lockless fast path, see raw_notify() */
+	if (READ_ONCE(so->dev) != dev)
 		return;
 
 	switch (msg) {
 	case NETDEV_UNREGISTER:
 		lock_sock(sk);
+		if (so->dev != dev) {
+			release_sock(sk);
+			break;
+		}
+
 		/* remove current filters & unregister */
 		if (so->bound) {
 			if (isotp_register_rxid(so))
@@ -1885,7 +1901,7 @@ static void isotp_notify(struct isotp_sock *so, unsigned long msg,
 
 		so->ifindex = 0;
 		so->bound  = 0;
-		so->dev = NULL;
+		WRITE_ONCE(so->dev, NULL);
 		release_sock(sk);
 
 		sk->sk_err = ENODEV;
diff --git a/net/can/raw.c b/net/can/raw.c
index dfea48768b23..0d8edfa772fb 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -85,6 +85,7 @@ struct raw_sock {
 	struct sock sk;
 	struct net_device *dev;
 	netdevice_tracker dev_tracker;
+	struct net_device *bind_dev; /* hint for raw_notify(), no reference */
 	struct list_head notifier;
 	int ifindex;
 	unsigned int bound:1;
@@ -224,6 +225,7 @@ static int raw_enable_filters(struct net *net, struct net_device *dev,
 	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);
@@ -258,9 +260,11 @@ static void raw_disable_filters(struct net *net, struct net_device *dev,
 {
 	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,
@@ -305,16 +309,28 @@ static void raw_notify(struct raw_sock *ro, unsigned long msg,
 		       struct net_device *dev)
 {
 	struct sock *sk = &ro->sk;
+	struct net_device *bind_dev;
 
 	if (!net_eq(dev_net(dev), sock_net(sk)))
 		return;
 
-	if (ro->dev != dev)
+	/* Lockless fast path, ro->bind_dev covers a concurrent bind().
+	 * Read it before ro->dev, pairs with smp_store_release() in
+	 * raw_bind(). A stale ro->bind_dev only costs a lock_sock().
+	 */
+	bind_dev = smp_load_acquire(&ro->bind_dev);
+	if (READ_ONCE(ro->dev) != dev &&
+	    (msg != NETDEV_UNREGISTER || bind_dev != dev))
 		return;
 
 	switch (msg) {
 	case NETDEV_UNREGISTER:
 		lock_sock(sk);
+		if (ro->dev != dev) {
+			release_sock(sk);
+			break;
+		}
+
 		/* remove current filters & unregister */
 		if (ro->bound) {
 			raw_disable_allfilters(dev_net(dev), dev, sk);
@@ -326,7 +342,7 @@ static void raw_notify(struct raw_sock *ro, unsigned long msg,
 
 		ro->ifindex = 0;
 		ro->bound = 0;
-		ro->dev = NULL;
+		WRITE_ONCE(ro->dev, NULL);
 		ro->count = 0;
 		release_sock(sk);
 
@@ -381,6 +397,7 @@ static int raw_init(struct sock *sk)
 	ro->bound            = 0;
 	ro->ifindex          = 0;
 	ro->dev              = NULL;
+	ro->bind_dev         = NULL;
 
 	/* set default filter to single entry dfilter */
 	ro->dfilter.can_id   = 0;
@@ -431,13 +448,15 @@ static int raw_release(struct socket *sock)
 	list_del(&ro->notifier);
 	spin_unlock(&raw_notifier_lock);
 
-	rtnl_lock();
 	lock_sock(sk);
 
-	/* remove current filters & unregister */
+	/* remove current filters & unregister
+	 * the tracked reference ro->dev keeps the device's receive lists
+	 * accessible, even after a namespace change
+	 */
 	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);
@@ -456,7 +475,6 @@ static int raw_release(struct socket *sock)
 	sock->sk = NULL;
 
 	release_sock(sk);
-	rtnl_unlock();
 
 	sock_prot_inuse_add(net, sk->sk_prot, -1);
 	sock_put(sk);
@@ -479,14 +497,25 @@ static int raw_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int len
 	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;
 
 	if (addr->can_ifindex) {
-		dev = dev_get_by_index(sock_net(sk), addr->can_ifindex);
+		/* Publish the device inside the RCU section of the lookup:
+		 * When it is unlisted concurrently, either raw_notify() sees
+		 * ro->bind_dev after synchronize_net() or the lookup fails.
+		 */
+		rcu_read_lock();
+		dev = dev_get_by_index_rcu(sock_net(sk), addr->can_ifindex);
+		if (dev) {
+			dev_hold(dev);
+			/* pairs with smp_load_acquire() in raw_notify() */
+			smp_store_release(&ro->bind_dev, dev);
+		}
+		rcu_read_unlock();
 		if (!dev) {
 			err = -ENODEV;
 			goto out;
@@ -496,7 +525,7 @@ static int raw_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int len
 			goto out_put_dev;
 		}
 
-		if (!(dev->flags & IFF_UP))
+		if (!(READ_ONCE(dev->flags) & IFF_UP))
 			notify_enetdown = 1;
 
 		ifindex = dev->ifindex;
@@ -517,7 +546,7 @@ static int raw_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int len
 		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);
@@ -528,16 +557,15 @@ static int raw_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int len
 		ro->ifindex = ifindex;
 		ro->bound = 1;
 		/* bind() ok -> hold a reference for new ro->dev */
-		ro->dev = dev;
+		WRITE_ONCE(ro->dev, dev);
 		netdev_hold(ro->dev, &ro->dev_tracker, GFP_KERNEL);
 	}
 
 out_put_dev:
-	/* remove potential reference from dev_get_by_index() */
+	/* remove reference from the device lookup */
 	dev_put(dev);
 out:
 	release_sock(sk);
-	rtnl_unlock();
 
 	if (notify_enetdown) {
 		sk->sk_err = ENETDOWN;
@@ -566,58 +594,41 @@ static int raw_getname(struct socket *sock, struct sockaddr *uaddr,
 }
 
 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;
-		}
-
+		/* @filter has been copied by raw_setsockopt(). It is linked
+		 * to the socket on success and freed by the caller on error.
+		 *
+		 * The reg_state check is only a shortcut: On a concurrent
+		 * unregistration or namespace change 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,
@@ -631,7 +642,7 @@ static int raw_setsockopt_locked(struct socket *sock, int optname,
 		/* 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;
@@ -647,8 +658,10 @@ static int raw_setsockopt_locked(struct socket *sock, int optname,
 
 		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 */
@@ -750,18 +763,47 @@ 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 before lock_sock(), so a stalled copy can
+		 * not block raw_notify() waiting for lock_sock() under RTNL.
+		 */
+		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;
+		}
+	}
+
 	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();
+
+	/* on error free the filters allocated for CAN_RAW_FILTER above */
+	if (err && count > 1)
+		kfree(filter);
 
 	return err;
 }
-- 
2.53.0

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

[netdev-bot+sashiko]

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

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

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

- [Medium] raw_bind() no longer holds RTNL, so the socket can miss a
  NETDEV_DOWN that arrives while it is binding.
- [Low] The commit message says "the callers pass the namespace that has
  been used for can_rx_register()", and the new kernel-doc of…
- [Low] The new comment on netns_can.rcvlists_lock in
  include/net/netns/can.h says it "protects rcv_lists_stats".

--

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

This commit adds a spinlock to struct can_dev_rcv_lists. The receive lists
of a CAN device are now protected per device instead of by the per-netns
rcvlists_lock, so can_rx_unregister() still finds subscriptions after the
device has moved to another namespace. It also drops RTNL from raw_bind(),
raw_setsockopt(), raw_release(), isotp_bind() and isotp_release(), and
relies on lock_sock() plus a device pointer published inside the RCU
lookup to serialize with the netdev notifiers.

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

> 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
> @@ -29,7 +29,7 @@ struct netns_can {
>  
>  	/* 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 */

[Severity: Low]
This isn't a bug, but is this comment accurate? The lock only serializes
the rcv_entries and rcv_entries_max updates in can_rx_register() and
can_rx_unregister().

can_init_stats() in net/can/proc.c updates other fields of the same
structure without taking rcvlists_lock. It runs from the stats timer, or
from the reset_stats proc read when the timer is disabled:

net/can/proc.c:can_init_stats() {
    ...
	rcv_lists_stats->stats_reset++;

	if (user_reset) {
		user_reset = 0;
		rcv_lists_stats->user_reset++;
	}
}

can_stats_proc_show() also reads these counters without the lock.

Could the comment name rcv_entries and rcv_entries_max specifically?

[ ... ]

> diff --git a/net/can/af_can.c b/net/can/af_can.c
> index d97f85fc3232..bf612c4b983d 100644
> --- a/net/can/af_can.c
> +++ b/net/can/af_can.c

[ ... ]

> @@ -509,6 +513,13 @@ static void can_rx_delete_receiver(struct rcu_head *rp)
>   *
>   * 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().
> + *  It selects the 'any' CAN device lists for @dev == NULL and is used for
> + *  the receive list statistics.
>   */

[Severity: Low]
The new kernel-doc says @net is the namespace that was used for
can_rx_register(). The commit message says the same: "the callers pass
the namespace that has been used for can_rx_register()".

Does j1939 follow this? It reads dev_net(ndev) again at unregister time:

net/can/j1939/main.c:j1939_can_rx_register() {
    ...
	ret = can_rx_register(dev_net(ndev), ndev, J1939_CAN_ID, J1939_CAN_MASK,
			      j1939_can_recv, priv, 0, NULL);
    ...
}

net/can/j1939/main.c:j1939_can_rx_unregister() {
    ...
	can_rx_unregister(dev_net(ndev), ndev, J1939_CAN_ID, J1939_CAN_MASK,
			  j1939_can_recv, priv);
    ...
}

A j1939 registration can outlive a namespace move. j1939_sk_bind() does
not hold RTNL. It calls j1939_netdev_start(), which registers with the old
namespace, before it calls j1939_jsk_add().

Suppose the NETDEV_UNREGISTER from __dev_change_net_namespace() runs
j1939_sk_netdev_event_unregister() in that window. The socket is not
J1939_SOCK_BOUND yet, so it is skipped and the registration survives the
move. The final release then passes the new namespace:

j1939_netdev_stop()
  __j1939_rx_release()
    j1939_can_rx_unregister()
      can_rx_unregister(dev_net(ndev), ...)

The receiver is still found through the per-device lists. However,
rcv_entries is decremented in the new namespace's rcv_lists_stats
(clamped at 0) and stays elevated in the old one. This accounting matches
the behaviour before the patch.

Should j1939_priv store the registration namespace (with a netns
reference)? Or should the kernel-doc and commit message describe a looser
rule?

[ ... ]

> diff --git a/net/can/raw.c b/net/can/raw.c
> index dfea48768b23..0d8edfa772fb 100644
> --- a/net/can/raw.c
> +++ b/net/can/raw.c

[ ... ]

> @@ -305,16 +309,28 @@ static void raw_notify(struct raw_sock *ro, unsigned long msg,
>  		       struct net_device *dev)
>  {
>  	struct sock *sk = &ro->sk;
> +	struct net_device *bind_dev;
>  
>  	if (!net_eq(dev_net(dev), sock_net(sk)))
>  		return;
>  
> -	if (ro->dev != dev)
> +	/* Lockless fast path, ro->bind_dev covers a concurrent bind().
> +	 * Read it before ro->dev, pairs with smp_store_release() in
> +	 * raw_bind(). A stale ro->bind_dev only costs a lock_sock().
> +	 */
> +	bind_dev = smp_load_acquire(&ro->bind_dev);
> +	if (READ_ONCE(ro->dev) != dev &&
> +	    (msg != NETDEV_UNREGISTER || bind_dev != dev))
>  		return;

[Severity: Medium]
Can a NETDEV_DOWN be lost while raw_bind() is running? The bind_dev hint
is only checked for NETDEV_UNREGISTER, and raw_bind() no longer holds
RTNL.

raw_bind() now runs these steps in order:

    smp_store_release(&ro->bind_dev, dev);
    ...
    if (!(READ_ONCE(dev->flags) & IFF_UP))   /* device still up here */
        notify_enetdown = 1;
    ...
    err = raw_enable_allfilters(sock_net(sk), dev, sk);  /* cond_resched() */
    ...
    WRITE_ONCE(ro->dev, dev);

Suppose dev_close() runs after the IFF_UP check but before ro->dev is
published. raw_notifier()->raw_notify() then sees ro->dev != dev with
msg == NETDEV_DOWN, so it returns at the check above and never reaches
case NETDEV_DOWN. raw_bind() finishes with notify_enetdown == 0.

The socket ends up bound to a down interface, and sk_err = ENETDOWN /
sk_error_report() is never raised. Before this patch, rtnl_lock() in
raw_bind() kept dev_close() out of this window.

isotp_bind() publishes so->dev before it reads IFF_UP, so it does not seem
to have this problem.

Could raw_notify() also check bind_dev for NETDEV_DOWN? Alternatively,
raw_bind() could publish ro->dev before the IFF_UP check.

[ ... ]

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

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

[Oliver Hartkopp]

On 20.10.26 19:11, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 2
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] raw_bind() no longer holds RTNL, so the socket can miss a
>    NETDEV_DOWN that arrives while it is binding.
> - [Low] The commit message says "the callers pass the namespace that has
>    been used for can_rx_register()", and the new kernel-doc of…
> - [Low] The new comment on netns_can.rcvlists_lock in
>    include/net/netns/can.h says it "protects rcv_lists_stats".
> 

Thanks for the review.

[Medium] NETDEV_DOWN during raw_bind():

The window exists, but it only affects a down transition in the middle
of raw_bind(). Missing the asynchronous ENETDOWN there is harmless and
not worth an additional barrier pair in raw_bind() and raw_notify():

- Every transmission on a CAN_RAW socket goes through can_send(), which
   checks IFF_UP and returns -ENETDOWN to sendmsg(). So the state is
   reported reliably on every send attempt.

- The missed notification does not affect the binding: the CAN_RAW
   socket stays bound and its filters stay registered across down/up,
   and frames are received again once the interface is up.

- Applications can not rely on this notification anyway: a CAN_RAW
   socket bound to all CAN interfaces (ifindex 0) never gets ENETDOWN.
   Applications that need the link state have to track it via
   rtnetlink, not via the CAN socket.

[Low] rcvlists_lock comment / j1939 @net: These are documentation
details; the j1939 statistics behaviour is unchanged by this patch, as
noted in the patch description.

Best regards,
Oliver


> --
> 
> can: protect the receive lists with a per-device lock
> 
> This commit adds a spinlock to struct can_dev_rcv_lists. The receive lists
> of a CAN device are now protected per device instead of by the per-netns
> rcvlists_lock, so can_rx_unregister() still finds subscriptions after the
> device has moved to another namespace. It also drops RTNL from raw_bind(),
> raw_setsockopt(), raw_release(), isotp_bind() and isotp_release(), and
> relies on lock_sock() plus a device pointer published inside the RCU
> lookup to serialize with the netdev notifiers.
> 
> Link: https://lore.kernel.org/linux-can/CEA6A38A-2646-4ADA-95B4-CBAE2F301A8E@doyensec.com/
> Link: https://patch.msgid.link/20261019152741.54854-6-socketcan@hartkopp.net
> 
>> 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
>> @@ -29,7 +29,7 @@ struct netns_can {
>>   
>>   	/* 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 */
> 
> [Severity: Low]
> This isn't a bug, but is this comment accurate? The lock only serializes
> the rcv_entries and rcv_entries_max updates in can_rx_register() and
> can_rx_unregister().
> 
> can_init_stats() in net/can/proc.c updates other fields of the same
> structure without taking rcvlists_lock. It runs from the stats timer, or
> from the reset_stats proc read when the timer is disabled:
> 
> net/can/proc.c:can_init_stats() {
>      ...
> 	rcv_lists_stats->stats_reset++;
> 
> 	if (user_reset) {
> 		user_reset = 0;
> 		rcv_lists_stats->user_reset++;
> 	}
> }
> 
> can_stats_proc_show() also reads these counters without the lock.
> 
> Could the comment name rcv_entries and rcv_entries_max specifically?
> 
> [ ... ]
> 
>> diff --git a/net/can/af_can.c b/net/can/af_can.c
>> index d97f85fc3232..bf612c4b983d 100644
>> --- a/net/can/af_can.c
>> +++ b/net/can/af_can.c
> 
> [ ... ]
> 
>> @@ -509,6 +513,13 @@ static void can_rx_delete_receiver(struct rcu_head *rp)
>>    *
>>    * 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().
>> + *  It selects the 'any' CAN device lists for @dev == NULL and is used for
>> + *  the receive list statistics.
>>    */
> 
> [Severity: Low]
> The new kernel-doc says @net is the namespace that was used for
> can_rx_register(). The commit message says the same: "the callers pass
> the namespace that has been used for can_rx_register()".
> 
> Does j1939 follow this? It reads dev_net(ndev) again at unregister time:
> 
> net/can/j1939/main.c:j1939_can_rx_register() {
>      ...
> 	ret = can_rx_register(dev_net(ndev), ndev, J1939_CAN_ID, J1939_CAN_MASK,
> 			      j1939_can_recv, priv, 0, NULL);
>      ...
> }
> 
> net/can/j1939/main.c:j1939_can_rx_unregister() {
>      ...
> 	can_rx_unregister(dev_net(ndev), ndev, J1939_CAN_ID, J1939_CAN_MASK,
> 			  j1939_can_recv, priv);
>      ...
> }
> 
> A j1939 registration can outlive a namespace move. j1939_sk_bind() does
> not hold RTNL. It calls j1939_netdev_start(), which registers with the old
> namespace, before it calls j1939_jsk_add().
> 
> Suppose the NETDEV_UNREGISTER from __dev_change_net_namespace() runs
> j1939_sk_netdev_event_unregister() in that window. The socket is not
> J1939_SOCK_BOUND yet, so it is skipped and the registration survives the
> move. The final release then passes the new namespace:
> 
> j1939_netdev_stop()
>    __j1939_rx_release()
>      j1939_can_rx_unregister()
>        can_rx_unregister(dev_net(ndev), ...)
> 
> The receiver is still found through the per-device lists. However,
> rcv_entries is decremented in the new namespace's rcv_lists_stats
> (clamped at 0) and stays elevated in the old one. This accounting matches
> the behaviour before the patch.
> 
> Should j1939_priv store the registration namespace (with a netns
> reference)? Or should the kernel-doc and commit message describe a looser
> rule?
> 
> [ ... ]
> 
>> diff --git a/net/can/raw.c b/net/can/raw.c
>> index dfea48768b23..0d8edfa772fb 100644
>> --- a/net/can/raw.c
>> +++ b/net/can/raw.c
> 
> [ ... ]
> 
>> @@ -305,16 +309,28 @@ static void raw_notify(struct raw_sock *ro, unsigned long msg,
>>   		       struct net_device *dev)
>>   {
>>   	struct sock *sk = &ro->sk;
>> +	struct net_device *bind_dev;
>>   
>>   	if (!net_eq(dev_net(dev), sock_net(sk)))
>>   		return;
>>   
>> -	if (ro->dev != dev)
>> +	/* Lockless fast path, ro->bind_dev covers a concurrent bind().
>> +	 * Read it before ro->dev, pairs with smp_store_release() in
>> +	 * raw_bind(). A stale ro->bind_dev only costs a lock_sock().
>> +	 */
>> +	bind_dev = smp_load_acquire(&ro->bind_dev);
>> +	if (READ_ONCE(ro->dev) != dev &&
>> +	    (msg != NETDEV_UNREGISTER || bind_dev != dev))
>>   		return;
> 
> [Severity: Medium]
> Can a NETDEV_DOWN be lost while raw_bind() is running? The bind_dev hint
> is only checked for NETDEV_UNREGISTER, and raw_bind() no longer holds
> RTNL.
> 
> raw_bind() now runs these steps in order:
> 
>      smp_store_release(&ro->bind_dev, dev);
>      ...
>      if (!(READ_ONCE(dev->flags) & IFF_UP))   /* device still up here */
>          notify_enetdown = 1;
>      ...
>      err = raw_enable_allfilters(sock_net(sk), dev, sk);  /* cond_resched() */
>      ...
>      WRITE_ONCE(ro->dev, dev);
> 
> Suppose dev_close() runs after the IFF_UP check but before ro->dev is
> published. raw_notifier()->raw_notify() then sees ro->dev != dev with
> msg == NETDEV_DOWN, so it returns at the check above and never reaches
> case NETDEV_DOWN. raw_bind() finishes with notify_enetdown == 0.
> 
> The socket ends up bound to a down interface, and sk_err = ENETDOWN /
> sk_error_report() is never raised. Before this patch, rtnl_lock() in
> raw_bind() kept dev_close() out of this window.
> 
> isotp_bind() publishes so->dev before it reads IFF_UP, so it does not seem
> to have this problem.
> 
> Could raw_notify() also check bind_dev for NETDEV_DOWN? Alternatively,
> raw_bind() could publish ro->dev before the IFF_UP check.
> 
> [ ... ]
> 

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

[Oliver Hartkopp]

On 20.10.26 21:00, Oliver Hartkopp wrote:
> 
> 
> On 20.10.26 19:11, netdev-bot+sashiko@kernel.org wrote:
>> Thank you for your contribution! Sashiko AI review found 3 potential
>> issue(s) to consider.
>>
>> Critical: 0 · High: 0 · Medium: 1 · Low: 2
>>
>> If you believe none of the issues are legitimate, please reply to each
>> issue of Medium severity or higher and explain why the AI is wrong.
>> If any of them are legitimate and you plan to respin, please let 
>> patchwork
>> know by including "pw-bot: cr" as a separate line at the end of your 
>> reply
>> (one such reply per series is enough).
>>
>> - [Medium] raw_bind() no longer holds RTNL, so the socket can miss a
>>    NETDEV_DOWN that arrives while it is binding.
>> - [Low] The commit message says "the callers pass the namespace that has
>>    been used for can_rx_register()", and the new kernel-doc of…
>> - [Low] The new comment on netns_can.rcvlists_lock in
>>    include/net/netns/can.h says it "protects rcv_lists_stats".
>>
> 
> Thanks for the review.
> 
> [Medium] NETDEV_DOWN during raw_bind():
> 
> The window exists, but it only affects a down transition in the middle
> of raw_bind(). Missing the asynchronous ENETDOWN there is harmless and
> not worth an additional barrier pair in raw_bind() and raw_notify():


I did a closer look into it.

Although the missing of -ENETDOWN in raw_bind() is not a big issue it 
changes the former behaviour which is more relevant for reading from the 
CAN_RAW socket than writing to it.

 >> isotp_bind() publishes so->dev before it reads IFF_UP, so it does not
 >> seem to have this problem.

In fact both raw and isotp need smp_mb() to have the correct ordering.
I would bet a beer sashiko-bot would have remarked it next time :-/

While at it I also fixed the low severity commit and documentation 
remarks from sashiko-bot in patch 5 and patch 4.

New v5 patch set with updated cover letter for can-next is here:

https://lore.kernel.org/linux-can/20261020203257.230955-1-socketcan@hartkopp.net/

Best regards,
Oliver

pw-bot: cr

> 
> - Every transmission on a CAN_RAW socket goes through can_send(), which
>    checks IFF_UP and returns -ENETDOWN to sendmsg(). So the state is
>    reported reliably on every send attempt.
> 
> - The missed notification does not affect the binding: the CAN_RAW
>    socket stays bound and its filters stay registered across down/up,
>    and frames are received again once the interface is up.
> 
> - Applications can not rely on this notification anyway: a CAN_RAW
>    socket bound to all CAN interfaces (ifindex 0) never gets ENETDOWN.
>    Applications that need the link state have to track it via
>    rtnetlink, not via the CAN socket.
> 
> [Low] rcvlists_lock comment / j1939 @net: These are documentation
> details; the j1939 statistics behaviour is unchanged by this patch, as
> noted in the patch description.
> 
> Best regards,
> Oliver
> 
> 
>> -- 
>>
>> can: protect the receive lists with a per-device lock
>>
>> This commit adds a spinlock to struct can_dev_rcv_lists. The receive 
>> lists
>> of a CAN device are now protected per device instead of by the per-netns
>> rcvlists_lock, so can_rx_unregister() still finds subscriptions after the
>> device has moved to another namespace. It also drops RTNL from 
>> raw_bind(),
>> raw_setsockopt(), raw_release(), isotp_bind() and isotp_release(), and
>> relies on lock_sock() plus a device pointer published inside the RCU
>> lookup to serialize with the netdev notifiers.
>>
>> Link: https://lore.kernel.org/linux-can/CEA6A38A-2646-4ADA-95B4- 
>> CBAE2F301A8E@doyensec.com/
>> Link: https://patch.msgid.link/20261019152741.54854-6- 
>> socketcan@hartkopp.net
>>
>>> 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
>>> @@ -29,7 +29,7 @@ struct netns_can {
>>>       /* 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 */
>>
>> [Severity: Low]
>> This isn't a bug, but is this comment accurate? The lock only serializes
>> the rcv_entries and rcv_entries_max updates in can_rx_register() and
>> can_rx_unregister().
>>
>> can_init_stats() in net/can/proc.c updates other fields of the same
>> structure without taking rcvlists_lock. It runs from the stats timer, or
>> from the reset_stats proc read when the timer is disabled:
>>
>> net/can/proc.c:can_init_stats() {
>>      ...
>>     rcv_lists_stats->stats_reset++;
>>
>>     if (user_reset) {
>>         user_reset = 0;
>>         rcv_lists_stats->user_reset++;
>>     }
>> }
>>
>> can_stats_proc_show() also reads these counters without the lock.
>>
>> Could the comment name rcv_entries and rcv_entries_max specifically?
>>
>> [ ... ]
>>
>>> diff --git a/net/can/af_can.c b/net/can/af_can.c
>>> index d97f85fc3232..bf612c4b983d 100644
>>> --- a/net/can/af_can.c
>>> +++ b/net/can/af_can.c
>>
>> [ ... ]
>>
>>> @@ -509,6 +513,13 @@ static void can_rx_delete_receiver(struct 
>>> rcu_head *rp)
>>>    *
>>>    * 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().
>>> + *  It selects the 'any' CAN device lists for @dev == NULL and is 
>>> used for
>>> + *  the receive list statistics.
>>>    */
>>
>> [Severity: Low]
>> The new kernel-doc says @net is the namespace that was used for
>> can_rx_register(). The commit message says the same: "the callers pass
>> the namespace that has been used for can_rx_register()".
>>
>> Does j1939 follow this? It reads dev_net(ndev) again at unregister time:
>>
>> net/can/j1939/main.c:j1939_can_rx_register() {
>>      ...
>>     ret = can_rx_register(dev_net(ndev), ndev, J1939_CAN_ID, 
>> J1939_CAN_MASK,
>>                   j1939_can_recv, priv, 0, NULL);
>>      ...
>> }
>>
>> net/can/j1939/main.c:j1939_can_rx_unregister() {
>>      ...
>>     can_rx_unregister(dev_net(ndev), ndev, J1939_CAN_ID, J1939_CAN_MASK,
>>               j1939_can_recv, priv);
>>      ...
>> }
>>
>> A j1939 registration can outlive a namespace move. j1939_sk_bind() does
>> not hold RTNL. It calls j1939_netdev_start(), which registers with the 
>> old
>> namespace, before it calls j1939_jsk_add().
>>
>> Suppose the NETDEV_UNREGISTER from __dev_change_net_namespace() runs
>> j1939_sk_netdev_event_unregister() in that window. The socket is not
>> J1939_SOCK_BOUND yet, so it is skipped and the registration survives the
>> move. The final release then passes the new namespace:
>>
>> j1939_netdev_stop()
>>    __j1939_rx_release()
>>      j1939_can_rx_unregister()
>>        can_rx_unregister(dev_net(ndev), ...)
>>
>> The receiver is still found through the per-device lists. However,
>> rcv_entries is decremented in the new namespace's rcv_lists_stats
>> (clamped at 0) and stays elevated in the old one. This accounting matches
>> the behaviour before the patch.
>>
>> Should j1939_priv store the registration namespace (with a netns
>> reference)? Or should the kernel-doc and commit message describe a looser
>> rule?
>>
>> [ ... ]
>>
>>> diff --git a/net/can/raw.c b/net/can/raw.c
>>> index dfea48768b23..0d8edfa772fb 100644
>>> --- a/net/can/raw.c
>>> +++ b/net/can/raw.c
>>
>> [ ... ]
>>
>>> @@ -305,16 +309,28 @@ static void raw_notify(struct raw_sock *ro, 
>>> unsigned long msg,
>>>                  struct net_device *dev)
>>>   {
>>>       struct sock *sk = &ro->sk;
>>> +    struct net_device *bind_dev;
>>>       if (!net_eq(dev_net(dev), sock_net(sk)))
>>>           return;
>>> -    if (ro->dev != dev)
>>> +    /* Lockless fast path, ro->bind_dev covers a concurrent bind().
>>> +     * Read it before ro->dev, pairs with smp_store_release() in
>>> +     * raw_bind(). A stale ro->bind_dev only costs a lock_sock().
>>> +     */
>>> +    bind_dev = smp_load_acquire(&ro->bind_dev);
>>> +    if (READ_ONCE(ro->dev) != dev &&
>>> +        (msg != NETDEV_UNREGISTER || bind_dev != dev))
>>>           return;
>>
>> [Severity: Medium]
>> Can a NETDEV_DOWN be lost while raw_bind() is running? The bind_dev hint
>> is only checked for NETDEV_UNREGISTER, and raw_bind() no longer holds
>> RTNL.
>>
>> raw_bind() now runs these steps in order:
>>
>>      smp_store_release(&ro->bind_dev, dev);
>>      ...
>>      if (!(READ_ONCE(dev->flags) & IFF_UP))   /* device still up here */
>>          notify_enetdown = 1;
>>      ...
>>      err = raw_enable_allfilters(sock_net(sk), dev, sk);  /* 
>> cond_resched() */
>>      ...
>>      WRITE_ONCE(ro->dev, dev);
>>
>> Suppose dev_close() runs after the IFF_UP check but before ro->dev is
>> published. raw_notifier()->raw_notify() then sees ro->dev != dev with
>> msg == NETDEV_DOWN, so it returns at the check above and never reaches
>> case NETDEV_DOWN. raw_bind() finishes with notify_enetdown == 0.
>>
>> The socket ends up bound to a down interface, and sk_err = ENETDOWN /
>> sk_error_report() is never raised. Before this patch, rtnl_lock() in
>> raw_bind() kept dev_close() out of this window.
>>
>> isotp_bind() publishes so->dev before it reads IFF_UP, so it does not 
>> seem
>> to have this problem.
>>
>> Could raw_notify() also check bind_dev for NETDEV_DOWN? Alternatively,
>> raw_bind() could publish ro->dev before the IFF_UP check.
>>
>> [ ... ]
>>
> 
>