* [PATCH v4 1/3] can: remove CAN filters independent from namespace
2026-09-29 15:41 [PATCH v4 0/3] CAN netlayer fixes for stable Oliver Hartkopp
@ 2026-09-29 15:41 ` Oliver Hartkopp
2026-10-03 6:00 ` netdev-bot+sashiko
2026-09-29 15:41 ` [PATCH v4 2/3] can: convert unreliable ARPHRD_CAN type checks to robust can_get_ml_priv() Oliver Hartkopp
2026-09-29 15:41 ` [PATCH v4 3/3] can: fix unique skb identifier regression under RPS Oliver Hartkopp
2 siblings, 1 reply; 7+ messages in thread
From: Oliver Hartkopp @ 2026-09-29 15:41 UTC (permalink / raw)
To: linux-can; +Cc: Oliver Hartkopp, Norbert Szetei, stable
When the devices namespace is changed the socket namespace and the device
namespace might differ. The net_eq(dev_net(dev), sock_net(sk)) check in
the CAN protocols netdev notifiers therefore led to skipping the required
removal of the CAN filters from the (namespace changed) CAN devices.
This patch removes the namespace equality check in the netdev notifiers for
BCM, ISOTP and RAW sockets. Since the struct net_device pointer is globally
unique, the notifier should always process the unregister event and remove
the CAN filters if it matches the original socket's bound device pointer.
In bcm.c netdevice comparisons were performed by checking the interface
index (bo->ifindex and op->ifindex) which is not namespace-safe either.
Introduce tracked netdevice pointers (bo->dev and op->tx_dev) for these
referenced devices to enable namespace-save device comparisons.
Additional put all bo->dev accesses in bcm_notify() under lock_sock().
In isotp.c the two missing can_rx_unregister() calling sites are converted
to use dev_net(dev) instead of sock_net(sk) to get the correct namespace.
Fixes: 8e8cda6d737d ("can: initial support for network namespaces")
Reported-by: Norbert Szetei <norbert@doyensec.com>
Link: https://lore.kernel.org/linux-can/CEA6A38A-2646-4ADA-95B4-CBAE2F301A8E@doyensec.com/
Cc: stable@vger.kernel.org
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
net/can/bcm.c | 100 ++++++++++++++++++++++++++++++++++++------------
net/can/isotp.c | 7 +---
net/can/raw.c | 3 --
3 files changed, 78 insertions(+), 32 deletions(-)
diff --git a/net/can/bcm.c b/net/can/bcm.c
index 3d637a1e0ac1..cd3522ec32c0 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -128,18 +128,22 @@ struct bcm_op {
struct canfd_frame sframe;
struct canfd_frame last_sframe;
struct sock *sk;
struct net_device *rx_reg_dev;
netdevice_tracker rx_reg_dev_tracker;
+ struct net_device *tx_dev;
+ netdevice_tracker tx_dev_tracker;
spinlock_t bcm_tx_lock; /* protect tx data and timer updates */
spinlock_t bcm_rx_update_lock; /* protect filter/timer data updates */
};
struct bcm_sock {
struct sock sk;
int bound;
int ifindex;
+ struct net_device *dev;
+ netdevice_tracker dev_tracker;
struct list_head notifier;
struct list_head rx_ops;
struct list_head tx_ops;
unsigned long dropped_usr_msgs;
struct proc_dir_entry *bcm_proc_read;
@@ -933,10 +937,13 @@ static void bcm_free_op_work(struct work_struct *work)
kfree(op->frames);
if ((op->last_frames) && (op->last_frames != &op->last_sframe))
kfree(op->last_frames);
+ if (op->tx_dev)
+ netdev_put(op->tx_dev, &op->tx_dev_tracker);
+
/* the last possible access to op->timer/op->thrtimer has now
* happened above via hrtimer_cancel() - op->sk is no longer
* needed by any pending timer callback, so drop our reference
*/
sock_put(op->sk);
@@ -1072,10 +1079,11 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
int ifindex, struct sock *sk)
{
struct bcm_sock *bo = bcm_sk(sk);
struct bcm_op *op;
struct canfd_frame *cf;
+ struct net_device *tx_dev;
bool add_op_to_list = false;
unsigned int i;
int err;
/* we need a real device to send frames */
@@ -1103,10 +1111,26 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
* therefore (complexity / locking) it is not supported.
*/
if (msg_head->nframes > op->nframes)
return -E2BIG;
+ /* Re-resolve and re-hold the target device if a concurrent
+ * NETDEV_UNREGISTER already cleared it (see bcm_notify()).
+ * op->ifindex and sock_net(sk) is unchanged.
+ */
+ if (!op->tx_dev) {
+ tx_dev = dev_get_by_index(sock_net(sk), ifindex);
+ if (tx_dev) {
+ op->tx_dev = tx_dev;
+ netdev_hold(tx_dev, &op->tx_dev_tracker,
+ GFP_KERNEL);
+ dev_put(tx_dev);
+ } else {
+ return -ENODEV;
+ }
+ }
+
/* get new CAN frames content into a staging buffer before
* locking: validate and normalize the frames there so that
* bcm_can_tx() / bcm_tx_timeout_handler() never observe a
* partially updated or unvalidated frame in op->frames
*/
@@ -1169,10 +1193,22 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
op = kzalloc(OPSIZ, GFP_KERNEL);
if (!op)
return -ENOMEM;
+ tx_dev = dev_get_by_index(sock_net(sk), ifindex);
+ if (tx_dev) {
+ op->tx_dev = tx_dev;
+ netdev_hold(tx_dev, &op->tx_dev_tracker, GFP_KERNEL);
+ dev_put(tx_dev);
+ } else {
+ /* prepare op->frames for goto free_op */
+ op->frames = &op->sframe;
+ err = -ENODEV;
+ goto free_op;
+ }
+
spin_lock_init(&op->bcm_tx_lock);
op->can_id = msg_head->can_id;
op->cfsiz = CFSIZ(msg_head->flags);
op->flags = msg_head->flags;
op->nframes = msg_head->nframes;
@@ -1184,12 +1220,14 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
if (msg_head->nframes > 1) {
op->frames = kmalloc_array(msg_head->nframes,
op->cfsiz,
GFP_KERNEL);
if (!op->frames) {
- kfree(op);
- return -ENOMEM;
+ /* prepare op->frames for goto free_op */
+ op->frames = &op->sframe;
+ err = -ENOMEM;
+ goto free_op;
}
} else
op->frames = &op->sframe;
for (i = 0; i < msg_head->nframes; i++) {
@@ -1267,10 +1305,13 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
bcm_tx_start_timer(op);
return msg_head->nframes * op->cfsiz + MHSIZ;
free_op:
+ if (op->tx_dev)
+ netdev_put(op->tx_dev, &op->tx_dev_tracker);
+
if (op->frames != &op->sframe)
kfree(op->frames);
kfree(op);
return err;
}
@@ -1790,46 +1831,47 @@ static int bcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
static void bcm_notify(struct bcm_sock *bo, unsigned long msg,
struct net_device *dev)
{
struct sock *sk = &bo->sk;
struct bcm_op *op;
- int notify_enodev = 0;
+ int sk_err = 0;
- if (!net_eq(dev_net(dev), sock_net(sk)))
- return;
+ lock_sock(sk);
switch (msg) {
case NETDEV_UNREGISTER:
- lock_sock(sk);
/* rx_ops: remove device specific receive entries */
list_for_each_entry(op, &bo->rx_ops, list) {
if (op->rx_reg_dev == dev)
bcm_rx_unreg(dev, op);
/* release an ANYDEV op's claim (see bcm_rx_handler())
* on this now confirmed-gone interface.
*/
- if (!op->ifindex) {
+ if (!op->ifindex && net_eq(dev_net(dev), sock_net(sk))) {
spin_lock_bh(&op->bcm_rx_update_lock);
if (op->if_detected == dev->ifindex)
op->if_detected = 0;
spin_unlock_bh(&op->bcm_rx_update_lock);
}
}
/* tx_ops: stop device specific cyclic transmissions on the
- * vanishing ifindex. Cancelling the timer is enough to stop
+ * vanishing device. Cancelling the timer is enough to stop
* cyclic bcm_can_tx() calls as there is no re-arming.
*/
list_for_each_entry(op, &bo->tx_ops, list)
- if (op->ifindex == dev->ifindex)
+ if (op->tx_dev == dev) {
hrtimer_cancel(&op->timer);
+ netdev_put(op->tx_dev, &op->tx_dev_tracker);
+ op->tx_dev = NULL;
+ }
/* remove device reference, if this is our bound device */
- if (bo->bound && bo->ifindex == dev->ifindex) {
+ if (bo->bound && bo->dev == dev) {
#if IS_ENABLED(CONFIG_PROC_FS)
if (sock_net(sk)->can.bcmproc_dir && bo->bcm_proc_read) {
remove_proc_entry(bo->procname, sock_net(sk)->can.bcmproc_dir);
bo->bcm_proc_read = NULL;
}
@@ -1839,28 +1881,27 @@ static void bcm_notify(struct bcm_sock *bo, unsigned long msg,
* accessed under lock_sock() so it needs no
* annotation.
*/
WRITE_ONCE(bo->bound, 0);
bo->ifindex = 0;
- notify_enodev = 1;
- }
-
- release_sock(sk);
-
- if (notify_enodev) {
- sk->sk_err = ENODEV;
- if (!sock_flag(sk, SOCK_DEAD))
- sk_error_report(sk);
+ netdev_put(bo->dev, &bo->dev_tracker);
+ bo->dev = NULL;
+ sk_err = ENODEV;
}
break;
case NETDEV_DOWN:
- if (bo->bound && bo->ifindex == dev->ifindex) {
- sk->sk_err = ENETDOWN;
- if (!sock_flag(sk, SOCK_DEAD))
- sk_error_report(sk);
- }
+ if (bo->bound && bo->dev == dev)
+ sk_err = ENETDOWN;
+ }
+
+ release_sock(sk);
+
+ if (sk_err) {
+ sk->sk_err = sk_err;
+ if (!sock_flag(sk, SOCK_DEAD))
+ sk_error_report(sk);
}
}
static int bcm_notifier(struct notifier_block *nb, unsigned long msg,
void *ptr)
@@ -1982,10 +2023,14 @@ static int bcm_release(struct socket *sock)
/* remove device reference */
if (bo->bound) {
WRITE_ONCE(bo->bound, 0);
bo->ifindex = 0;
+ if (bo->dev) {
+ netdev_put(bo->dev, &bo->dev_tracker);
+ bo->dev = NULL;
+ }
}
sock_orphan(sk);
sock->sk = NULL;
@@ -2029,25 +2074,32 @@ static int bcm_connect(struct socket *sock, struct sockaddr_unsized *uaddr, int
ret = -ENODEV;
goto fail;
}
bo->ifindex = dev->ifindex;
+ bo->dev = dev;
+ netdev_hold(dev, &bo->dev_tracker, GFP_KERNEL);
dev_put(dev);
} else {
/* no interface reference for ifindex = 0 ('any' CAN device) */
bo->ifindex = 0;
+ bo->dev = NULL;
}
#if IS_ENABLED(CONFIG_PROC_FS)
if (net->can.bcmproc_dir) {
/* unique socket address as filename */
sprintf(bo->procname, "%llu", sock_i_ino(sk));
bo->bcm_proc_read = proc_create_net_single(bo->procname, 0644,
net->can.bcmproc_dir,
bcm_proc_show, sk);
if (!bo->bcm_proc_read) {
+ if (bo->dev) {
+ netdev_put(bo->dev, &bo->dev_tracker);
+ bo->dev = NULL;
+ }
ret = -ENOMEM;
goto fail;
}
}
#endif /* CONFIG_PROC_FS */
diff --git a/net/can/isotp.c b/net/can/isotp.c
index 155530aedce2..0835a4758a72 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -1490,15 +1490,15 @@ static int isotp_release(struct socket *sock)
/* remove current filters & unregister
* tracked reference so->dev is taken at bind() time with rtnl_lock
*/
if (so->bound && so->dev) {
if (isotp_register_rxid(so))
- can_rx_unregister(net, so->dev, so->rxid,
+ can_rx_unregister(dev_net(so->dev), so->dev, so->rxid,
SINGLE_MASK(so->rxid),
isotp_rcv, sk);
- can_rx_unregister(net, so->dev, so->txid,
+ can_rx_unregister(dev_net(so->dev), so->dev, so->txid,
SINGLE_MASK(so->txid),
isotp_rcv_echo, sk);
netdev_put(so->dev, &so->dev_tracker);
}
@@ -1846,13 +1846,10 @@ static int isotp_getsockopt(struct socket *sock, int level, int optname,
static void isotp_notify(struct isotp_sock *so, unsigned long msg,
struct net_device *dev)
{
struct sock *sk = &so->sk;
- if (!net_eq(dev_net(dev), sock_net(sk)))
- return;
-
if (so->dev != dev)
return;
switch (msg) {
case NETDEV_UNREGISTER:
diff --git a/net/can/raw.c b/net/can/raw.c
index 82d9c0499c95..c5596fc9aac5 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -300,13 +300,10 @@ static int raw_enable_allfilters(struct net *net, struct net_device *dev,
static void raw_notify(struct raw_sock *ro, unsigned long msg,
struct net_device *dev)
{
struct sock *sk = &ro->sk;
- if (!net_eq(dev_net(dev), sock_net(sk)))
- return;
-
if (ro->dev != dev)
return;
switch (msg) {
case NETDEV_UNREGISTER:
--
2.53.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v4 1/3] can: remove CAN filters independent from namespace
2026-09-29 15:41 ` [PATCH v4 1/3] can: remove CAN filters independent from namespace Oliver Hartkopp
@ 2026-10-03 6:00 ` netdev-bot+sashiko
0 siblings, 0 replies; 7+ messages in thread
From: netdev-bot+sashiko @ 2026-10-03 6:00 UTC (permalink / raw)
To: socketcan; +Cc: linux-can, norbert, stable, kuba
Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.
Critical: 0 · High: 2 · Medium: 1 · Low: 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).
- [High] bcm_notify() now calls lock_sock(sk) right away for every BCM
socket on the global bcm_notifier_list.
- [High] Releasing a BCM socket that races with a namespace move of its
bound device can still leave a receive filter pointing at a freed…
- [Medium] The commit message's account of the bug cannot happen.
- [Low] The commit message presents the isotp_release() change as
converting two 'missing' call sites.
--
can: remove CAN filters independent from namespace
This patch removes the net_eq(dev_net(dev), sock_net(sk)) early return
from the raw, isotp and bcm netdev notifiers. In bcm.c it adds tracked
device pointers (bo->dev, op->tx_dev) for device comparisons and takes
lock_sock() for all of bcm_notify(). In isotp_release() it passes
dev_net(so->dev) to can_rx_unregister() instead of sock_net(sk).
Link: https://lore.kernel.org/linux-can/CEA6A38A-2646-4ADA-95B4-CBAE2F301A8E@doyensec.com/
> When the devices namespace is changed the socket namespace and the device
> namespace might differ. The net_eq(dev_net(dev), sock_net(sk)) check in
> the CAN protocols netdev notifiers therefore led to skipping the required
> removal of the CAN filters from the (namespace changed) CAN devices.
[Severity: Medium]
Is this description of the bug accurate? In __dev_change_net_namespace(),
NETDEV_UNREGISTER is sent while dev_net(dev) still points to the old
namespace. The namespace is switched only afterwards:
net/core/dev.c:__dev_change_net_namespace() {
...
call_netdevice_notifiers(NETDEV_UNREGISTER, dev);
...
dev_net_set(dev, net);
...
}
Every bound device is resolved with dev_get_by_index(sock_net(sk), ...).
raw_bind() and isotp_bind() do this under rtnl_lock(). bcm_connect() and
bcm_rx_setup() do it under lock_sock(). So for every socket still on the
notifier list, the removed net_eq() check seems to have always been true
when its device got NETDEV_UNREGISTER.
In raw_notify() and isotp_notify(), the ro->dev != dev and so->dev != dev
checks that follow already made the net_eq() check redundant.
The bug that actually existed appears to be the sock_net(sk) argument in
isotp_release(). That function runs after the socket has left the
notifier list.
Could the commit message describe the real mechanism? As written, it also
justifies the bcm.c rework (bo->dev, op->tx_dev and the unconditional
lock_sock() in bcm_notify()). That rework is needed only because the
net_eq() check was removed, and it is the source of the bcm_notify()
concern below.
> In isotp.c the two missing can_rx_unregister() calling sites are converted
> to use dev_net(dev) instead of sock_net(sk) to get the correct namespace.
[Severity: Low]
This hunk looks like the actual fix for a use-after-free, but the commit
message doesn't say so. Could it describe the race and the use-after-free?
isotp_release() removes the socket from isotp_notifier_list before it
takes rtnl_lock(). If __dev_change_net_namespace() runs in that window,
its NETDEV_UNREGISTER notifier never sees the socket, and the device moves
to the new namespace.
Before this patch, isotp_release() then passed sock_net(sk) to
can_rx_unregister(), which returns early on the namespace mismatch:
net/can/af_can.c:can_rx_unregister() {
...
if (dev && !net_eq(net, dev_net(dev)))
return;
...
}
The isotp_rcv() and isotp_rcv_echo() receivers stay registered with
data == sk, and sock_put(sk) then frees the socket. can_rx_register()
holds no reference on sk, so the next matching frame on the moved device
calls isotp_rcv() on freed memory.
raw_release() already used dev_net(ro->dev) under rtnl_lock(), and bcm
goes through bcm_rx_unreg(), which uses dev_net(dev).
> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index 3d637a1e0ac1a..cd3522ec32c0a 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
[ ... ]
> @@ -1792,15 +1833,13 @@ static void bcm_notify(struct bcm_sock *bo, unsigned long msg,
> {
> struct sock *sk = &bo->sk;
> struct bcm_op *op;
> - int notify_enodev = 0;
> + int sk_err = 0;
>
> - if (!net_eq(dev_net(dev), sock_net(sk)))
> - return;
> + lock_sock(sk);
[Severity: High]
Can a user confined to their own network namespace now stall rtnl_mutex
here indefinitely?
bcm_notifier() walks every BCM socket on bcm_notifier_list for each CAN
NETDEV_DOWN or NETDEV_UNREGISTER, with rtnl_lock() held. Before this
patch, sockets in a foreign namespace returned before taking any lock,
and NETDEV_DOWN never took lock_sock(). Now lock_sock() is taken on every
BCM socket in the system for both events.
bcm_sendmsg() holds the socket lock across user copies:
bcm_sendmsg()
lock_sock(sk);
bcm_tx_setup()
memcpy_from_msg((u8 *)cf, msg, op->cfsiz);
bcm_rx_setup() has the same pattern.
An unprivileged user can put the frame data on a page whose fault they
control, for example a FUSE-backed mapping inside a user namespace. The
socket lock can then be held for as long as they want.
Any host-side "ip link set can0 down", unplug, unregister or netns move of
any CAN device would then block in bcm_notify() with rtnl_lock() held.
All link, address and route configuration and netns creation or cleanup
would stall behind it.
A similar same-namespace stall was already possible for users who can
create and delete their own vcan. This patch extends it to host devices
and to NETDEV_DOWN.
Could the bo->dev, op->tx_dev and op->rx_reg_dev comparisons be done as a
prefilter, so that lock_sock() is not taken on unrelated sockets? Even
with the rest of the series applied, only the device type check in
bcm_notifier() changes, so this lock_sock() stays unconditional.
>
> switch (msg) {
>
> case NETDEV_UNREGISTER:
> - lock_sock(sk);
[ ... ]
> @@ -1984,6 +2025,10 @@ static int bcm_release(struct socket *sock)
> if (bo->bound) {
> WRITE_ONCE(bo->bound, 0);
> bo->ifindex = 0;
> + if (bo->dev) {
> + netdev_put(bo->dev, &bo->dev_tracker);
> + bo->dev = NULL;
> + }
> }
[Severity: High]
Can bcm_release() still leave an rx filter pointing at a freed bcm_op if
it races with a namespace move of the bound device?
bcm_release() removes the socket from bcm_notifier_list and then takes
only lock_sock(sk), not rtnl_lock(). For each rx_op with op->rx_reg_dev,
teardown goes through bcm_rx_unreg():
bcm_rx_unreg()
can_rx_unregister(dev_net(dev), dev, ...)
if (dev && !net_eq(net, dev_net(dev)))
return;
Meanwhile __dev_change_net_namespace() runs under rtnl_lock(). Its
NETDEV_UNREGISTER notifier skips this socket because the socket is already
off the list. It then calls dev_net_set(dev, net).
Suppose dev_net_set() lands between the dev_net(dev) read in
bcm_rx_unreg() and the re-read in can_rx_unregister(). The receiver then
stays linked with func == bcm_rx_handler and data == op.
can_rx_unregister() returns void, so bcm_rx_unreg() still clears
op->rx_reg_dev and drops the reference. bcm_release() then frees the op:
bcm_remove_op()
call_rcu() -> bcm_free_op_work()
sock_put(op->sk);
kfree(op);
The next matching frame on the moved device would call bcm_rx_handler()
on freed memory. The window per op is narrow, but it can be retried, and
a socket with many rx_ops widens it.
isotp_release() and raw_release() avoid this by holding rtnl_lock() around
the unregister. Should bcm_release() take rtnl_lock() as well?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260929154125.3298-1-socketcan%40hartkopp.net
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v4 2/3] can: convert unreliable ARPHRD_CAN type checks to robust can_get_ml_priv()
2026-09-29 15:41 [PATCH v4 0/3] CAN netlayer fixes for stable Oliver Hartkopp
2026-09-29 15:41 ` [PATCH v4 1/3] can: remove CAN filters independent from namespace Oliver Hartkopp
@ 2026-09-29 15:41 ` Oliver Hartkopp
2026-09-29 15:41 ` [PATCH v4 3/3] can: fix unique skb identifier regression under RPS Oliver Hartkopp
2 siblings, 0 replies; 7+ messages in thread
From: Oliver Hartkopp @ 2026-09-29 15:41 UTC (permalink / raw)
To: linux-can; +Cc: Oliver Hartkopp, stable, Oleksij Rempel
Commit 4e096a18867a ("net: introduce CAN specific pointer in the struct
net_device") introduced an explicit way to assign the midlayer private
pointer (dev->ml_priv) to named users like ML_PRIV_CAN.
With this extension the CAN device specific ml_priv assignment became a
robust indicator to identify a valid CAN device, when can_get_ml_priv()
returns a valid pointer.
This has been used directly by the referenced commit in the CAN specific
j1939 and proc code but not in the other parts of the CAN subsystem.
With the TUN/TAP driver a device's ARPHRD type can be controlled by
userspace independently of its midlayer private data (ml_priv). The
TUNSETLINK ioctl allows a down TUN/TAP device to overwrite its hardware
type to become ARPHRD_CAN while dev->ml_priv remains NULL (uninitialized).
Instead of checking dev->type being the unreliable ARPHRD_CAN value convert
the missing "valid CAN devices" checks to can_get_ml_priv().
Fixes: 4e096a18867a ("net: introduce CAN specific pointer in the struct net_device")
Cc: stable@kernel.org
Cc: Oleksij Rempel <o.rempel@pengutronix.de>
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
net/can/af_can.c | 12 ++++++------
net/can/bcm.c | 7 ++++---
net/can/gw.c | 7 ++++---
net/can/isotp.c | 5 +++--
net/can/raw.c | 4 ++--
5 files changed, 19 insertions(+), 16 deletions(-)
diff --git a/net/can/af_can.c b/net/can/af_can.c
index 7bc86b176b4d..ef435f22ac93 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -224,11 +224,11 @@ int can_send(struct sk_buff *skb, int loop)
if (unlikely(skb->len > READ_ONCE(skb->dev->mtu))) {
err = -EMSGSIZE;
goto inval_skb;
}
- if (unlikely(skb->dev->type != ARPHRD_CAN)) {
+ if (unlikely(!can_get_ml_priv(skb->dev))) {
err = -EPERM;
goto inval_skb;
}
if (unlikely(!(skb->dev->flags & IFF_UP))) {
@@ -450,11 +450,11 @@ int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
struct can_dev_rcv_lists *dev_rcv_lists;
struct can_rcv_lists_stats *rcv_lists_stats = net->can.rcv_lists_stats;
/* insert new receiver (dev,canid,mask) -> (func,data) */
- if (dev && (dev->type != ARPHRD_CAN || !can_get_ml_priv(dev)))
+ if (dev && !can_get_ml_priv(dev))
return -ENODEV;
if (dev && !net_eq(net, dev_net(dev)))
return -ENODEV;
@@ -517,11 +517,11 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
struct receiver *rcv = NULL;
struct hlist_head *rcv_list;
struct can_rcv_lists_stats *rcv_lists_stats = net->can.rcv_lists_stats;
struct can_dev_rcv_lists *dev_rcv_lists;
- if (dev && dev->type != ARPHRD_CAN)
+ if (dev && !can_get_ml_priv(dev))
return;
if (dev && !net_eq(net, dev_net(dev)))
return;
@@ -685,11 +685,11 @@ static void can_receive(struct sk_buff *skb, struct net_device *dev)
}
static int can_rcv(struct sk_buff *skb, struct net_device *dev,
struct packet_type *pt, struct net_device *orig_dev)
{
- if (unlikely(dev->type != ARPHRD_CAN || !can_get_ml_priv(dev) ||
+ if (unlikely(!can_get_ml_priv(dev) ||
!can_skb_ext_find(skb) || !can_is_can_skb(skb))) {
pr_warn_once("PF_CAN: dropped non conform CAN skbuff: dev type %d, len %d\n",
dev->type, skb->len);
kfree_skb_reason(skb, SKB_DROP_REASON_CAN_RX_INVALID_FRAME);
@@ -701,11 +701,11 @@ static int can_rcv(struct sk_buff *skb, struct net_device *dev,
}
static int canfd_rcv(struct sk_buff *skb, struct net_device *dev,
struct packet_type *pt, struct net_device *orig_dev)
{
- if (unlikely(dev->type != ARPHRD_CAN || !can_get_ml_priv(dev) ||
+ if (unlikely(!can_get_ml_priv(dev) ||
!can_skb_ext_find(skb) || !can_is_canfd_skb(skb))) {
pr_warn_once("PF_CAN: dropped non conform CAN FD skbuff: dev type %d, len %d\n",
dev->type, skb->len);
kfree_skb_reason(skb, SKB_DROP_REASON_CANFD_RX_INVALID_FRAME);
@@ -717,11 +717,11 @@ static int canfd_rcv(struct sk_buff *skb, struct net_device *dev,
}
static int canxl_rcv(struct sk_buff *skb, struct net_device *dev,
struct packet_type *pt, struct net_device *orig_dev)
{
- if (unlikely(dev->type != ARPHRD_CAN || !can_get_ml_priv(dev) ||
+ if (unlikely(!can_get_ml_priv(dev) ||
!can_skb_ext_find(skb) || !can_is_canxl_skb(skb))) {
pr_warn_once("PF_CAN: dropped non conform CAN XL skbuff: dev type %d, len %d\n",
dev->type, skb->len);
kfree_skb_reason(skb, SKB_DROP_REASON_CANXL_RX_INVALID_FRAME);
diff --git a/net/can/bcm.c b/net/can/bcm.c
index cd3522ec32c0..940b917e3830 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -52,10 +52,11 @@
#include <linux/netdevice.h>
#include <linux/socket.h>
#include <linux/if_arp.h>
#include <linux/skbuff.h>
#include <linux/can.h>
+#include <linux/can/can-ml.h>
#include <linux/can/core.h>
#include <linux/can/skb.h>
#include <linux/can/bcm.h>
#include <linux/slab.h>
#include <linux/workqueue.h>
@@ -1758,11 +1759,11 @@ static int bcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
if (!dev) {
ret = -ENODEV;
goto out_release;
}
- if (dev->type != ARPHRD_CAN) {
+ if (!can_get_ml_priv(dev)) {
dev_put(dev);
ret = -ENODEV;
goto out_release;
}
@@ -1906,11 +1907,11 @@ static void bcm_notify(struct bcm_sock *bo, unsigned long msg,
static int bcm_notifier(struct notifier_block *nb, unsigned long msg,
void *ptr)
{
struct net_device *dev = netdev_notifier_info_to_dev(ptr);
- if (dev->type != ARPHRD_CAN)
+ if (!can_get_ml_priv(dev))
return NOTIFY_DONE;
if (msg != NETDEV_UNREGISTER && msg != NETDEV_DOWN)
return NOTIFY_DONE;
if (unlikely(bcm_busy_notifier)) /* Check for reentrant bug. */
return NOTIFY_DONE;
@@ -2067,11 +2068,11 @@ static int bcm_connect(struct socket *sock, struct sockaddr_unsized *uaddr, int
dev = dev_get_by_index(net, addr->can_ifindex);
if (!dev) {
ret = -ENODEV;
goto fail;
}
- if (dev->type != ARPHRD_CAN) {
+ if (!can_get_ml_priv(dev)) {
dev_put(dev);
ret = -ENODEV;
goto fail;
}
diff --git a/net/can/gw.c b/net/can/gw.c
index 0ec99f68aa45..d9912dea738b 100644
--- a/net/can/gw.c
+++ b/net/can/gw.c
@@ -50,10 +50,11 @@
#include <linux/net.h>
#include <linux/netdevice.h>
#include <linux/if_arp.h>
#include <linux/skbuff.h>
#include <linux/can.h>
+#include <linux/can/can-ml.h>
#include <linux/can/core.h>
#include <linux/can/skb.h>
#include <linux/can/gw.h>
#include <net/can.h>
#include <net/rtnetlink.h>
@@ -607,11 +608,11 @@ static int cgw_notifier(struct notifier_block *nb,
unsigned long msg, void *ptr)
{
struct net_device *dev = netdev_notifier_info_to_dev(ptr);
struct net *net = dev_net(dev);
- if (dev->type != ARPHRD_CAN)
+ if (!can_get_ml_priv(dev))
return NOTIFY_DONE;
if (msg == NETDEV_UNREGISTER) {
struct cgw_job *gwj = NULL;
struct hlist_node *nx;
@@ -1158,19 +1159,19 @@ static int cgw_create_job(struct sk_buff *skb, struct nlmsghdr *nlh,
gwj->src.dev = __dev_get_by_index(net, gwj->ccgw.src_idx);
if (!gwj->src.dev)
goto out;
- if (gwj->src.dev->type != ARPHRD_CAN)
+ if (!can_get_ml_priv(gwj->src.dev))
goto out;
gwj->dst.dev = __dev_get_by_index(net, gwj->ccgw.dst_idx);
if (!gwj->dst.dev)
goto out;
- if (gwj->dst.dev->type != ARPHRD_CAN)
+ if (!can_get_ml_priv(gwj->dst.dev))
goto out;
/* is sending the skb back to the incoming interface intended? */
if (gwj->src.dev == gwj->dst.dev &&
!(gwj->flags & CGW_FLAGS_CAN_IIF_TX_OK)) {
diff --git a/net/can/isotp.c b/net/can/isotp.c
index 0835a4758a72..d3f79efc9c1e 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -63,10 +63,11 @@
#include <linux/netdevice.h>
#include <linux/socket.h>
#include <linux/if_arp.h>
#include <linux/skbuff.h>
#include <linux/can.h>
+#include <linux/can/can-ml.h>
#include <linux/can/core.h>
#include <linux/can/skb.h>
#include <linux/can/isotp.h>
#include <linux/slab.h>
#include <net/can.h>
@@ -1604,11 +1605,11 @@ static int isotp_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int l
dev = dev_get_by_index(net, addr->can_ifindex);
if (!dev) {
err = -ENODEV;
goto out;
}
- if (dev->type != ARPHRD_CAN) {
+ if (!can_get_ml_priv(dev)) {
err = -ENODEV;
goto out_put_dev;
}
if (READ_ONCE(dev->mtu) < so->ll.mtu) {
err = -EINVAL;
@@ -1888,11 +1889,11 @@ static void isotp_notify(struct isotp_sock *so, unsigned long msg,
static int isotp_notifier(struct notifier_block *nb, unsigned long msg,
void *ptr)
{
struct net_device *dev = netdev_notifier_info_to_dev(ptr);
- if (dev->type != ARPHRD_CAN)
+ if (!can_get_ml_priv(dev))
return NOTIFY_DONE;
if (msg != NETDEV_UNREGISTER && msg != NETDEV_DOWN)
return NOTIFY_DONE;
if (unlikely(isotp_busy_notifier)) /* Check for reentrant bug. */
return NOTIFY_DONE;
diff --git a/net/can/raw.c b/net/can/raw.c
index c5596fc9aac5..7c48ff36e6cd 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -339,11 +339,11 @@ static void raw_notify(struct raw_sock *ro, unsigned long msg,
static int raw_notifier(struct notifier_block *nb, unsigned long msg,
void *ptr)
{
struct net_device *dev = netdev_notifier_info_to_dev(ptr);
- if (dev->type != ARPHRD_CAN)
+ if (!can_get_ml_priv(dev))
return NOTIFY_DONE;
if (msg != NETDEV_UNREGISTER && msg != NETDEV_DOWN)
return NOTIFY_DONE;
if (unlikely(raw_busy_notifier)) /* Check for reentrant bug. */
return NOTIFY_DONE;
@@ -482,11 +482,11 @@ static int raw_bind(struct socket *sock, struct sockaddr_unsized *uaddr, int len
dev = dev_get_by_index(sock_net(sk), addr->can_ifindex);
if (!dev) {
err = -ENODEV;
goto out;
}
- if (dev->type != ARPHRD_CAN) {
+ if (!can_get_ml_priv(dev)) {
err = -ENODEV;
goto out_put_dev;
}
if (!(dev->flags & IFF_UP))
--
2.53.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH v4 3/3] can: fix unique skb identifier regression under RPS
2026-09-29 15:41 [PATCH v4 0/3] CAN netlayer fixes for stable Oliver Hartkopp
2026-09-29 15:41 ` [PATCH v4 1/3] can: remove CAN filters independent from namespace Oliver Hartkopp
2026-09-29 15:41 ` [PATCH v4 2/3] can: convert unreliable ARPHRD_CAN type checks to robust can_get_ml_priv() Oliver Hartkopp
@ 2026-09-29 15:41 ` Oliver Hartkopp
2026-09-29 16:09 ` sashiko-bot
2026-10-03 6:00 ` netdev-bot+sashiko
2 siblings, 2 replies; 7+ messages in thread
From: Oliver Hartkopp @ 2026-09-29 15:41 UTC (permalink / raw)
To: linux-can; +Cc: Oliver Hartkopp, Joerg Willmann, stable
Commit d4fb6514ff8e ("can: use skb hash instead of private variable in
headroom") moved the per-skb unique identifier used for raw_rcv()
duplicate detection into skb->hash.
With RPS enabled, get_rps_cpu() calls skb_get_hash() before the frame
reaches the CAN subsystem. Since CAN skbs have no L3/L4 headers, the
flow dissector assigns every CAN frame the same non-zero software
hash. can_set_skb_uid() only generated a new identifier when
skb->hash was 0, so it kept this constant hash. When the SLAB
allocator later reused the same skb address, raw_rcv() mistook the
next legitimate frame for a duplicate and dropped it.
Fix this by storing the CAN UID in the CAN skb extension
(struct can_skb_ext::can_skb_uid) instead of skb->hash, decoupling it
from any hash the network stack may compute. can_set_skb_uid() keeps
its "assign only if unset" behaviour, which preserves UIDs set before
transmission (e.g. by isotp to identify echo frames) across the local
loopback path.
Frames whose extension is still shared with another clone (e.g. via
tc mirred or netem duplicate) are given a private extension copy
before the UID is assigned, so that independently received clones
never end up with the same UID.
can-gw and vxcan additionally clear the UID of forwarded/duplicated
frames so each newly routed frame gets its own unique identifier.
Fixes: d4fb6514ff8e ("can: use skb hash instead of private variable in headroom")
Reported-by: Joerg Willmann <joe@clnt.de>
Closes: https://lore.kernel.org/linux-can/2859AD3D-C805-41A0-9036-C5E8EE152419@clnt.de/
Cc: stable@vger.kernel.org
Tested-by: Oliver Hartkopp <socketcan@hartkopp.net>
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
v2: We need a non-zero UID for the cfecho checks in isotp.c
So restore the former while (!(skb->hash)) statement
v3: omit the problematic use of skb->hash reported by sashiko bot
v4: address tc mirred or netem duplicate clones suggested by sashiko bot
---
drivers/net/can/vxcan.c | 3 +++
include/linux/can/core.h | 3 ++-
include/linux/can/skb.h | 4 ++-
include/net/can.h | 2 ++
net/can/af_can.c | 57 ++++++++++++++++++++++++++++++----------
net/can/gw.c | 3 +++
net/can/isotp.c | 16 ++++++-----
net/can/raw.c | 10 ++++---
8 files changed, 73 insertions(+), 25 deletions(-)
diff --git a/drivers/net/can/vxcan.c b/drivers/net/can/vxcan.c
index 9e2e25d02471..51148a81d1d9 100644
--- a/drivers/net/can/vxcan.c
+++ b/drivers/net/can/vxcan.c
@@ -77,10 +77,13 @@ static netdev_tx_t vxcan_xmit(struct sk_buff *oskb, struct net_device *dev)
goto out_unlock;
}
/* reset CAN GW hop counter */
csx->can_gw_hops = 0;
+ /* start with new CAN skb UID in the other namespace */
+ csx->can_skb_uid = 0;
+
skb->pkt_type = PACKET_BROADCAST;
skb->dev = peer;
skb->ip_summed = CHECKSUM_UNNECESSARY;
len = can_skb_get_data_len(skb);
diff --git a/include/linux/can/core.h b/include/linux/can/core.h
index 3287232e3cad..2de74c2b78b6 100644
--- a/include/linux/can/core.h
+++ b/include/linux/can/core.h
@@ -15,10 +15,11 @@
#define _CAN_CORE_H
#include <linux/can.h>
#include <linux/skbuff.h>
#include <linux/netdevice.h>
+#include <net/can.h>
#define DNAME(dev) ((dev) ? (dev)->name : "any")
/**
* struct can_proto - CAN protocol structure
@@ -56,9 +57,9 @@ extern void can_rx_unregister(struct net *net, struct net_device *dev,
canid_t can_id, canid_t mask,
void (*func)(struct sk_buff *, void *),
void *data);
extern int can_send(struct sk_buff *skb, int loop);
-void can_set_skb_uid(struct sk_buff *skb);
+void can_set_skb_uid(struct can_skb_ext *csx);
void can_sock_destruct(struct sock *sk);
#endif /* !_CAN_CORE_H */
diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
index a70a02967071..5d27843862fc 100644
--- a/include/linux/can/skb.h
+++ b/include/linux/can/skb.h
@@ -41,12 +41,14 @@ bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb);
static inline struct can_skb_ext *can_skb_ext_add(struct sk_buff *skb)
{
struct can_skb_ext *csx = skb_ext_add(skb, SKB_EXT_CAN);
/* skb_ext_add() returns uninitialized space */
- if (csx)
+ if (csx) {
csx->can_gw_hops = 0;
+ csx->can_skb_uid = 0;
+ }
return csx;
}
static inline struct can_skb_ext *can_skb_ext_find(struct sk_buff *skb)
diff --git a/include/net/can.h b/include/net/can.h
index 6db9e826f0e0..2b8af2598732 100644
--- a/include/net/can.h
+++ b/include/net/can.h
@@ -15,14 +15,16 @@
* struct can_skb_ext - skb extensions for CAN specific content
* @can_iif: ifindex of the first interface the CAN frame appeared on
* @can_framelen: cached echo CAN frame length for bql
* @can_gw_hops: can-gw CAN frame time-to-live counter
* @can_ext_flags: CAN skb extensions flags
+ * @can_skb_uid: CAN skb UID for raw_rcv and isotp echo handling
*/
struct can_skb_ext {
int can_iif;
u16 can_framelen;
u8 can_gw_hops;
u8 can_ext_flags;
+ u32 can_skb_uid;
};
#endif /* _NET_CAN_H */
diff --git a/net/can/af_can.c b/net/can/af_can.c
index ef435f22ac93..4aa233d7e8a1 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -639,17 +639,14 @@ static int can_rcv_filter(struct can_dev_rcv_lists *dev_rcv_lists, struct sk_buf
}
return matches;
}
-void can_set_skb_uid(struct sk_buff *skb)
+void can_set_skb_uid(struct can_skb_ext *csx)
{
- /* create non-zero unique skb identifier together with *skb */
- while (!(skb->hash))
- skb->hash = atomic_inc_return(&skbcounter);
-
- skb->sw_hash = 1;
+ while (!(csx->can_skb_uid))
+ csx->can_skb_uid = atomic_inc_return(&skbcounter);
}
EXPORT_SYMBOL(can_set_skb_uid);
static void can_receive(struct sk_buff *skb, struct net_device *dev)
{
@@ -660,12 +657,10 @@ static void can_receive(struct sk_buff *skb, struct net_device *dev)
/* update statistics */
atomic_long_inc(&pkg_stats->rx_frames);
atomic_long_inc(&pkg_stats->rx_frames_delta);
- can_set_skb_uid(skb);
-
rcu_read_lock();
/* deliver the packet to sockets listening on all devices */
matches = can_rcv_filter(net->can.rx_alldev_list, skb);
@@ -682,54 +677,88 @@ static void can_receive(struct sk_buff *skb, struct net_device *dev)
atomic_long_inc(&pkg_stats->matches);
atomic_long_inc(&pkg_stats->matches_delta);
}
}
+static struct can_skb_ext *can_get_unshared_skb_ext(struct sk_buff *skb,
+ struct can_skb_ext *csx)
+{
+ /* create unshared CAN skb_extension for netem/mirred skb clones */
+ if (unlikely(skb_cloned(skb)))
+ return skb_ext_add(skb, SKB_EXT_CAN);
+ else
+ return csx;
+}
+
static int can_rcv(struct sk_buff *skb, struct net_device *dev,
struct packet_type *pt, struct net_device *orig_dev)
{
- if (unlikely(!can_get_ml_priv(dev) ||
- !can_skb_ext_find(skb) || !can_is_can_skb(skb))) {
+ struct can_skb_ext *csx = can_skb_ext_find(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);
kfree_skb_reason(skb, SKB_DROP_REASON_CAN_RX_INVALID_FRAME);
return NET_RX_DROP;
}
+ csx = can_get_unshared_skb_ext(skb, csx);
+ if (unlikely(!csx)) {
+ kfree_skb_reason(skb, SKB_DROP_REASON_NOMEM);
+ return NET_RX_DROP;
+ }
+
+ can_set_skb_uid(csx);
can_receive(skb, dev);
return NET_RX_SUCCESS;
}
static int canfd_rcv(struct sk_buff *skb, struct net_device *dev,
struct packet_type *pt, struct net_device *orig_dev)
{
- if (unlikely(!can_get_ml_priv(dev) ||
- !can_skb_ext_find(skb) || !can_is_canfd_skb(skb))) {
+ struct can_skb_ext *csx = can_skb_ext_find(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);
kfree_skb_reason(skb, SKB_DROP_REASON_CANFD_RX_INVALID_FRAME);
return NET_RX_DROP;
}
+ csx = can_get_unshared_skb_ext(skb, csx);
+ if (unlikely(!csx)) {
+ kfree_skb_reason(skb, SKB_DROP_REASON_NOMEM);
+ return NET_RX_DROP;
+ }
+
+ can_set_skb_uid(csx);
can_receive(skb, dev);
return NET_RX_SUCCESS;
}
static int canxl_rcv(struct sk_buff *skb, struct net_device *dev,
struct packet_type *pt, struct net_device *orig_dev)
{
- if (unlikely(!can_get_ml_priv(dev) ||
- !can_skb_ext_find(skb) || !can_is_canxl_skb(skb))) {
+ struct can_skb_ext *csx = can_skb_ext_find(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);
kfree_skb_reason(skb, SKB_DROP_REASON_CANXL_RX_INVALID_FRAME);
return NET_RX_DROP;
}
+ csx = can_get_unshared_skb_ext(skb, csx);
+ if (unlikely(!csx)) {
+ kfree_skb_reason(skb, SKB_DROP_REASON_NOMEM);
+ return NET_RX_DROP;
+ }
+
+ can_set_skb_uid(csx);
can_receive(skb, dev);
return NET_RX_SUCCESS;
}
/* af_can protocol functions */
diff --git a/net/can/gw.c b/net/can/gw.c
index d9912dea738b..54bb5bd3242a 100644
--- a/net/can/gw.c
+++ b/net/can/gw.c
@@ -526,10 +526,13 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data)
}
/* put the incremented hop counter in the cloned skb */
ncsx->can_gw_hops = csx->can_gw_hops + 1;
+ /* force a new CAN UID generation for the routed frame */
+ ncsx->can_skb_uid = 0;
+
/* first processing of this CAN frame -> adjust to private hop limit */
if (gwj->limit_hops && ncsx->can_gw_hops == 1)
ncsx->can_gw_hops = max_hops - gwj->limit_hops + 1;
nskb->dev = gwj->dst.dev;
diff --git a/net/can/isotp.c b/net/can/isotp.c
index d3f79efc9c1e..860c5221e299 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -890,11 +890,11 @@ static void isotp_send_cframe(struct isotp_sock *so)
}
csx->can_iif = dev->ifindex;
/* set uid in tx skb to identify CF echo frames */
- can_set_skb_uid(skb);
+ can_set_skb_uid(csx);
cf = (struct canfd_frame *)skb->data;
skb_put_zero(skb, so->ll.mtu);
/* create consecutive frame */
@@ -916,11 +916,11 @@ static void isotp_send_cframe(struct isotp_sock *so)
old_cfecho = READ_ONCE(so->cfecho);
if (old_cfecho)
pr_notice_once("can-isotp: cfecho is %08X != 0\n", old_cfecho);
/* set consecutive frame echo tag */
- WRITE_ONCE(so->cfecho, skb->hash);
+ WRITE_ONCE(so->cfecho, csx->can_skb_uid);
/* send frame with local echo enabled */
can_send_ret = can_send(skb, 1);
if (can_send_ret) {
pr_notice_once("can-isotp: %s: can_send_ret %pe\n",
@@ -968,22 +968,26 @@ static void isotp_create_fframe(struct canfd_frame *cf, struct isotp_sock *so,
static void isotp_rcv_echo(struct sk_buff *skb, void *data)
{
struct sock *sk = (struct sock *)data;
struct isotp_sock *so = isotp_sk(sk);
+ struct can_skb_ext *csx = can_skb_ext_find(skb);
/* only handle my own local echo CF/SF skb's (no FF!) */
if (skb->sk != sk)
return;
+ if (WARN_ON_ONCE(!csx))
+ return;
+
/* unlike isotp_rcv_fc()/isotp_rcv_cf(), not already under so->rx_lock
* (no isotp_rcv() caller here), so take it ourselves
*/
spin_lock(&so->rx_lock);
/* so->cfecho may since belong to a new transfer; recheck under lock */
- if (READ_ONCE(so->cfecho) != skb->hash)
+ if (READ_ONCE(so->cfecho) != csx->can_skb_uid)
goto out_unlock;
/* cancel local echo timeout */
hrtimer_cancel(&so->echotimer);
@@ -1221,11 +1225,11 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
}
csx->can_iif = dev->ifindex;
/* set uid in tx skb to identify CF echo frames */
- can_set_skb_uid(skb);
+ can_set_skb_uid(csx);
so->tx.len = size;
so->tx.idx = 0;
cf = (struct canfd_frame *)skb->data;
@@ -1260,11 +1264,11 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
cf->data[SF_PCI_SZ4 + ae] = size;
else
cf->data[ae] |= size;
/* set CF echo tag for isotp_rcv_echo() (SF-mode) */
- WRITE_ONCE(so->cfecho, skb->hash);
+ WRITE_ONCE(so->cfecho, csx->can_skb_uid);
} else {
/* send first frame */
isotp_create_fframe(cf, so, ae);
@@ -1277,11 +1281,11 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
/* disable wait for FCs due to activated block size */
so->txfc.bs = 0;
/* set CF echo tag for isotp_rcv_echo() (CF-mode) */
- WRITE_ONCE(so->cfecho, skb->hash);
+ WRITE_ONCE(so->cfecho, csx->can_skb_uid);
} else {
/* standard flow control check */
new_state = ISOTP_WAIT_FIRST_FC;
/* start timeout for FC */
diff --git a/net/can/raw.c b/net/can/raw.c
index 7c48ff36e6cd..7873bfc584ef 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -75,11 +75,11 @@ MODULE_ALIAS("can-proto-1");
* storing the single filter in dfilter, to avoid using dynamic memory.
*/
struct uniqframe {
const struct sk_buff *skb;
- u32 hash;
+ u32 can_skb_uid;
unsigned int join_rx_count;
};
struct raw_sock {
struct sock sk;
@@ -131,16 +131,20 @@ static void raw_rcv(struct sk_buff *oskb, void *data)
struct sock *sk = (struct sock *)data;
struct raw_sock *ro = raw_sk(sk);
enum skb_drop_reason reason;
struct sockaddr_can *addr;
struct sk_buff *skb;
+ struct can_skb_ext *csx = can_skb_ext_find(oskb);
unsigned int *pflags;
/* check the received tx sock reference */
if (!ro->recv_own_msgs && oskb->sk == sk)
return;
+ if (WARN_ON_ONCE(!csx))
+ return;
+
/* make sure to not pass oversized frames to the socket */
if (!ro->fd_frames && can_is_canfd_skb(oskb))
return;
if (can_is_canxl_skb(oskb)) {
@@ -163,21 +167,21 @@ static void raw_rcv(struct sk_buff *oskb, void *data)
}
}
/* eliminate multiple filter matches for the same skb */
if (this_cpu_ptr(ro->uniq)->skb == oskb &&
- this_cpu_ptr(ro->uniq)->hash == oskb->hash) {
+ this_cpu_ptr(ro->uniq)->can_skb_uid == csx->can_skb_uid) {
if (!ro->join_filters)
return;
this_cpu_inc(ro->uniq->join_rx_count);
/* drop frame until all enabled filters matched */
if (this_cpu_ptr(ro->uniq)->join_rx_count < ro->count)
return;
} else {
this_cpu_ptr(ro->uniq)->skb = oskb;
- this_cpu_ptr(ro->uniq)->hash = oskb->hash;
+ this_cpu_ptr(ro->uniq)->can_skb_uid = csx->can_skb_uid;
this_cpu_ptr(ro->uniq)->join_rx_count = 1;
/* drop first frame to check all enabled filters? */
if (ro->join_filters && ro->count > 1)
return;
}
--
2.53.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v4 3/3] can: fix unique skb identifier regression under RPS
2026-09-29 15:41 ` [PATCH v4 3/3] can: fix unique skb identifier regression under RPS Oliver Hartkopp
@ 2026-09-29 16:09 ` sashiko-bot
2026-10-03 6:00 ` netdev-bot+sashiko
1 sibling, 0 replies; 7+ messages in thread
From: sashiko-bot @ 2026-09-29 16:09 UTC (permalink / raw)
To: Oliver Hartkopp
Cc: Marc Kleine-Budde, linux-can, Oliver Hartkopp, Vincent Mailhol,
Oleksij Rempel
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] can: data race and extension corruption due to incorrect skb_cloned() check
--
commit 1ddbc558b8096bdaecb507a831137177beb7d0ad
Author: Oliver Hartkopp <socketcan@hartkopp.net>
can: fix unique skb identifier regression under RPS
This commit moves the per-skb unique identifier used for duplicate detection
into a CAN skb extension instead of using skb->hash. It aims to fix an issue
where RPS assigned a non-zero hash to CAN frames, preventing a unique ID from
being generated and causing legitimate frames to be dropped as duplicates.
Link: https://lore.kernel.org/linux-can/2859AD3D-C805-41A0-9036-C5E8EE152419@clnt.de/
> diff --git a/net/can/af_can.c b/net/can/af_can.c
> index ef435f22ac933..4aa233d7e8a1d 100644
> --- a/net/can/af_can.c
> +++ b/net/can/af_can.c
[ ... ]
> -void can_set_skb_uid(struct sk_buff *skb)
> +void can_set_skb_uid(struct can_skb_ext *csx)
> {
> - /* create non-zero unique skb identifier together with *skb */
> - while (!(skb->hash))
> - skb->hash = atomic_inc_return(&skbcounter);
> -
> - skb->sw_hash = 1;
> + while (!(csx->can_skb_uid))
> + csx->can_skb_uid = atomic_inc_return(&skbcounter);
> }
[Severity: High]
If the extension block passed here is shared with another SKB, will this write
to csx->can_skb_uid cause a data race and corrupt the clone's UID?
[ ... ]
> +static struct can_skb_ext *can_get_unshared_skb_ext(struct sk_buff *skb,
> + struct can_skb_ext *csx)
> +{
> + /* create unshared CAN skb_extension for netem/mirred skb clones */
> + if (unlikely(skb_cloned(skb)))
> + return skb_ext_add(skb, SKB_EXT_CAN);
> + else
> + return csx;
> +}
[Severity: High]
Is skb_cloned() the correct check to determine if the extension block is shared?
If a CAN SKB is cloned (for example, via tc mirred) and its data buffer is
later unshared via skb_unshare() or pskb_expand_head(), the data buffer is
copied but the extension block remains shared because __skb_ext_copy() only
increments its refcount.
In this scenario, skb_cloned(skb) returns false because the data buffer itself
is no longer shared. Returning csx directly here would bypass the COW refcount
logic inside skb_ext_add() and pass a shared extension block to
can_set_skb_uid().
This seems like it could cause raw_rcv() to mistakenly identify legitimate
frames as duplicates and silently drop them. Should this rely directly on the
refcount handling within skb_ext_add() instead?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260929154125.3298-1-socketcan@hartkopp.net?part=3
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v4 3/3] can: fix unique skb identifier regression under RPS
2026-09-29 15:41 ` [PATCH v4 3/3] can: fix unique skb identifier regression under RPS Oliver Hartkopp
2026-09-29 16:09 ` sashiko-bot
@ 2026-10-03 6:00 ` netdev-bot+sashiko
1 sibling, 0 replies; 7+ messages in thread
From: netdev-bot+sashiko @ 2026-10-03 6:00 UTC (permalink / raw)
To: socketcan; +Cc: linux-can, norbert, stable, kuba
Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.
Critical: 0 · High: 1 · Medium: 1 · Low: 0
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [High] In net/can/af_can.c, can_get_unshared_skb_ext() uses the wrong
test to decide whether the CAN skb extension is shared.
- [Medium] A UID set before transmission survives the new extension
unsharing, so duplicated or mirrored copies of such frames still share…
--
can: fix unique skb identifier regression under RPS
This patch moves the CAN per-skb unique identifier used for raw_rcv()
duplicate detection from skb->hash into a new can_skb_uid field in
struct can_skb_ext. The UID is now assigned in can_rcv(), canfd_rcv()
and canxl_rcv() after the CAN skb extension is made private for cloned
skbs, and can-gw and vxcan reset it for forwarded frames.
Closes: https://lore.kernel.org/linux-can/2859AD3D-C805-41A0-9036-C5E8EE152419@clnt.de/
> diff --git a/net/can/af_can.c b/net/can/af_can.c
> index ef435f22ac933..4aa233d7e8a1d 100644
> --- a/net/can/af_can.c
> +++ b/net/can/af_can.c
[ ... ]
> @@ -684,11 +679,22 @@ static void can_receive(struct sk_buff *skb, struct net_device *dev)
> }
> }
>
> +static struct can_skb_ext *can_get_unshared_skb_ext(struct sk_buff *skb,
> + struct can_skb_ext *csx)
> +{
> + /* create unshared CAN skb_extension for netem/mirred skb clones */
> + if (unlikely(skb_cloned(skb)))
[Severity: High]
Is skb_cloned() the right test for whether the extension is shared?
skb_cloned() only tells you whether the packet data is shared through
shinfo->dataref. The skb_ext has its own refcount.
__copy_skb_header() shares the extension through __skb_ext_copy() for
copies as well as clones:
include/linux/skbuff.h:__skb_ext_copy() {
...
refcount_inc(&ext->refcnt);
dst->extensions = ext;
...
}
So results of skb_copy(), pskb_copy() and skb_unshare() have private
data but still share the CAN extension with their source.
One way to reach this is netem with duplicate and corrupt on a CAN
device:
net/sched/sch_netem.c:netem_enqueue() {
...
skb2 = skb_clone(skb, GFP_ATOMIC);
...
skb = skb_unshare(skb, GFP_ATOMIC);
...
}
After this, skb2 has dataref == 1 and the copy has private data, so
skb_cloned() is false for both. They still point to the same skb_ext
with refcnt == 2.
Echo skbs from can_create_echo_skb() on IFF_ECHO devices (e.g. vcan
echo=1) can end up in the same state once the parent is consumed.
In that case can_get_unshared_skb_ext() returns the shared csx, and
can_set_skb_uid() writes can_skb_uid in place:
while (!(csx->can_skb_uid))
csx->can_skb_uid = atomic_inc_return(&skbcounter);
The second frame then sees a non-zero UID and keeps it. Doesn't this
contradict the commit message statement that "independently received
clones never end up with the same UID"?
With a shared UID, raw_rcv() can again drop a legitimate frame when the
slab reuses the skb address on the same CPU:
if (this_cpu_ptr(ro->uniq)->skb == oskb &&
this_cpu_ptr(ro->uniq)->can_skb_uid == csx->can_skb_uid) {
If the two frames go through can_rcv() on different CPUs, the
check-then-store in can_set_skb_uid() is also an unsynchronized write
into memory that both skbs share. The UID could then change while
raw_rcv() on the other CPU is between filter matches. Could that break
the join_filters counting?
skb_ext_add() already does the refcount-based check in
skb_ext_maybe_cow():
if (refcount_read(&old->refcnt) == 1)
return old;
Would it be enough to call skb_ext_add() unconditionally here, or to
check the extension refcount, instead of gating on skb_cloned()?
> + return skb_ext_add(skb, SKB_EXT_CAN);
[Severity: Medium]
Does the private copy really get its own UID if the frame was already
tagged before transmission?
isotp_sendmsg() and isotp_send_cframe() call can_set_skb_uid(csx) on
the tx skb before can_send(). So the UID is already non-zero (say X)
by the time netem duplicate (skb_clone) or tc mirred mirror/redirect
copies the skb. Neither path resets the UID, unlike can-gw and vxcan
in this patch.
When such a clone reaches can_rcv() still cloned, skb_ext_add() goes
through skb_ext_maybe_cow(). That function copies the whole extension,
X included:
memcpy(new, old, old->chunks * SKB_EXT_ALIGN_VALUE);
can_set_skb_uid() then leaves the non-zero X in place, so the private
copy keeps the same identity as the original.
Once can_receive() consumes the first copy, a later copy (the echo
clone from can_create_echo_skb(), or the mirrored frame) can be
allocated at the freed slab address. On the same CPU, a raw socket
that did not send the frame, such as candump, would then match both
uniq->skb and uniq->can_skb_uid in raw_rcv() and drop the frame as a
duplicate.
The commit message only explains why the UID is kept across the local
loopback path. Should duplicated or mirrored copies of pre-tagged
frames also get a fresh UID, so that the "never end up with the same
UID" claim holds?
> + else
> + return csx;
> +}
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260929154125.3298-1-socketcan%40hartkopp.net
^ permalink raw reply [flat|nested] 7+ messages in thread