* [PATCH v6 1/9] can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF
2026-07-08 9:22 [PATCH v6 0/9] can: bcm: collected fixes - final Oliver Hartkopp via B4 Relay
@ 2026-07-08 9:22 ` Oliver Hartkopp via B4 Relay
2026-07-08 9:41 ` sashiko-bot
2026-07-08 9:22 ` [PATCH v6 2/9] can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure Oliver Hartkopp via B4 Relay
` (7 subsequent siblings)
8 siblings, 1 reply; 17+ messages in thread
From: Oliver Hartkopp via B4 Relay @ 2026-07-08 9:22 UTC (permalink / raw)
To: linux-can; +Cc: Oliver Hartkopp, Marc Kleine-Budde, Lee Jones, Feng Xue
From: Lee Jones <lee@kernel.org>
Commit f1b4e32aca08 ("can: bcm: use call_rcu() instead of costly
synchronize_rcu()") replaced synchronize_rcu() in bcm_delete_rx_op()
with call_rcu() and introduced the RX_NO_AUTOTIMER flag.
However, this flag check was omitted for thrtimer in the packet rx
fast-path. During BCM RX operation teardown, a concurrent RCU reader
(bcm_rx_handler) can race and re-arm thrtimer via
bcm_rx_update_and_send() after call_rcu() has been scheduled. Once
the RCU grace period elapses, bcm_op is freed. The subsequently
firing thrtimer then dereferences the deallocated op, causing a UAF.
Adding flag checks to the rx fast-path (bcm_rx_update_and_send) does not
fully close the TOCTOU race and introduces latency for every CAN frame.
Conversely, calling hrtimer_cancel() directly inside the RCU callback
(softirq context) is fatal as hrtimer_cancel() can sleep, triggering
a "scheduling while atomic" panic.
Resolve this by deferring the timer cancellation and memory free to a
dedicated unbound workqueue (bcm_wq). The RCU callback now queues a
work item to bcm_wq, which safely cancels both timers and deallocates
memory in sleepable process context. A dedicated workqueue is used to
prevent system-wide WQ saturation and is cleanly flushed/destroyed
on module unload to avoid rmmod page faults.
Since the deferred work can now outlive the calling context by an
unbounded amount, also take a reference on op->sk when it is assigned
and drop it only once the deferred work has cancelled both timers, so a
socket can no longer be freed out from under a still-armed timer whose
callback (bcm_send_to_user()) dereferences op->sk.
Fixes: f1b4e32aca08 ("can: bcm: use call_rcu() instead of costly synchronize_rcu()")
Tested-by: Feng Xue <feng.xue@outlook.com>
Tested-by: Oliver Hartkopp <socketcan@hartkopp.net>
Signed-off-by: Lee Jones <lee@kernel.org>
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
net/can/bcm.c | 37 ++++++++++++++++++++++++++++++++++---
1 file changed, 34 insertions(+), 3 deletions(-)
diff --git a/net/can/bcm.c b/net/can/bcm.c
index a4bef2c48a55..bdf53241bd7b 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -56,10 +56,11 @@
#include <linux/can.h>
#include <linux/can/core.h>
#include <linux/can/skb.h>
#include <linux/can/bcm.h>
#include <linux/slab.h>
+#include <linux/workqueue.h>
#include <linux/spinlock.h>
#include <net/can.h>
#include <net/sock.h>
#include <net/net_namespace.h>
@@ -90,10 +91,12 @@ MODULE_LICENSE("Dual BSD/GPL");
MODULE_AUTHOR("Oliver Hartkopp <oliver.hartkopp@volkswagen.de>");
MODULE_ALIAS("can-proto-2");
#define BCM_MIN_NAMELEN CAN_REQUIRED_SIZE(struct sockaddr_can, can_ifindex)
+static struct workqueue_struct *bcm_wq;
+
/*
* easy access to the first 64 bit of can(fd)_frame payload. cp->data is
* 64 bit aligned so the offset has to be multiples of 8 which is ensured
* by the only callers in bcm_rx_cmp_to_index() bcm_rx_handler().
*/
@@ -103,10 +106,11 @@ static inline u64 get_u64(const struct canfd_frame *cp, int offset)
}
struct bcm_op {
struct list_head list;
struct rcu_head rcu;
+ struct work_struct work;
int ifindex;
canid_t can_id;
u32 flags;
unsigned long frames_abs, frames_filtered;
struct bcm_timeval ival1, ival2;
@@ -791,23 +795,40 @@ static struct bcm_op *bcm_find_op(struct list_head *ops,
}
return NULL;
}
-static void bcm_free_op_rcu(struct rcu_head *rcu_head)
+static void bcm_free_op_work(struct work_struct *work)
{
- struct bcm_op *op = container_of(rcu_head, struct bcm_op, rcu);
+ struct bcm_op *op = container_of(work, struct bcm_op, work);
+
+ hrtimer_cancel(&op->timer);
+ hrtimer_cancel(&op->thrtimer);
if ((op->frames) && (op->frames != &op->sframe))
kfree(op->frames);
if ((op->last_frames) && (op->last_frames != &op->last_sframe))
kfree(op->last_frames);
+ /* 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);
+
kfree(op);
}
+static void bcm_free_op_rcu(struct rcu_head *rcu_head)
+{
+ struct bcm_op *op = container_of(rcu_head, struct bcm_op, rcu);
+
+ INIT_WORK(&op->work, bcm_free_op_work);
+ queue_work(bcm_wq, &op->work);
+}
+
static void bcm_remove_op(struct bcm_op *op)
{
hrtimer_cancel(&op->timer);
hrtimer_cancel(&op->thrtimer);
@@ -1058,10 +1079,11 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
/* tx_ops never compare with previous received messages */
op->last_frames = NULL;
/* bcm_can_tx / bcm_tx_timeout_handler needs this */
op->sk = sk;
+ sock_hold(sk);
op->ifindex = ifindex;
/* initialize uninitialized (kzalloc) structure */
hrtimer_setup(&op->timer, bcm_tx_timeout_handler, CLOCK_MONOTONIC,
HRTIMER_MODE_REL_SOFT);
@@ -1219,10 +1241,11 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
}
}
/* bcm_can_tx / bcm_tx_timeout_handler needs this */
op->sk = sk;
+ sock_hold(sk);
op->ifindex = ifindex;
/* ifindex for timeout events w/o previous frame reception */
op->rx_ifindex = ifindex;
@@ -1837,15 +1860,19 @@ static struct notifier_block canbcm_notifier = {
static int __init bcm_module_init(void)
{
int err;
+ bcm_wq = alloc_workqueue("can-bcm-wq", WQ_UNBOUND, 0);
+ if (!bcm_wq)
+ return -ENOMEM;
+
pr_info("can: broadcast manager protocol\n");
err = register_pernet_subsys(&canbcm_pernet_ops);
if (err)
- return err;
+ goto register_pernet_failed;
err = register_netdevice_notifier(&canbcm_notifier);
if (err)
goto register_notifier_failed;
@@ -1859,17 +1886,21 @@ static int __init bcm_module_init(void)
register_proto_failed:
unregister_netdevice_notifier(&canbcm_notifier);
register_notifier_failed:
unregister_pernet_subsys(&canbcm_pernet_ops);
+register_pernet_failed:
+ destroy_workqueue(bcm_wq);
return err;
}
static void __exit bcm_module_exit(void)
{
can_proto_unregister(&bcm_can_proto);
unregister_netdevice_notifier(&canbcm_notifier);
unregister_pernet_subsys(&canbcm_pernet_ops);
+ rcu_barrier();
+ destroy_workqueue(bcm_wq);
}
module_init(bcm_module_init);
module_exit(bcm_module_exit);
--
2.53.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v6 1/9] can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF
2026-07-08 9:22 ` [PATCH v6 1/9] can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF Oliver Hartkopp via B4 Relay
@ 2026-07-08 9:41 ` sashiko-bot
0 siblings, 0 replies; 17+ messages in thread
From: sashiko-bot @ 2026-07-08 9:41 UTC (permalink / raw)
To: Oliver Hartkopp
Cc: Marc Kleine-Budde, Vincent Mailhol, Oleksij Rempel, linux-can,
Oliver Hartkopp
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [High] Unregistering the CAN protocol before flushing the workqueue causes a kernel WARNING and leaks the slab cache during module unload.
Pre-existing issues:
- [High] Unsafe lockless iteration of `tx_ops` and `rx_ops` lists without RCU wrappers leads to data races and potential kernel crashes.
- [High] Unprotected data races during CAN frame configuration updates against concurrent timers and softirq handlers.
--
--- Patch [1]: [PATCH v6 1/9] can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF ---
commit 31beeaafb4b7e3f7496ed0979679240bbb843e27
Author: Lee Jones <lee@kernel.org>
Subject: can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF
This commit replaces immediate call_rcu() with workqueue deferral for BCM
operations to safely cancel hrtimers that can sleep, fixing a use-after-free.
It adds a reference count on op->sk to prevent the socket from disappearing
before the workqueue executes.
> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index a4bef2c48a559..bdf53241bd7b3 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
[ ... ]
> @@ -1060,6 +1081,7 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>
> /* bcm_can_tx / bcm_tx_timeout_handler needs this */
> op->sk = sk;
> + sock_hold(sk);
> op->ifindex = ifindex;
[Severity: High]
This is a pre-existing issue, but does lockless list iteration of tx_ops
and rx_ops miss RCU wrappers?
When reading /proc/net/can-bcm/* via bcm_proc_show(), the code iterates
over tx_ops locklessly using the non-RCU list_for_each_entry().
Concurrently, operations added in bcm_tx_setup() (and bcm_rx_setup() for
rx_ops) use the standard list_add() instead of list_add_rcu():
bcm_tx_setup() {
...
list_add(&op->list, &bo->tx_ops);
}
Since list_add() omits the smp_store_release() memory barrier, can a
concurrent reader see uninitialized or partially linked list pointers,
leading to page faults or infinite loops?
[ ... ]
> @@ -1221,6 +1243,7 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>
> /* bcm_can_tx / bcm_tx_timeout_handler needs this */
> op->sk = sk;
> + sock_hold(sk);
> op->ifindex = ifindex;
[Severity: High]
This is a pre-existing issue, but are CAN frame configuration updates
unprotected against concurrent softirq handlers and timers?
In bcm_rx_setup(), memcpy_from_msg() directly modifies op->frames and
op->last_frames without synchronization. Concurrently, bcm_rx_handler()
accesses these frames locklessly in softirq context:
bcm_rx_handler() {
...
if ((get_u64(op->frames, 0) & get_u64(rxframe, 0)) ==
(get_u64(op->frames, 0) &
get_u64(op->frames + op->cfsiz * i, 0))) {
...
}
Similarly in bcm_tx_setup(), memcpy_from_msg() writes user data directly
into op->frames while a firing timer could trigger bcm_can_tx(), which
reads from op->frames locklessly.
Could this result in torn reads and cause the transmission or processing of
corrupted or partially validated CAN frames?
[ ... ]
> @@ -1869,6 +1898,8 @@ static void __exit bcm_module_exit(void)
> can_proto_unregister(&bcm_can_proto);
> unregister_netdevice_notifier(&canbcm_notifier);
> unregister_pernet_subsys(&canbcm_pernet_ops);
> + rcu_barrier();
> + destroy_workqueue(bcm_wq);
> }
[Severity: High]
Can the CAN protocol unregister before the workqueue is flushed?
If a user creates a BCM socket, arms a timer, and closes the socket, and then
the module is immediately unloaded, bcm_module_exit() executes
can_proto_unregister() before destroy_workqueue().
At this point, the deferred bcm_free_op_work() may still be pending in the
workqueue holding a reference to the socket.
Does this cause can_proto_unregister() to destroy the socket's slab cache
while it is still populated, leaking the cache and causing a potential
memory corruption when sk_free() falls back to kfree() on a slab-allocated
object?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-bcm_fixes-v6-0-80710a2bc398@hartkopp.net?part=1
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v6 2/9] can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure
2026-07-08 9:22 [PATCH v6 0/9] can: bcm: collected fixes - final Oliver Hartkopp via B4 Relay
2026-07-08 9:22 ` [PATCH v6 1/9] can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF Oliver Hartkopp via B4 Relay
@ 2026-07-08 9:22 ` Oliver Hartkopp via B4 Relay
2026-07-08 9:39 ` sashiko-bot
2026-07-08 9:22 ` [PATCH v6 3/9] can: bcm: add locking when updating filter and timer values Oliver Hartkopp via B4 Relay
` (6 subsequent siblings)
8 siblings, 1 reply; 17+ messages in thread
From: Oliver Hartkopp via B4 Relay @ 2026-07-08 9:22 UTC (permalink / raw)
To: linux-can; +Cc: Oliver Hartkopp, Marc Kleine-Budde, Ginger
From: Oliver Hartkopp <socketcan@hartkopp.net>
bcm_sendmsg() reads bo->ifindex and checks bo->bound before taking
lock_sock(), while bcm_notify(), bcm_connect() and bcm_release() all
mutate both fields under that same lock. Because the lockless reads
and the locked writes are unordered with respect to each other, a
racing bcm_notify() (device unregister) or bcm_connect() (concurrent
bind on another thread sharing the socket) can make bcm_sendmsg()
observe an inconsistent combination, e.g. a stale bound=1 together
with the now-cleared ifindex=0, silently turning a socket bound to a
specific CAN interface into one that also matches "any" interface.
Keep the lockless bo->bound check purely as a fast-path reject, and
move the ifindex read (and a bo->bound re-check) into the locked
section, where every writer already serializes. This removes the
possibility of observing the two fields torn against each other,
rather than trying to fix it with more READ_ONCE()/WRITE_ONCE() pairs
on two independently updated fields. Annotate the now-purely-lockless
bo->bound accesses consistently across all its write sites.
Also fix bcm_rx_setup() silently returning success when the target
device disappears concurrently instead of reporting -ENODEV, so a
broken RX op is no longer left registered as if it had succeeded.
Reported-by: Ginger <ginger.jzllee@gmail.com>
Closes: https://lore.kernel.org/linux-can/CAGp+u1aBK8QVjsvAxM2Ldzep4rEbsP9x_pV3At4g=h1kVEtyhA@mail.gmail.com/
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
net/can/bcm.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++-------------
1 file changed, 51 insertions(+), 14 deletions(-)
diff --git a/net/can/bcm.c b/net/can/bcm.c
index bdf53241bd7b..b612135b017d 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -1321,10 +1321,15 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
bcm_rx_handler, op,
"bcm", sk);
op->rx_reg_dev = dev;
dev_put(dev);
+ } else {
+ /* the requested device is gone - do not
+ * silently succeed without registering
+ */
+ err = -ENODEV;
}
} else
err = can_rx_register(sock_net(sk), NULL, op->can_id,
REGMASK(op->can_id),
@@ -1394,16 +1399,17 @@ static int bcm_tx_send(struct msghdr *msg, int ifindex, struct sock *sk,
*/
static int bcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
{
struct sock *sk = sock->sk;
struct bcm_sock *bo = bcm_sk(sk);
- int ifindex = bo->ifindex; /* default ifindex for this bcm_op */
+ int ifindex;
struct bcm_msg_head msg_head;
int cfsiz;
int ret; /* read bytes or error codes as return value */
- if (!bo->bound)
+ /* Lockless fast-path check for bound socket */
+ if (!READ_ONCE(bo->bound))
return -ENOTCONN;
/* check for valid message length from userspace */
if (size < MHSIZ)
return -EINVAL;
@@ -1415,43 +1421,65 @@ static int bcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
cfsiz = CFSIZ(msg_head.flags);
if ((size - MHSIZ) % cfsiz)
return -EINVAL;
+ lock_sock(sk);
+
+ /* Re-validate under the socket lock: a concurrent bcm_notify()
+ * may have unbound this socket (device removal) after the
+ * lockless fast-path check above. bo->ifindex is only ever
+ * mutated under lock_sock(), so reading it here - instead of
+ * before taking the lock - guarantees it can't be observed
+ * torn against bo->bound.
+ */
+ if (!bo->bound) {
+ ret = -ENOTCONN;
+ goto out_release;
+ }
+
+ /* default ifindex for this bcm_op */
+ ifindex = bo->ifindex;
+
/* check for alternative ifindex for this bcm_op */
if (!ifindex && msg->msg_name) {
/* no bound device as default => check msg_name */
DECLARE_SOCKADDR(struct sockaddr_can *, addr, msg->msg_name);
- if (msg->msg_namelen < BCM_MIN_NAMELEN)
- return -EINVAL;
+ if (msg->msg_namelen < BCM_MIN_NAMELEN) {
+ ret = -EINVAL;
+ goto out_release;
+ }
- if (addr->can_family != AF_CAN)
- return -EINVAL;
+ if (addr->can_family != AF_CAN) {
+ ret = -EINVAL;
+ goto out_release;
+ }
/* ifindex from sendto() */
ifindex = addr->can_ifindex;
if (ifindex) {
struct net_device *dev;
dev = dev_get_by_index(sock_net(sk), ifindex);
- if (!dev)
- return -ENODEV;
+ if (!dev) {
+ ret = -ENODEV;
+ goto out_release;
+ }
if (dev->type != ARPHRD_CAN) {
dev_put(dev);
- return -ENODEV;
+ ret = -ENODEV;
+ goto out_release;
}
dev_put(dev);
}
}
- lock_sock(sk);
-
switch (msg_head.opcode) {
case TX_SETUP:
ret = bcm_tx_setup(&msg_head, msg, ifindex, sk);
break;
@@ -1497,10 +1525,11 @@ static int bcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
default:
ret = -EINVAL;
break;
}
+out_release:
release_sock(sk);
return ret;
}
@@ -1533,11 +1562,16 @@ static void bcm_notify(struct bcm_sock *bo, unsigned long msg,
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;
}
#endif
- bo->bound = 0;
+ /* Paired with the lockless fast-path check in
+ * bcm_sendmsg(); bo->ifindex itself is only ever
+ * accessed under lock_sock() so it needs no
+ * annotation.
+ */
+ WRITE_ONCE(bo->bound, 0);
bo->ifindex = 0;
notify_enodev = 1;
}
release_sock(sk);
@@ -1674,11 +1708,11 @@ static int bcm_release(struct socket *sock)
list_for_each_entry_safe(op, next, &bo->rx_ops, list)
bcm_remove_op(op);
/* remove device reference */
if (bo->bound) {
- bo->bound = 0;
+ WRITE_ONCE(bo->bound, 0);
bo->ifindex = 0;
}
sock_orphan(sk);
sock->sk = NULL;
@@ -1744,11 +1778,14 @@ static int bcm_connect(struct socket *sock, struct sockaddr_unsized *uaddr, int
goto fail;
}
}
#endif /* CONFIG_PROC_FS */
- bo->bound = 1;
+ /* bo->ifindex above is fully assigned before this point; pairs
+ * with the lockless fast-path check in bcm_sendmsg()
+ */
+ WRITE_ONCE(bo->bound, 1);
fail:
release_sock(sk);
return ret;
--
2.53.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v6 2/9] can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure
2026-07-08 9:22 ` [PATCH v6 2/9] can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure Oliver Hartkopp via B4 Relay
@ 2026-07-08 9:39 ` sashiko-bot
0 siblings, 0 replies; 17+ messages in thread
From: sashiko-bot @ 2026-07-08 9:39 UTC (permalink / raw)
To: Oliver Hartkopp
Cc: Vincent Mailhol, Oliver Hartkopp, Oleksij Rempel, linux-can,
Marc Kleine-Budde
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues:
- [High] Lockless, non-RCU iteration of the tx_ops and rx_ops lists in bcm_proc_show() races with concurrent list mutations, leading to list corruption and crashes.
- [High] bcm_release() leaks CAN receiver hooks and causes a Use-After-Free if the network device is unregistered concurrently.
- [Medium] bcm_rx_setup() silently fails to register the CAN RX hook when updating an existing operation for a recreated device.
- [Medium] bcm_notify() accesses bo->ifindex locklessly during NETDEV_DOWN, contradicting the patch's assumption and risking missed notifications.
--
--- Patch [2]: [PATCH v6 2/9] can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure ---
commit 216efa99603464dc19876367ec3eb7d6e1414515
Author: Oliver Hartkopp <socketcan@hartkopp.net>
Date: Wed Jul 8 09:22:28 2026 +0000
can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure
This commit fixes a lockless race condition between bcm_sendmsg() and
functions that mutate bo->bound and bo->ifindex by re-validating the
state under the socket lock. It also addresses a silent failure in
bcm_rx_setup() when the target device is removed concurrently.
Link: https://lore.kernel.org/linux-can/CAGp+u1aBK8QVjsvAxM2Ldzep4rEbsP9x_pV3At4g=h1kVEtyhA@mail.gmail.com/
> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index bdf53241bd7b3..b612135b017d1 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
[ ... ]
> @@ -1323,6 +1323,11 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>
> op->rx_reg_dev = dev;
> dev_put(dev);
> + } else {
> + /* the requested device is gone - do not
> + * silently succeed without registering
> + */
> + err = -ENODEV;
> }
>
> } else
[Severity: Medium]
This is a pre-existing issue, but can bcm_rx_setup() silently fail to
register the CAN RX hook when updating an existing operation?
If a socket is bound to 'any' interface and a user issues an RX_SETUP
for a specific device that was previously removed and recreated, it
appears do_rx_register remains 0:
bcm_rx_setup() {
...
if (op) {
...
/* Only an update -> do not call can_rx_register() */
do_rx_register = 0;
}
}
Since bcm_notify() unregisters the hook and sets op->rx_reg_dev = NULL
when the device is removed, updating the dormant bcm_op without
registering the hook again leaves the socket unable to receive frames.
[ ... ]
> @@ -1535,7 +1564,12 @@ static void bcm_notify(struct bcm_sock *bo, unsigned long msg,
> bo->bcm_proc_read = NULL;
> }
> #endif
> - bo->bound = 0;
> + /* Paired with the lockless fast-path check in
> + * bcm_sendmsg(); bo->ifindex itself is only ever
> + * accessed under lock_sock() so it needs no
> + * annotation.
> + */
> + WRITE_ONCE(bo->bound, 0);
> bo->ifindex = 0;
> notify_enodev = 1;
> }
[Severity: Medium]
This is a pre-existing issue, but the new comment states bo->ifindex is
only ever accessed under lock_sock(). Is this accurate?
Looking at NETDEV_DOWN handling in bcm_notify(), it accesses bo->ifindex
locklessly:
bcm_notify() {
...
case NETDEV_DOWN:
if (bo->bound && bo->ifindex == dev->ifindex) {
sk->sk_err = ENETDOWN;
...
}
}
Could a concurrent bcm_connect() cause this to read a stale bo->ifindex
and miss sending the ENETDOWN notification?
[ ... ]
> @@ -1676,7 +1710,7 @@ static int bcm_release(struct socket *sock)
>
> /* remove device reference */
> if (bo->bound) {
> - bo->bound = 0;
> + WRITE_ONCE(bo->bound, 0);
> bo->ifindex = 0;
> }
[Severity: High]
This is a pre-existing issue, but does bcm_release() risk leaking CAN
receiver hooks if the network device is unregistered concurrently?
When a CAN socket is closed, bcm_release() relies on dev_get_by_index()
to find the device and unregister the hook:
bcm_release() {
...
dev = dev_get_by_index(net, op->ifindex);
if (dev) {
bcm_rx_unreg(dev, op);
dev_put(dev);
}
}
If the device is being unregistered concurrently, it is removed from the
network namespace index by unlist_netdevice(), so dev_get_by_index()
returns NULL. This causes bcm_release() to skip calling bcm_rx_unreg(),
leaving a dangling pointer in the CAN core's dev_rcv_lists that can lead
to a use-after-free.
[ ... ]
> @@ -1746,7 +1780,10 @@ static int bcm_connect(struct socket *sock, struct sockaddr_unsized *uaddr, int
> }
> #endif /* CONFIG_PROC_FS */
>
> - bo->bound = 1;
> + /* bo->ifindex above is fully assigned before this point; pairs
> + * with the lockless fast-path check in bcm_sendmsg()
> + */
> + WRITE_ONCE(bo->bound, 1);
>
> fail:
> release_sock(sk);
[Severity: Medium]
This is a pre-existing issue, but does this lack the necessary memory
barriers to enforce ordering?
Because bcm_notify() can read these fields locklessly during a NETDEV_DOWN
event, doesn't WRITE_ONCE() alone fail to guarantee that the store to
bo->ifindex is visible to other CPUs before bo->bound = 1 without an
explicit smp_wmb()?
[Severity: High]
This isn't a bug introduced by this patch, but is there a race condition
between lockless list iteration in bcm_proc_show() and list mutations?
When a user reads /proc/net/can-bcm/*, bcm_proc_show() iterates over
bo->tx_ops and bo->rx_ops holding only the rcu_read_lock():
bcm_proc_show() {
...
list_for_each_entry(op, &bo->tx_ops, list) {
...
}
}
However, when adding elements, the writers appear to use list_add() instead
of list_add_rcu(), which fails to emit the necessary smp_store_release()
barriers:
bcm_tx_setup() {
...
list_add(&op->list, &bo->tx_ops);
}
Can weakly-ordered CPUs make the new node's next pointer visible to the
lockless reader before the node itself is fully initialized, causing a
crash?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-bcm_fixes-v6-0-80710a2bc398@hartkopp.net?part=2
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v6 3/9] can: bcm: add locking when updating filter and timer values
2026-07-08 9:22 [PATCH v6 0/9] can: bcm: collected fixes - final Oliver Hartkopp via B4 Relay
2026-07-08 9:22 ` [PATCH v6 1/9] can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF Oliver Hartkopp via B4 Relay
2026-07-08 9:22 ` [PATCH v6 2/9] can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure Oliver Hartkopp via B4 Relay
@ 2026-07-08 9:22 ` Oliver Hartkopp via B4 Relay
2026-07-08 9:37 ` sashiko-bot
2026-07-08 9:22 ` [PATCH v6 4/9] can: bcm: fix CAN frame rx/tx statistics Oliver Hartkopp via B4 Relay
` (5 subsequent siblings)
8 siblings, 1 reply; 17+ messages in thread
From: Oliver Hartkopp via B4 Relay @ 2026-07-08 9:22 UTC (permalink / raw)
To: linux-can; +Cc: Oliver Hartkopp, Marc Kleine-Budde, syzbot+75e5e4ae00c3b4bb544e
From: Oliver Hartkopp <socketcan@hartkopp.net>
KCSAN detected a simultaneous access to timer values that can be
overwritten in bcm_rx_setup() when updating timer and filter content
while bcm_rx_handler(), bcm_rx_timeout_handler() or bcm_rx_thr_handler()
run concurrently on incoming CAN traffic.
Protect the timer (ival1/ival2/kt_ival1/kt_ival2/kt_lastmsg) and filter
(nframes/flags/frames/last_frames) updates in bcm_rx_setup() with a new
per-op bcm_rx_update_lock, taken with the matching scope in the RX
handlers. memcpy_from_msg() is staged into a temporary buffer before the
lock is taken, since it can sleep and must not run under a spinlock.
hrtimer_cancel() is always called without bcm_rx_update_lock held, since
bcm_rx_timeout_handler()/bcm_rx_thr_handler() take the same lock and a
running callback would otherwise deadlock against the canceller.
Also close a related race: bcm_rx_setup() cleared the RTR flag in the
stored reply frame's can_id as a separate, unprotected step after the
frame content was already installed, so a concurrent bcm_rx_handler()
could transmit a stale reply with CAN_RTR_FLAG still set. Fold that
normalization into the initial frame preparation instead (on the staged
buffer for updates, directly on op->frames pre-registration for new
ops), so the installed frame is always atomically self-consistent.
bcm_rx_handler()'s RX_RTR_FRAME check now takes a lock-protected
snapshot of op->flags before deciding whether to call bcm_can_tx(),
but does not hold the lock across that call.
Also take a lock-protected snapshot of the currframe in bcm_can_tx()
to avoid partly overwrites by content updates in bcm_tx_setup().
Omit calling hrtimer_forward() with zero interval in bcm_rx_thr_handler().
kt_ival2 may have been concurrently cleared by bcm_rx_setup() before it
cancels this timer, so check kt_ival2 inside the bcm_rx_update_lock.
Fixes: c2aba69d0c36 ("can: bcm: add locking for bcm_op runtime updates")
Reported-by: syzbot+75e5e4ae00c3b4bb544e@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/linux-can/6975d5cf.a00a0220.33ccc7.0022.GAE@google.com/
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
net/can/bcm.c | 136 ++++++++++++++++++++++++++++++++++++++++++++--------------
1 file changed, 104 insertions(+), 32 deletions(-)
diff --git a/net/can/bcm.c b/net/can/bcm.c
index b612135b017d..af4bb4182bc2 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -127,10 +127,11 @@ struct bcm_op {
struct canfd_frame sframe;
struct canfd_frame last_sframe;
struct sock *sk;
struct net_device *rx_reg_dev;
spinlock_t bcm_tx_lock; /* protect currframe/count in runtime updates */
+ spinlock_t bcm_rx_update_lock; /* protect filter/timer data updates */
};
struct bcm_sock {
struct sock sk;
int bound;
@@ -291,26 +292,30 @@ static int bcm_proc_show(struct seq_file *m, void *v)
/*
* bcm_can_tx - send the (next) CAN frame to the appropriate CAN interface
* of the given bcm tx op
*/
-static void bcm_can_tx(struct bcm_op *op)
+static void bcm_can_tx(struct bcm_op *op, struct canfd_frame *cf)
{
struct sk_buff *skb;
struct can_skb_ext *csx;
struct net_device *dev;
- struct canfd_frame *cf;
+ struct canfd_frame cframe;
int err;
/* no target device? => exit */
if (!op->ifindex)
return;
- /* read currframe under lock protection */
- spin_lock_bh(&op->bcm_tx_lock);
- cf = op->frames + op->cfsiz * op->currframe;
- spin_unlock_bh(&op->bcm_tx_lock);
+ if (!cf) {
+ /* read currframe under lock protection */
+ spin_lock_bh(&op->bcm_tx_lock);
+ memcpy(&cframe, op->frames + op->cfsiz * op->currframe,
+ op->cfsiz);
+ cf = &cframe;
+ spin_unlock_bh(&op->bcm_tx_lock);
+ }
dev = dev_get_by_index(sock_net(op->sk), op->ifindex);
if (!dev) {
/* RFC: should this bcm_op remove itself here? */
return;
@@ -459,11 +464,11 @@ static enum hrtimer_restart bcm_tx_timeout_handler(struct hrtimer *hrtimer)
{
struct bcm_op *op = container_of(hrtimer, struct bcm_op, timer);
struct bcm_msg_head msg_head;
if (op->kt_ival1 && (op->count > 0)) {
- bcm_can_tx(op);
+ bcm_can_tx(op, NULL);
if (!op->count && (op->flags & TX_COUNTEVT)) {
/* create notification to user */
memset(&msg_head, 0, sizeof(msg_head));
msg_head.opcode = TX_EXPIRED;
@@ -476,11 +481,11 @@ static enum hrtimer_restart bcm_tx_timeout_handler(struct hrtimer *hrtimer)
bcm_send_to_user(op, &msg_head, NULL, 0);
}
} else if (op->kt_ival2) {
- bcm_can_tx(op);
+ bcm_can_tx(op, NULL);
}
return bcm_tx_set_expiry(op, &op->timer) ?
HRTIMER_RESTART : HRTIMER_NORESTART;
}
@@ -620,10 +625,12 @@ static void bcm_rx_starttimer(struct bcm_op *op)
static enum hrtimer_restart bcm_rx_timeout_handler(struct hrtimer *hrtimer)
{
struct bcm_op *op = container_of(hrtimer, struct bcm_op, timer);
struct bcm_msg_head msg_head;
+ spin_lock_bh(&op->bcm_rx_update_lock);
+
/* if user wants to be informed, when cyclic CAN-Messages come back */
if ((op->flags & RX_ANNOUNCE_RESUME) && op->last_frames) {
/* clear received CAN frames to indicate 'nothing received' */
memset(op->last_frames, 0, op->nframes * op->cfsiz);
}
@@ -636,10 +643,12 @@ static enum hrtimer_restart bcm_rx_timeout_handler(struct hrtimer *hrtimer)
msg_head.ival1 = op->ival1;
msg_head.ival2 = op->ival2;
msg_head.can_id = op->can_id;
msg_head.nframes = 0;
+ spin_unlock_bh(&op->bcm_rx_update_lock);
+
bcm_send_to_user(op, &msg_head, NULL, 0);
return HRTIMER_NORESTART;
}
@@ -684,30 +693,43 @@ static int bcm_rx_thr_flush(struct bcm_op *op)
* Check for throttled data and send it to the userspace
*/
static enum hrtimer_restart bcm_rx_thr_handler(struct hrtimer *hrtimer)
{
struct bcm_op *op = container_of(hrtimer, struct bcm_op, thrtimer);
+ enum hrtimer_restart ret;
+
+ spin_lock_bh(&op->bcm_rx_update_lock);
- if (bcm_rx_thr_flush(op)) {
+ /* kt_ival2 may have been concurrently cleared by bcm_rx_setup()
+ * before it cancels this timer - never forward with a zero
+ * interval in that case.
+ */
+ if (bcm_rx_thr_flush(op) && op->kt_ival2) {
hrtimer_forward_now(hrtimer, op->kt_ival2);
- return HRTIMER_RESTART;
+ ret = HRTIMER_RESTART;
} else {
/* rearm throttle handling */
op->kt_lastmsg = 0;
- return HRTIMER_NORESTART;
+ ret = HRTIMER_NORESTART;
}
+
+ spin_unlock_bh(&op->bcm_rx_update_lock);
+
+ return ret;
}
/*
* bcm_rx_handler - handle a CAN frame reception
*/
static void bcm_rx_handler(struct sk_buff *skb, void *data)
{
struct bcm_op *op = (struct bcm_op *)data;
const struct canfd_frame *rxframe = (struct canfd_frame *)skb->data;
+ struct canfd_frame rtrframe;
unsigned int i;
unsigned char traffic_flags;
+ bool rtr_frame;
if (op->can_id != rxframe->can_id)
return;
/* make sure to handle the correct frame type (CAN / CAN FD) */
@@ -727,13 +749,22 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data)
/* save originator for recvfrom() */
op->rx_ifindex = skb->dev->ifindex;
/* update statistics */
op->frames_abs++;
- if (op->flags & RX_RTR_FRAME) {
+ /* snapshot the flag under lock: op->flags/op->frames may be updated
+ * concurrently by bcm_rx_setup().
+ */
+ spin_lock_bh(&op->bcm_rx_update_lock);
+ rtr_frame = op->flags & RX_RTR_FRAME;
+ if (rtr_frame)
+ memcpy(&rtrframe, op->frames, op->cfsiz);
+ spin_unlock_bh(&op->bcm_rx_update_lock);
+
+ if (rtr_frame) {
/* send reply for RTR-request (placed in op->frames[0]) */
- bcm_can_tx(op);
+ bcm_can_tx(op, &rtrframe);
return;
}
/* compute flags to distinguish between own/local/remote CAN traffic */
traffic_flags = 0;
@@ -741,10 +772,12 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data)
traffic_flags |= RX_LOCAL;
if (skb->sk == op->sk)
traffic_flags |= RX_OWN;
}
+ spin_lock_bh(&op->bcm_rx_update_lock);
+
if (op->flags & RX_FILTER_ID) {
/* the easiest case */
bcm_rx_update_and_send(op, op->last_frames, rxframe,
traffic_flags);
goto rx_starttimer;
@@ -776,10 +809,12 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data)
}
}
rx_starttimer:
bcm_rx_starttimer(op);
+
+ spin_unlock_bh(&op->bcm_rx_update_lock);
}
/*
* helpers for bcm_op handling: find & delete bcm [rx|tx] op elements
*/
@@ -1114,11 +1149,11 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
/* spec: send CAN frame when starting timer */
op->flags |= TX_ANNOUNCE;
}
if (op->flags & TX_ANNOUNCE)
- bcm_can_tx(op);
+ bcm_can_tx(op, NULL);
if (op->flags & STARTTIMER)
bcm_tx_start_timer(op);
return msg_head->nframes * op->cfsiz + MHSIZ;
@@ -1128,10 +1163,28 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
kfree(op->frames);
kfree(op);
return err;
}
+static void bcm_rx_setup_rtr_check(struct bcm_msg_head *msg_head,
+ struct bcm_op *op, void *new_frames)
+{
+ /* funny feature in RX(!)_SETUP only for RTR-mode:
+ * copy can_id into frame BUT without RTR-flag to
+ * prevent a full-load-loopback-test ... ;-]
+ * normalize this on the staged buffer, before it is
+ * ever installed into op->frames.
+ */
+ if (msg_head->flags & RX_RTR_FRAME) {
+ struct canfd_frame *frame0 = new_frames;
+
+ if ((msg_head->flags & TX_CP_CAN_ID) ||
+ frame0->can_id == op->can_id)
+ frame0->can_id = op->can_id & ~CAN_RTR_FLAG;
+ }
+}
+
/*
* bcm_rx_setup - create or update a bcm rx op (for bcm_sendmsg)
*/
static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
int ifindex, struct sock *sk)
@@ -1162,10 +1215,12 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
return -EINVAL;
/* check the given can_id */
op = bcm_find_op(&bo->rx_ops, msg_head, ifindex);
if (op) {
+ void *new_frames = NULL;
+
/* update existing BCM operation */
/*
* Do we need more space for the CAN frames than currently
* allocated? -> This is a _really_ unusual use-case and
@@ -1173,33 +1228,55 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
*/
if (msg_head->nframes > op->nframes)
return -E2BIG;
if (msg_head->nframes) {
- /* update CAN frames content */
- err = memcpy_from_msg(op->frames, msg,
+ /* get new CAN frames content before locking */
+ new_frames = kmalloc(msg_head->nframes * op->cfsiz,
+ GFP_KERNEL);
+ if (!new_frames)
+ return -ENOMEM;
+
+ err = memcpy_from_msg(new_frames, msg,
msg_head->nframes * op->cfsiz);
- if (err < 0)
+ if (err < 0) {
+ kfree(new_frames);
return err;
+ }
- /* clear last_frames to indicate 'nothing received' */
- memset(op->last_frames, 0, msg_head->nframes * op->cfsiz);
+ bcm_rx_setup_rtr_check(msg_head, op, new_frames);
}
+ spin_lock_bh(&op->bcm_rx_update_lock);
op->nframes = msg_head->nframes;
op->flags = msg_head->flags;
+ if (msg_head->nframes) {
+ /* update CAN frames content */
+ memcpy(op->frames, new_frames,
+ msg_head->nframes * op->cfsiz);
+
+ /* clear last_frames to indicate 'nothing received' */
+ memset(op->last_frames, 0,
+ msg_head->nframes * op->cfsiz);
+ }
+ spin_unlock_bh(&op->bcm_rx_update_lock);
+
+ /* free temporary frames / kfree(NULL) is safe */
+ kfree(new_frames);
+
/* Only an update -> do not call can_rx_register() */
do_rx_register = 0;
} else {
/* insert new BCM operation for the given can_id */
op = kzalloc(OPSIZ, GFP_KERNEL);
if (!op)
return -ENOMEM;
spin_lock_init(&op->bcm_tx_lock);
+ spin_lock_init(&op->bcm_rx_update_lock);
op->can_id = msg_head->can_id;
op->nframes = msg_head->nframes;
op->cfsiz = CFSIZ(msg_head->flags);
op->flags = msg_head->flags;
@@ -1237,10 +1314,12 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
if (op->last_frames != &op->last_sframe)
kfree(op->last_frames);
kfree(op);
return err;
}
+
+ bcm_rx_setup_rtr_check(msg_head, op, op->frames);
}
/* bcm_can_tx / bcm_tx_timeout_handler needs this */
op->sk = sk;
sock_hold(sk);
@@ -1264,45 +1343,38 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
} /* if ((op = bcm_find_op(&bo->rx_ops, msg_head->can_id, ifindex))) */
/* check flags */
if (op->flags & RX_RTR_FRAME) {
- struct canfd_frame *frame0 = op->frames;
-
/* no timers in RTR-mode */
hrtimer_cancel(&op->thrtimer);
hrtimer_cancel(&op->timer);
-
- /*
- * funny feature in RX(!)_SETUP only for RTR-mode:
- * copy can_id into frame BUT without RTR-flag to
- * prevent a full-load-loopback-test ... ;-]
- */
- if ((op->flags & TX_CP_CAN_ID) ||
- (frame0->can_id == op->can_id))
- frame0->can_id = op->can_id & ~CAN_RTR_FLAG;
-
} else {
if (op->flags & SETTIMER) {
/* set timer value */
+ spin_lock_bh(&op->bcm_rx_update_lock);
op->ival1 = msg_head->ival1;
op->ival2 = msg_head->ival2;
op->kt_ival1 = bcm_timeval_to_ktime(msg_head->ival1);
op->kt_ival2 = bcm_timeval_to_ktime(msg_head->ival2);
+ op->kt_lastmsg = 0;
+ spin_unlock_bh(&op->bcm_rx_update_lock);
/* disable an active timer due to zero value? */
if (!op->kt_ival1)
hrtimer_cancel(&op->timer);
/*
* In any case cancel the throttle timer, flush
* potentially blocked msgs and reset throttle handling
*/
- op->kt_lastmsg = 0;
hrtimer_cancel(&op->thrtimer);
+
+ spin_lock_bh(&op->bcm_rx_update_lock);
bcm_rx_thr_flush(op);
+ spin_unlock_bh(&op->bcm_rx_update_lock);
}
if ((op->flags & STARTTIMER) && op->kt_ival1)
hrtimer_start(&op->timer, op->kt_ival1,
HRTIMER_MODE_REL_SOFT);
--
2.53.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v6 3/9] can: bcm: add locking when updating filter and timer values
2026-07-08 9:22 ` [PATCH v6 3/9] can: bcm: add locking when updating filter and timer values Oliver Hartkopp via B4 Relay
@ 2026-07-08 9:37 ` sashiko-bot
0 siblings, 0 replies; 17+ messages in thread
From: sashiko-bot @ 2026-07-08 9:37 UTC (permalink / raw)
To: Oliver Hartkopp
Cc: Oleksij Rempel, Vincent Mailhol, linux-can, Marc Kleine-Budde,
Oliver Hartkopp
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] Torn reads of 64-bit ktime_t variables and op->count in bcm_tx_timeout_handler.
- [Medium] Unprotected state modifications in bcm_rx_handler lead to data races when listening on all interfaces.
--
--- Patch [3]: [PATCH v6 3/9] can: bcm: add locking when updating filter and timer values ---
commit 27c469169060f50f95adf2e7ea3b4871de414a2e
Author: Oliver Hartkopp <socketcan@hartkopp.net>
can: bcm: add locking when updating filter and timer values
KCSAN detected a simultaneous access to timer values that can be
overwritten in bcm_rx_setup() when updating timer and filter content
while bcm_rx_handler(), bcm_rx_timeout_handler() or bcm_rx_thr_handler()
run concurrently on incoming CAN traffic.
Link: https://lore.kernel.org/linux-can/6975d5cf.a00a0220.33ccc7.0022.GAE@google.com/
> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index b612135b017d1..af4bb4182bc29 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -461,7 +466,7 @@ static enum hrtimer_restart bcm_tx_timeout_handler(struct hrtimer *hrtimer)
> struct bcm_msg_head msg_head;
>
> if (op->kt_ival1 && (op->count > 0)) {
> - bcm_can_tx(op);
> + bcm_can_tx(op, NULL);
> if (!op->count && (op->flags & TX_COUNTEVT)) {
[Severity: High]
This is a pre-existing issue, but does bcm_tx_timeout_handler() read
kt_ival1, kt_ival2, and count without holding bcm_tx_lock?
A user application could concurrently update timer values via
bcm_sendmsg(TX_SETUP | SETTIMER) while the active hrtimer callback
bcm_tx_timeout_handler() executes in softirq context. This could result in a
torn read of the 64-bit ktime_t timeout values on 32-bit systems,
potentially causing incorrect timer expiry times. Data races on op->count
can also cause TOCTOU bugs resulting in missed or extra transmissions.
In bcm_tx_setup(), the timer interval values appear to be written without
acquiring bcm_tx_lock:
if (op->flags & SETTIMER) {
/* set timer values */
op->ival1 = msg_head->ival1;
op->ival2 = msg_head->ival2;
op->kt_ival1 = bcm_timeval_to_ktime(msg_head->ival1);
op->kt_ival2 = bcm_timeval_to_ktime(msg_head->ival2);
[ ... ]
> @@ -729,9 +751,18 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data)
> /* update statistics */
> op->frames_abs++;
[Severity: Medium]
This isn't a bug introduced by this patch, but can op->rx_stamp,
op->rx_ifindex, and op->frames_abs be updated concurrently on multiple CPUs
before bcm_rx_update_lock is acquired?
If a BCM socket is bound to all interfaces (ifindex = 0) and multiple CAN
interfaces receive frames matching the filter simultaneously,
bcm_rx_handler() can run concurrently on multiple CPUs for the same bcm_op.
Could this lead to lost statistic increments, interleaved metadata, or a
torn read of the 64-bit rx_stamp on 32-bit systems?
> - if (op->flags & RX_RTR_FRAME) {
> + /* snapshot the flag under lock: op->flags/op->frames may be updated
> + * concurrently by bcm_rx_setup().
> + */
> + spin_lock_bh(&op->bcm_rx_update_lock);
> + rtr_frame = op->flags & RX_RTR_FRAME;
> + if (rtr_frame)
> + memcpy(&rtrframe, op->frames, op->cfsiz);
> + spin_unlock_bh(&op->bcm_rx_update_lock);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-bcm_fixes-v6-0-80710a2bc398@hartkopp.net?part=3
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v6 4/9] can: bcm: fix CAN frame rx/tx statistics
2026-07-08 9:22 [PATCH v6 0/9] can: bcm: collected fixes - final Oliver Hartkopp via B4 Relay
` (2 preceding siblings ...)
2026-07-08 9:22 ` [PATCH v6 3/9] can: bcm: add locking when updating filter and timer values Oliver Hartkopp via B4 Relay
@ 2026-07-08 9:22 ` Oliver Hartkopp via B4 Relay
2026-07-08 9:39 ` sashiko-bot
2026-07-08 9:22 ` [PATCH v6 5/9] can: bcm: add missing rcu list annotations and operations Oliver Hartkopp via B4 Relay
` (4 subsequent siblings)
8 siblings, 1 reply; 17+ messages in thread
From: Oliver Hartkopp via B4 Relay @ 2026-07-08 9:22 UTC (permalink / raw)
To: linux-can; +Cc: Oliver Hartkopp, Marc Kleine-Budde
From: Oliver Hartkopp <socketcan@hartkopp.net>
KCSAN detected a data race within the bcm_rx_handler() when two CAN frames
have been simultaneously received and processed in a single rx op by two
different CPUs.
Use atomic operations with (signed) long data types to access the
statistics in the hot path to fix the KCSAN complaint.
Additionally simplify the update and check of statistics overflow by
using the atomic operations in a separate bcm_update_stats() function and
put this function under bcm_rx_update_lock to prevent races when resetting
the counters.
Fixes: ffd980f976e7 ("[CAN]: Add broadcast manager (bcm) protocol")
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
net/can/bcm.c | 47 ++++++++++++++++++++++++++++++-----------------
1 file changed, 30 insertions(+), 17 deletions(-)
diff --git a/net/can/bcm.c b/net/can/bcm.c
index af4bb4182bc2..340b7fc89744 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -110,11 +110,11 @@ struct bcm_op {
struct rcu_head rcu;
struct work_struct work;
int ifindex;
canid_t can_id;
u32 flags;
- unsigned long frames_abs, frames_filtered;
+ atomic_long_t frames_abs, frames_filtered;
struct bcm_timeval ival1, ival2;
struct hrtimer timer, thrtimer;
ktime_t rx_stamp, kt_ival1, kt_ival2, kt_lastmsg;
int rx_ifindex;
int cfsiz;
@@ -227,14 +227,17 @@ static int bcm_proc_show(struct seq_file *m, void *v)
rcu_read_lock();
list_for_each_entry_rcu(op, &bo->rx_ops, list) {
- unsigned long reduction;
+ long reduction, frames_filtered, frames_abs;
+
+ frames_filtered = atomic_long_read(&op->frames_filtered);
+ frames_abs = atomic_long_read(&op->frames_abs);
/* print only active entries & prevent division by zero */
- if (!op->frames_abs)
+ if (!frames_abs)
continue;
seq_printf(m, "rx_op: %03X %-5s ", op->can_id,
bcm_proc_getifname(net, ifname, op->ifindex));
@@ -252,13 +255,13 @@ static int bcm_proc_show(struct seq_file *m, void *v)
if (op->kt_ival2)
seq_printf(m, "thr=%lld ",
(long long)ktime_to_us(op->kt_ival2));
seq_printf(m, "# recv %ld (%ld) => reduction: ",
- op->frames_filtered, op->frames_abs);
+ frames_filtered, frames_abs);
- reduction = 100 - (op->frames_filtered * 100) / op->frames_abs;
+ reduction = 100 - (frames_filtered * 100) / frames_abs;
seq_printf(m, "%s%ld%%\n",
(reduction == 100) ? "near " : "", reduction);
}
@@ -278,20 +281,30 @@ static int bcm_proc_show(struct seq_file *m, void *v)
if (op->kt_ival2)
seq_printf(m, "t2=%lld ",
(long long)ktime_to_us(op->kt_ival2));
- seq_printf(m, "# sent %ld\n", op->frames_abs);
+ seq_printf(m, "# sent %ld\n",
+ atomic_long_read(&op->frames_abs));
}
seq_putc(m, '\n');
rcu_read_unlock();
return 0;
}
#endif /* CONFIG_PROC_FS */
+static void bcm_update_stats(struct bcm_op *op)
+{
+ /* prevent statistics overflow */
+ if (atomic_long_inc_return(&op->frames_abs) > LONG_MAX / 100) {
+ atomic_long_set(&op->frames_filtered, 0);
+ atomic_long_set(&op->frames_abs, 0);
+ }
+}
+
/*
* bcm_can_tx - send the (next) CAN frame to the appropriate CAN interface
* of the given bcm tx op
*/
static void bcm_can_tx(struct bcm_op *op, struct canfd_frame *cf)
@@ -342,11 +355,11 @@ static void bcm_can_tx(struct bcm_op *op, struct canfd_frame *cf)
/* update currframe and count under lock protection */
spin_lock_bh(&op->bcm_tx_lock);
if (!err)
- op->frames_abs++;
+ bcm_update_stats(op);
op->currframe++;
/* reached last frame? */
if (op->currframe >= op->nframes)
@@ -495,16 +508,13 @@ static enum hrtimer_restart bcm_tx_timeout_handler(struct hrtimer *hrtimer)
*/
static void bcm_rx_changed(struct bcm_op *op, struct canfd_frame *data)
{
struct bcm_msg_head head;
- /* update statistics */
- op->frames_filtered++;
-
- /* prevent statistics overflow */
- if (op->frames_filtered > ULONG_MAX/100)
- op->frames_filtered = op->frames_abs = 0;
+ /* update statistics (frames_filtered <= frames_abs) */
+ if (atomic_long_read(&op->frames_abs))
+ atomic_long_inc(&op->frames_filtered);
/* this element is not throttled anymore */
data->flags &= ~RX_THR;
memset(&head, 0, sizeof(head));
@@ -746,20 +756,23 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data)
/* save rx timestamp */
op->rx_stamp = skb->tstamp;
/* save originator for recvfrom() */
op->rx_ifindex = skb->dev->ifindex;
- /* update statistics */
- op->frames_abs++;
- /* snapshot the flag under lock: op->flags/op->frames may be updated
- * concurrently by bcm_rx_setup().
+ /* snapshot RTR content under lock: op->flags/op->frames may be
+ * updated concurrently by bcm_rx_setup().
*/
spin_lock_bh(&op->bcm_rx_update_lock);
+
+ /* update statistics with lock analogue to bcm_rx_changed() callers */
+ bcm_update_stats(op);
+
rtr_frame = op->flags & RX_RTR_FRAME;
if (rtr_frame)
memcpy(&rtrframe, op->frames, op->cfsiz);
+
spin_unlock_bh(&op->bcm_rx_update_lock);
if (rtr_frame) {
/* send reply for RTR-request (placed in op->frames[0]) */
bcm_can_tx(op, &rtrframe);
--
2.53.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v6 4/9] can: bcm: fix CAN frame rx/tx statistics
2026-07-08 9:22 ` [PATCH v6 4/9] can: bcm: fix CAN frame rx/tx statistics Oliver Hartkopp via B4 Relay
@ 2026-07-08 9:39 ` sashiko-bot
0 siblings, 0 replies; 17+ messages in thread
From: sashiko-bot @ 2026-07-08 9:39 UTC (permalink / raw)
To: Oliver Hartkopp
Cc: Vincent Mailhol, Oliver Hartkopp, Oleksij Rempel, linux-can,
Marc Kleine-Budde
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [Medium] The split of statistic updates across two separate spinlock critical sections in bcm_rx_handler allows frames_filtered to persistently exceed frames_abs after a counter reset, breaking their invariant and causing negative reduction percentages.
Pre-existing issues:
- [High] Concurrent, lockless updates to op->rx_stamp and op->rx_ifindex in bcm_rx_handler() cause data races and potential torn writes.
--
--- Patch [4]: [PATCH v6 4/9] can: bcm: fix CAN frame rx/tx statistics ---
commit fb8c8dc88e31fc268389c46d858c779a051a866c
Author: Oliver Hartkopp <socketcan@hartkopp.net>
can: bcm: fix CAN frame rx/tx statistics
KCSAN detected a data race within the bcm_rx_handler() when two CAN frames
have been simultaneously received and processed in a single rx op by two
different CPUs.
Use atomic operations with (signed) long data types to access the
statistics in the hot path to fix the KCSAN complaint.
Additionally simplify the update and check of statistics overflow by
using the atomic operations in a separate bcm_update_stats() function and
put this function under bcm_rx_update_lock to prevent races when resetting
the counters.
> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index af4bb4182bc29..340b7fc89744e 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
[ ... ]
> @@ -497,12 +510,9 @@ static void bcm_rx_changed(struct bcm_op *op, struct canfd_frame *data)
> {
> struct bcm_msg_head head;
>
> - /* update statistics */
> - op->frames_filtered++;
> -
> - /* prevent statistics overflow */
> - if (op->frames_filtered > ULONG_MAX/100)
> - op->frames_filtered = op->frames_abs = 0;
> + /* update statistics (frames_filtered <= frames_abs) */
> + if (atomic_long_read(&op->frames_abs))
> + atomic_long_inc(&op->frames_filtered);
[Severity: Medium]
With the statistic updates now split across two separate lock regions in
bcm_rx_handler(), is it possible for frames_filtered to exceed frames_abs?
If Thread A calls bcm_update_stats() in the first lock region and triggers
the counter reset, then releases the lock, could Thread B interleave and
increment frames_abs to 1 before Thread A reaches the second lock region?
When Thread A subsequently calls bcm_rx_changed() in the second lock region,
it would observe frames_abs == 1 and increment frames_filtered (for the
frame that already caused the reset).
If Thread B then enters the second lock region and also increments
frames_filtered, wouldn't frames_filtered become larger than frames_abs?
When read by bcm_proc_show(), would this cause the reduction calculation
(100 - (frames_filtered * 100) / frames_abs) to yield a negative percentage?
[ ... ]
> @@ -748,16 +758,19 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data)
> op->rx_stamp = skb->tstamp;
> /* save originator for recvfrom() */
> op->rx_ifindex = skb->dev->ifindex;
> - /* update statistics */
> - op->frames_abs++;
>
> - /* snapshot the flag under lock: op->flags/op->frames may be updated
> - * concurrently by bcm_rx_setup().
> + /* snapshot RTR content under lock: op->flags/op->frames may be
> + * updated concurrently by bcm_rx_setup().
> */
> spin_lock_bh(&op->bcm_rx_update_lock);
[Severity: High]
This isn't a bug introduced by this patch, but since bcm_rx_handler() can be
executed concurrently on multiple CPUs for the same bcm_op, can these lockless
updates to op->rx_stamp (a 64-bit ktime_t) and op->rx_ifindex cause data races?
If so, could this lead to torn writes on 32-bit platforms, causing corrupted
timestamps and interface indices to be sent to userspace?
> +
> + /* update statistics with lock analogue to bcm_rx_changed() callers */
> + bcm_update_stats(op);
> +
> rtr_frame = op->flags & RX_RTR_FRAME;
> if (rtr_frame)
> memcpy(&rtrframe, op->frames, op->cfsiz);
> +
> spin_unlock_bh(&op->bcm_rx_update_lock);
>
> if (rtr_frame) {
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-bcm_fixes-v6-0-80710a2bc398@hartkopp.net?part=4
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v6 5/9] can: bcm: add missing rcu list annotations and operations
2026-07-08 9:22 [PATCH v6 0/9] can: bcm: collected fixes - final Oliver Hartkopp via B4 Relay
` (3 preceding siblings ...)
2026-07-08 9:22 ` [PATCH v6 4/9] can: bcm: fix CAN frame rx/tx statistics Oliver Hartkopp via B4 Relay
@ 2026-07-08 9:22 ` Oliver Hartkopp via B4 Relay
2026-07-08 9:43 ` sashiko-bot
2026-07-08 9:22 ` [PATCH v6 6/9] can: bcm: extend bcm_tx_lock usage for data and timer updates Oliver Hartkopp via B4 Relay
` (3 subsequent siblings)
8 siblings, 1 reply; 17+ messages in thread
From: Oliver Hartkopp via B4 Relay @ 2026-07-08 9:22 UTC (permalink / raw)
To: linux-can; +Cc: Oliver Hartkopp, Marc Kleine-Budde, sashiko-reviews
From: Oliver Hartkopp <socketcan@hartkopp.net>
sashiko-bot remarked the missing use of list_add_rcu() in
bcm_[rx|tx]_setup() to have a proper initialized bcm_op structure
when bcm_proc_show() traverses the bcm_op's under rcu_read_lock().
To cover all initial settings of the bcm_op's the list_add_rcu() calls
are moved to the end of the setup code.
While at it, also fix the mirroring removal side: bcm_release() called
bcm_remove_op() - which frees the op via call_rcu() - on ops that were
still linked in bo->tx_ops/bo->rx_ops, without list_del_rcu() first.
Unlink each op with list_del_rcu() before handing it to bcm_remove_op(),
matching the existing pattern in bcm_delete_tx_op()/bcm_delete_rx_op().
Reported-by: sashiko-reviews@lists.linux.dev
Closes: https://lore.kernel.org/linux-can/20260610094654.A1FFE1F00893@smtp.kernel.org/
Fixes: dac5e6249159 ("can: bcm: add missing rcu read protection for procfs content")
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
net/can/bcm.c | 25 ++++++++++++++++---------
1 file changed, 16 insertions(+), 9 deletions(-)
diff --git a/net/can/bcm.c b/net/can/bcm.c
index 340b7fc89744..fca7c714cc52 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -263,11 +263,11 @@ static int bcm_proc_show(struct seq_file *m, void *v)
seq_printf(m, "%s%ld%%\n",
(reduction == 100) ? "near " : "", reduction);
}
- list_for_each_entry(op, &bo->tx_ops, list) {
+ list_for_each_entry_rcu(op, &bo->tx_ops, list) {
seq_printf(m, "tx_op: %03X %s ", op->can_id,
bcm_proc_getifname(net, ifname, op->ifindex));
if (op->flags & CAN_FD_FRAME)
@@ -997,10 +997,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;
+ bool add_op_to_list = false;
unsigned int i;
int err;
/* we need a real device to send frames */
if (!ifindex)
@@ -1138,12 +1139,11 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
/* currently unused in tx_ops */
hrtimer_setup(&op->thrtimer, hrtimer_dummy_timeout, CLOCK_MONOTONIC,
HRTIMER_MODE_REL_SOFT);
- /* add this bcm_op to the list of the tx_ops */
- list_add(&op->list, &bo->tx_ops);
+ add_op_to_list = true;
} /* if ((op = bcm_find_op(&bo->tx_ops, msg_head->can_id, ifindex))) */
if (op->flags & SETTIMER) {
/* set timer values */
@@ -1161,10 +1161,14 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
hrtimer_cancel(&op->timer);
/* spec: send CAN frame when starting timer */
op->flags |= TX_ANNOUNCE;
}
+ /* add this bcm_op to the list of the tx_ops? */
+ if (add_op_to_list)
+ list_add_rcu(&op->list, &bo->tx_ops);
+
if (op->flags & TX_ANNOUNCE)
bcm_can_tx(op, NULL);
if (op->flags & STARTTIMER)
bcm_tx_start_timer(op);
@@ -1345,13 +1349,10 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
hrtimer_setup(&op->timer, bcm_rx_timeout_handler, CLOCK_MONOTONIC,
HRTIMER_MODE_REL_SOFT);
hrtimer_setup(&op->thrtimer, bcm_rx_thr_handler, CLOCK_MONOTONIC,
HRTIMER_MODE_REL_SOFT);
- /* add this bcm_op to the list of the rx_ops */
- list_add(&op->list, &bo->rx_ops);
-
/* call can_rx_register() */
do_rx_register = 1;
} /* if ((op = bcm_find_op(&bo->rx_ops, msg_head->can_id, ifindex))) */
@@ -1419,14 +1420,16 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
err = can_rx_register(sock_net(sk), NULL, op->can_id,
REGMASK(op->can_id),
bcm_rx_handler, op, "bcm", sk);
if (err) {
/* this bcm rx op is broken -> remove it */
- list_del_rcu(&op->list);
bcm_remove_op(op);
return err;
}
+
+ /* add this bcm_op to the list of the rx_ops */
+ list_add_rcu(&op->list, &bo->rx_ops);
}
return msg_head->nframes * op->cfsiz + MHSIZ;
}
@@ -1756,12 +1759,14 @@ static int bcm_release(struct socket *sock)
/* remove procfs entry */
if (net->can.bcmproc_dir && bo->bcm_proc_read)
remove_proc_entry(bo->procname, net->can.bcmproc_dir);
#endif /* CONFIG_PROC_FS */
- list_for_each_entry_safe(op, next, &bo->tx_ops, list)
+ list_for_each_entry_safe(op, next, &bo->tx_ops, list) {
+ list_del_rcu(&op->list);
bcm_remove_op(op);
+ }
list_for_each_entry_safe(op, next, &bo->rx_ops, list) {
/*
* Don't care if we're bound or not (due to netdev problems)
* can_rx_unregister() is always a save thing to do here.
@@ -1788,12 +1793,14 @@ static int bcm_release(struct socket *sock)
}
synchronize_rcu();
- list_for_each_entry_safe(op, next, &bo->rx_ops, list)
+ list_for_each_entry_safe(op, next, &bo->rx_ops, list) {
+ list_del_rcu(&op->list);
bcm_remove_op(op);
+ }
/* remove device reference */
if (bo->bound) {
WRITE_ONCE(bo->bound, 0);
bo->ifindex = 0;
--
2.53.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v6 5/9] can: bcm: add missing rcu list annotations and operations
2026-07-08 9:22 ` [PATCH v6 5/9] can: bcm: add missing rcu list annotations and operations Oliver Hartkopp via B4 Relay
@ 2026-07-08 9:43 ` sashiko-bot
0 siblings, 0 replies; 17+ messages in thread
From: sashiko-bot @ 2026-07-08 9:43 UTC (permalink / raw)
To: Oliver Hartkopp
Cc: Marc Kleine-Budde, Oliver Hartkopp, Vincent Mailhol,
Oleksij Rempel, linux-can
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues:
- [Critical] bcm_tx_setup() updates CAN frames directly in the active op->frames buffer without locking and before validation, allowing a concurrent timer to transmit unvalidated, malformed frames.
- [Medium] bcm_tx_setup() updates 64-bit ktime_t timer variables without locking, risking torn reads on 32-bit systems.
- [Medium] Memory leak of CAN receiver object due to race between bcm_delete_rx_op() and NETDEV_UNREGISTER.
--
--- Patch [5]: [PATCH v6 5/9] can: bcm: add missing rcu list annotations and operations ---
commit f371f28179652ae60b7b0cd6b25f7f33429dc82a
Author: Oliver Hartkopp <socketcan@hartkopp.net>
can: bcm: add missing rcu list annotations and operations
This patch fixes missing list_add_rcu() calls in bcm_tx_setup() and
bcm_rx_setup() by moving them to the end of the setup code. It also fixes
bcm_release() to properly call list_del_rcu() before bcm_remove_op() when
removing operations.
Link: https://lore.kernel.org/linux-can/20260610094654.A1FFE1F00893@smtp.kernel.org/
> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index 340b7fc89744e..fca7c714cc52f 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -999,6 +999,7 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
> struct bcm_sock *bo = bcm_sk(sk);
> struct bcm_op *op;
> struct canfd_frame *cf;
> + bool add_op_to_list = false;
> unsigned int i;
> int err;
[Severity: Critical]
This is a pre-existing issue, but does bcm_tx_setup() update CAN frames
directly in the active op->frames buffer without locking and before
validation?
When updating an existing TX operation, user data is copied directly into
op->frames without holding bcm_tx_lock:
err = memcpy_from_msg((u8 *)cf, msg, op->cfsiz);
if (op->flags & CAN_FD_FRAME) {
if (cf->len > 64)
err = -EINVAL;
Since the operation is already active, a concurrent bcm_tx_timeout_handler()
could trigger and call bcm_can_tx(). The timer handler acquires bcm_tx_lock,
copies the torn/unvalidated frame, and transmits it:
memcpy(&cframe, op->frames + op->cfsiz * op->currframe, op->cfsiz);
cf = &cframe;
If an unprivileged user provides an invalid length, could
bcm_tx_timeout_handler() transmit it before bcm_tx_setup() rejects it,
leading to an out-of-bounds read of the kernel skb?
[Severity: Medium]
This is also a pre-existing issue, but are the 64-bit ktime_t timer
variables updated without lock protection in bcm_tx_setup()?
When the SETTIMER flag is provided, the variables are updated directly:
op->kt_ival1 = bcm_timeval_to_ktime(msg_head->ival1);
op->kt_ival2 = bcm_timeval_to_ktime(msg_head->ival2);
The timer handler reads these ktime_t values locklessly in
bcm_tx_set_expiry():
if (op->kt_ival1 && op->count)
ival = op->kt_ival1;
else if (op->kt_ival2)
ival = op->kt_ival2;
On 32-bit systems, could this result in a torn read of the 64-bit ktime_t
values, causing bogus timer expiration times?
[ ... ]
> @@ -1790,8 +1795,10 @@ static int bcm_release(struct socket *sock)
>
> synchronize_rcu();
>
> - list_for_each_entry_safe(op, next, &bo->rx_ops, list)
> + list_for_each_entry_safe(op, next, &bo->rx_ops, list) {
> + list_del_rcu(&op->list);
> bcm_remove_op(op);
> + }
>
> /* remove device reference */
> if (bo->bound) {
[Severity: Medium]
This is a pre-existing issue, but is there a memory leak of the CAN
receiver object due to a race between bcm_delete_rx_op() and
NETDEV_UNREGISTER?
If bcm_delete_rx_op() races with unregister_netdevice(), dev_get_by_index()
returns NULL:
dev = dev_get_by_index(sock_net(op->sk), op->ifindex);
if (dev) {
bcm_rx_unreg(dev, op);
dev_put(dev);
}
The op is then removed from the list via list_del_rcu(&op->list):
list_del_rcu(&op->list);
bcm_remove_op(op);
Making it invisible to the subsequent NETDEV_UNREGISTER event handler in
bcm_notifier(). Since bcm_rx_unreg() is skipped in both places, does this
permanently leak the receiver object allocated in can_rx_register()?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-bcm_fixes-v6-0-80710a2bc398@hartkopp.net?part=5
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v6 6/9] can: bcm: extend bcm_tx_lock usage for data and timer updates
2026-07-08 9:22 [PATCH v6 0/9] can: bcm: collected fixes - final Oliver Hartkopp via B4 Relay
` (4 preceding siblings ...)
2026-07-08 9:22 ` [PATCH v6 5/9] can: bcm: add missing rcu list annotations and operations Oliver Hartkopp via B4 Relay
@ 2026-07-08 9:22 ` Oliver Hartkopp via B4 Relay
2026-07-08 9:22 ` [PATCH v6 7/9] can: bcm: validate frame length in bcm_rx_setup() for RTR replies Oliver Hartkopp via B4 Relay
` (2 subsequent siblings)
8 siblings, 0 replies; 17+ messages in thread
From: Oliver Hartkopp via B4 Relay @ 2026-07-08 9:22 UTC (permalink / raw)
To: linux-can; +Cc: Oliver Hartkopp, Marc Kleine-Budde
From: Oliver Hartkopp <socketcan@hartkopp.net>
Stage new CAN frame content for an existing tx op into a kmalloc()'d
buffer and validate it there, mirroring the approach already used in
bcm_rx_setup(). Only copy the validated data into op->frames while
holding op->bcm_tx_lock, so bcm_can_tx() and bcm_tx_timeout_handler()
can no longer observe a partially updated or unvalidated frame.
Add a missing error path for memcpy_from_msg() when copying CAN frame
data from userspace.
Also move the kt_ival1/kt_ival2/ival1/ival2 updates in bcm_tx_setup()
under op->bcm_tx_lock, and read kt_ival1/kt_ival2/count under the same
lock in bcm_tx_set_expiry() and bcm_tx_timeout_handler(), closing the
torn 64-bit ktime_t read on 32-bit platforms.
Fixes: c2aba69d0c36 ("can: bcm: add locking for bcm_op runtime updates")
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
net/can/bcm.c | 81 +++++++++++++++++++++++++++++++++++++++++------------------
1 file changed, 57 insertions(+), 24 deletions(-)
diff --git a/net/can/bcm.c b/net/can/bcm.c
index fca7c714cc52..fefa77045253 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -126,11 +126,11 @@ struct bcm_op {
void *last_frames;
struct canfd_frame sframe;
struct canfd_frame last_sframe;
struct sock *sk;
struct net_device *rx_reg_dev;
- spinlock_t bcm_tx_lock; /* protect currframe/count in runtime updates */
+ 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;
@@ -453,16 +453,22 @@ static void bcm_send_to_user(struct bcm_op *op, struct bcm_msg_head *head,
static bool bcm_tx_set_expiry(struct bcm_op *op, struct hrtimer *hrt)
{
ktime_t ival;
+ spin_lock_bh(&op->bcm_tx_lock);
+
if (op->kt_ival1 && op->count)
ival = op->kt_ival1;
- else if (op->kt_ival2)
+ else if (op->kt_ival2) {
ival = op->kt_ival2;
- else
+ } else {
+ spin_unlock_bh(&op->bcm_tx_lock);
return false;
+ }
+
+ spin_unlock_bh(&op->bcm_tx_lock);
hrtimer_set_expires(hrt, ktime_add(ktime_get(), ival));
return true;
}
@@ -475,12 +481,21 @@ static void bcm_tx_start_timer(struct bcm_op *op)
/* bcm_tx_timeout_handler - performs cyclic CAN frame transmissions */
static enum hrtimer_restart bcm_tx_timeout_handler(struct hrtimer *hrtimer)
{
struct bcm_op *op = container_of(hrtimer, struct bcm_op, timer);
struct bcm_msg_head msg_head;
+ bool tx_ival1, tx_ival2;
- if (op->kt_ival1 && (op->count > 0)) {
+ /* snapshot kt_ival1/kt_ival2/count under lock to avoid torn
+ * ktime_t reads racing with concurrent bcm_tx_setup() updates
+ */
+ spin_lock_bh(&op->bcm_tx_lock);
+ tx_ival1 = op->kt_ival1 && (op->count > 0);
+ tx_ival2 = !!op->kt_ival2;
+ spin_unlock_bh(&op->bcm_tx_lock);
+
+ if (tx_ival1) {
bcm_can_tx(op, NULL);
if (!op->count && (op->flags & TX_COUNTEVT)) {
/* create notification to user */
memset(&msg_head, 0, sizeof(msg_head));
@@ -493,11 +508,11 @@ static enum hrtimer_restart bcm_tx_timeout_handler(struct hrtimer *hrtimer)
msg_head.nframes = 0;
bcm_send_to_user(op, &msg_head, NULL, 0);
}
- } else if (op->kt_ival2) {
+ } else if (tx_ival2) {
bcm_can_tx(op, NULL);
}
return bcm_tx_set_expiry(op, &op->timer) ?
HRTIMER_RESTART : HRTIMER_NORESTART;
@@ -1016,64 +1031,80 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
return -EINVAL;
/* check the given can_id */
op = bcm_find_op(&bo->tx_ops, msg_head, ifindex);
if (op) {
+ void *new_frames;
+
/* update existing BCM operation */
/*
* Do we need more space for the CAN frames than currently
* allocated? -> This is a _really_ unusual use-case and
* therefore (complexity / locking) it is not supported.
*/
if (msg_head->nframes > op->nframes)
return -E2BIG;
- /* update CAN frames content */
+ /* 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
+ */
+ new_frames = kmalloc(msg_head->nframes * op->cfsiz, GFP_KERNEL);
+ if (!new_frames)
+ return -ENOMEM;
+
for (i = 0; i < msg_head->nframes; i++) {
- cf = op->frames + op->cfsiz * i;
+ cf = new_frames + op->cfsiz * i;
err = memcpy_from_msg((u8 *)cf, msg, op->cfsiz);
+ if (err < 0) {
+ kfree(new_frames);
+ return err;
+ }
if (op->flags & CAN_FD_FRAME) {
if (cf->len > 64)
err = -EINVAL;
} else {
if (cf->len > 8)
err = -EINVAL;
}
- if (err < 0)
+ if (err < 0) {
+ kfree(new_frames);
return err;
+ }
if (msg_head->flags & TX_CP_CAN_ID) {
/* copy can_id into frame */
cf->can_id = msg_head->can_id;
}
}
+
+ spin_lock_bh(&op->bcm_tx_lock);
+
+ /* update CAN frames content */
+ memcpy(op->frames, new_frames, msg_head->nframes * op->cfsiz);
+
op->flags = msg_head->flags;
- /* only lock for unlikely count/nframes/currframe changes */
if (op->nframes != msg_head->nframes ||
- op->flags & TX_RESET_MULTI_IDX ||
- op->flags & SETTIMER) {
-
- spin_lock_bh(&op->bcm_tx_lock);
+ op->flags & TX_RESET_MULTI_IDX) {
+ /* potentially update changed nframes */
+ op->nframes = msg_head->nframes;
+ /* restart multiple frame transmission */
+ op->currframe = 0;
+ }
- if (op->nframes != msg_head->nframes ||
- op->flags & TX_RESET_MULTI_IDX) {
- /* potentially update changed nframes */
- op->nframes = msg_head->nframes;
- /* restart multiple frame transmission */
- op->currframe = 0;
- }
+ if (op->flags & SETTIMER)
+ op->count = msg_head->count;
- if (op->flags & SETTIMER)
- op->count = msg_head->count;
+ spin_unlock_bh(&op->bcm_tx_lock);
- spin_unlock_bh(&op->bcm_tx_lock);
- }
+ kfree(new_frames);
} else {
/* insert new BCM operation for the given can_id */
op = kzalloc(OPSIZ, GFP_KERNEL);
@@ -1145,14 +1176,16 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
} /* if ((op = bcm_find_op(&bo->tx_ops, msg_head->can_id, ifindex))) */
if (op->flags & SETTIMER) {
/* set timer values */
+ spin_lock_bh(&op->bcm_tx_lock);
op->ival1 = msg_head->ival1;
op->ival2 = msg_head->ival2;
op->kt_ival1 = bcm_timeval_to_ktime(msg_head->ival1);
op->kt_ival2 = bcm_timeval_to_ktime(msg_head->ival2);
+ spin_unlock_bh(&op->bcm_tx_lock);
/* disable an active timer due to zero values? */
if (!op->kt_ival1 && !op->kt_ival2)
hrtimer_cancel(&op->timer);
}
--
2.53.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v6 7/9] can: bcm: validate frame length in bcm_rx_setup() for RTR replies
2026-07-08 9:22 [PATCH v6 0/9] can: bcm: collected fixes - final Oliver Hartkopp via B4 Relay
` (5 preceding siblings ...)
2026-07-08 9:22 ` [PATCH v6 6/9] can: bcm: extend bcm_tx_lock usage for data and timer updates Oliver Hartkopp via B4 Relay
@ 2026-07-08 9:22 ` Oliver Hartkopp via B4 Relay
2026-07-08 9:22 ` [PATCH v6 8/9] can: bcm: add missing device refcount for CAN filter removal Oliver Hartkopp via B4 Relay
2026-07-08 9:22 ` [PATCH v6 9/9] can: bcm: fix data race on rx_stamp/rx_ifindex in bcm_rx_handler() Oliver Hartkopp via B4 Relay
8 siblings, 0 replies; 17+ messages in thread
From: Oliver Hartkopp via B4 Relay @ 2026-07-08 9:22 UTC (permalink / raw)
To: linux-can; +Cc: Oliver Hartkopp, Marc Kleine-Budde
From: Oliver Hartkopp <socketcan@hartkopp.net>
bcm_tx_setup() validates cf->len against the CAN/CAN FD DLC limits
before installing frames for TX_SETUP, but bcm_rx_setup() never did
the same for the RTR-reply frame configured via RX_SETUP with
RX_RTR_FRAME.
Fixes: ffd980f976e7 ("[CAN]: Add broadcast manager (bcm) protocol")
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
net/can/bcm.c | 59 +++++++++++++++++++++++++++++++++++++++++------------------
1 file changed, 41 insertions(+), 18 deletions(-)
diff --git a/net/can/bcm.c b/net/can/bcm.c
index fefa77045253..300f1f88c4d2 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -1213,26 +1213,41 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
kfree(op->frames);
kfree(op);
return err;
}
-static void bcm_rx_setup_rtr_check(struct bcm_msg_head *msg_head,
- struct bcm_op *op, void *new_frames)
+static int bcm_rx_setup_rtr_check(struct bcm_msg_head *msg_head,
+ struct bcm_op *op, void *new_frames)
{
+ struct canfd_frame *frame0 = new_frames;
+
+ if (!(msg_head->flags & RX_RTR_FRAME))
+ return 0;
+
+ /* this frame is sent out as-is by bcm_can_tx() whenever a matching
+ * remote request is received, so validate its length the same way
+ * bcm_tx_setup() validates TX_SETUP frames before installing it
+ */
+ if (msg_head->flags & CAN_FD_FRAME) {
+ if (frame0->len > 64)
+ return -EINVAL;
+ } else {
+ if (frame0->len > 8)
+ return -EINVAL;
+ }
+
/* funny feature in RX(!)_SETUP only for RTR-mode:
* copy can_id into frame BUT without RTR-flag to
* prevent a full-load-loopback-test ... ;-]
* normalize this on the staged buffer, before it is
* ever installed into op->frames.
*/
- if (msg_head->flags & RX_RTR_FRAME) {
- struct canfd_frame *frame0 = new_frames;
+ if ((msg_head->flags & TX_CP_CAN_ID) ||
+ frame0->can_id == op->can_id)
+ frame0->can_id = op->can_id & ~CAN_RTR_FLAG;
- if ((msg_head->flags & TX_CP_CAN_ID) ||
- frame0->can_id == op->can_id)
- frame0->can_id = op->can_id & ~CAN_RTR_FLAG;
- }
+ return 0;
}
/*
* bcm_rx_setup - create or update a bcm rx op (for bcm_sendmsg)
*/
@@ -1291,11 +1306,15 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
if (err < 0) {
kfree(new_frames);
return err;
}
- bcm_rx_setup_rtr_check(msg_head, op, new_frames);
+ err = bcm_rx_setup_rtr_check(msg_head, op, new_frames);
+ if (err < 0) {
+ kfree(new_frames);
+ return err;
+ }
}
spin_lock_bh(&op->bcm_rx_update_lock);
op->nframes = msg_head->nframes;
op->flags = msg_head->flags;
@@ -1356,20 +1375,16 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
}
if (msg_head->nframes) {
err = memcpy_from_msg(op->frames, msg,
msg_head->nframes * op->cfsiz);
- if (err < 0) {
- if (op->frames != &op->sframe)
- kfree(op->frames);
- if (op->last_frames != &op->last_sframe)
- kfree(op->last_frames);
- kfree(op);
- return err;
- }
+ if (err < 0)
+ goto free_op;
- bcm_rx_setup_rtr_check(msg_head, op, op->frames);
+ err = bcm_rx_setup_rtr_check(msg_head, op, op->frames);
+ if (err < 0)
+ goto free_op;
}
/* bcm_can_tx / bcm_tx_timeout_handler needs this */
op->sk = sk;
sock_hold(sk);
@@ -1462,10 +1477,18 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
/* add this bcm_op to the list of the rx_ops */
list_add_rcu(&op->list, &bo->rx_ops);
}
return msg_head->nframes * op->cfsiz + MHSIZ;
+
+free_op:
+ if (op->frames != &op->sframe)
+ kfree(op->frames);
+ if (op->last_frames != &op->last_sframe)
+ kfree(op->last_frames);
+ kfree(op);
+ return err;
}
/*
* bcm_tx_send - send a single CAN frame to the CAN interface (for bcm_sendmsg)
*/
--
2.53.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v6 8/9] can: bcm: add missing device refcount for CAN filter removal
2026-07-08 9:22 [PATCH v6 0/9] can: bcm: collected fixes - final Oliver Hartkopp via B4 Relay
` (6 preceding siblings ...)
2026-07-08 9:22 ` [PATCH v6 7/9] can: bcm: validate frame length in bcm_rx_setup() for RTR replies Oliver Hartkopp via B4 Relay
@ 2026-07-08 9:22 ` Oliver Hartkopp via B4 Relay
2026-07-08 9:45 ` sashiko-bot
2026-07-08 9:22 ` [PATCH v6 9/9] can: bcm: fix data race on rx_stamp/rx_ifindex in bcm_rx_handler() Oliver Hartkopp via B4 Relay
8 siblings, 1 reply; 17+ messages in thread
From: Oliver Hartkopp via B4 Relay @ 2026-07-08 9:22 UTC (permalink / raw)
To: linux-can; +Cc: Oliver Hartkopp, Marc Kleine-Budde, sashiko-bot
From: Oliver Hartkopp <socketcan@hartkopp.net>
sashiko-bot remarked a problem with a concurrent device unregistration
in isotp.c which also is present in the bcm.c code. A former fix for raw.c
commit c275a176e4b6 ("can: raw: add missing refcount for memory leak fix")
introduced a netdevice_tracker which solves the issue for bcm.c too.
bcm_release(), bcm_delete_rx_op() and bcm_notifier() relied on
dev_get_by_index(ifindex) to re-find the device for an rx_op before
unregistering its filter. If a concurrent NETDEV_UNREGISTER has already
unlisted the device from the ifindex table, that lookup fails and
can_rx_unregister() is silently skipped, leaving a stale CAN filter
pointing at the soon-to-be-freed bcm_op/socket.
Hold a netdev_hold()/netdev_put() tracked reference on op->rx_reg_dev
from the moment the rx filter is registered in bcm_rx_setup() until it
is unregistered in bcm_rx_unreg(), and use that reference directly in
bcm_release() and bcm_delete_rx_op() instead of re-looking the device
up by ifindex.
Reported-by: sashiko-bot@kernel.org
Link: https://sashiko.dev/#/patchset/20260707094716.63578-1-socketcan@hartkopp.net
Fixes: ffd980f976e7 ("[CAN]: Add broadcast manager (bcm) protocol")
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
net/can/bcm.c | 47 +++++++++++++++++++++++++++--------------------
1 file changed, 27 insertions(+), 20 deletions(-)
diff --git a/net/can/bcm.c b/net/can/bcm.c
index 300f1f88c4d2..4bdca72f9327 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -126,10 +126,11 @@ struct bcm_op {
void *last_frames;
struct canfd_frame sframe;
struct canfd_frame last_sframe;
struct sock *sk;
struct net_device *rx_reg_dev;
+ netdevice_tracker rx_reg_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 {
@@ -904,10 +905,11 @@ static void bcm_rx_unreg(struct net_device *dev, struct bcm_op *op)
can_rx_unregister(dev_net(dev), dev, op->can_id,
REGMASK(op->can_id), bcm_rx_handler, op);
/* mark as removed subscription */
op->rx_reg_dev = NULL;
+ netdev_put(dev, &op->rx_reg_dev_tracker);
} else
printk(KERN_ERR "can-bcm: bcm_rx_unreg: registered device "
"mismatch %p %p\n", op->rx_reg_dev, dev);
}
@@ -934,21 +936,18 @@ static int bcm_delete_rx_op(struct list_head *ops, struct bcm_msg_head *mh,
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.
*/
- if (op->rx_reg_dev) {
- struct net_device *dev;
-
- dev = dev_get_by_index(sock_net(op->sk),
- op->ifindex);
- if (dev) {
- bcm_rx_unreg(dev, op);
- dev_put(dev);
- }
- }
+ if (op->rx_reg_dev)
+ bcm_rx_unreg(op->rx_reg_dev, op);
} else
can_rx_unregister(sock_net(op->sk), NULL,
op->can_id,
REGMASK(op->can_id),
bcm_rx_handler, op);
@@ -1453,11 +1452,21 @@ 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);
- op->rx_reg_dev = dev;
+ /* keep a tracked reference so that a later
+ * unregister can safely reach the device even
+ * if a concurrent NETDEV_UNREGISTER has
+ * already unlisted it by ifindex
+ */
+ if (!err) {
+ op->rx_reg_dev = dev;
+ netdev_hold(dev,
+ &op->rx_reg_dev_tracker,
+ GFP_KERNEL);
+ }
dev_put(dev);
} else {
/* the requested device is gone - do not
* silently succeed without registering
*/
@@ -1830,20 +1839,18 @@ static int bcm_release(struct socket *sock)
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.
*/
- if (op->rx_reg_dev) {
- struct net_device *dev;
-
- dev = dev_get_by_index(net, op->ifindex);
- if (dev) {
- bcm_rx_unreg(dev, op);
- dev_put(dev);
- }
- }
+ if (op->rx_reg_dev)
+ bcm_rx_unreg(op->rx_reg_dev, op);
} else
can_rx_unregister(net, NULL, op->can_id,
REGMASK(op->can_id),
bcm_rx_handler, op);
--
2.53.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v6 8/9] can: bcm: add missing device refcount for CAN filter removal
2026-07-08 9:22 ` [PATCH v6 8/9] can: bcm: add missing device refcount for CAN filter removal Oliver Hartkopp via B4 Relay
@ 2026-07-08 9:45 ` sashiko-bot
0 siblings, 0 replies; 17+ messages in thread
From: sashiko-bot @ 2026-07-08 9:45 UTC (permalink / raw)
To: Oliver Hartkopp
Cc: Marc Kleine-Budde, Vincent Mailhol, Oleksij Rempel,
Oliver Hartkopp, linux-can
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] BCM TX operations (tx_ops) are not cleaned up during NETDEV_UNREGISTER, causing continuous timer firing and potential cross-device traffic injection upon ifindex reuse.
- [Medium] Updating an existing BCM RX filter after device recreation silently fails to re-register the CAN receiver.
--
--- Patch [8]: [PATCH v6 8/9] can: bcm: add missing device refcount for CAN filter removal ---
commit e5a4c52c71d52a19d27614862f641c4332b115e5
Author: Oliver Hartkopp <socketcan@hartkopp.net>
can: bcm: add missing device refcount for CAN filter removal
This patch fixes a CAN filter removal issue where a concurrent
NETDEV_UNREGISTER can cause a lookup failure for a device by ifindex.
It prevents skipping can_rx_unregister() by holding a netdev reference on
op->rx_reg_dev from filter registration until unregistration, rather than
looking up the device by index again.
Link: https://sashiko.dev/#/patchset/20260707094716.63578-1-socketcan@hartkopp.net
> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index 300f1f88c4d2c..4bdca72f9327f 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
[ ... ]
> @@ -1455,7 +1454,17 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
> bcm_rx_handler, op,
> "bcm", sk);
>
> - op->rx_reg_dev = dev;
> + /* keep a tracked reference so that a later
> + * unregister can safely reach the device even
> + * if a concurrent NETDEV_UNREGISTER has
> + * already unlisted it by ifindex
> + */
> + if (!err) {
> + op->rx_reg_dev = dev;
> + netdev_hold(dev,
> + &op->rx_reg_dev_tracker,
> + GFP_KERNEL);
> + }
> dev_put(dev);
[Severity: Medium]
This is a pre-existing issue, but does updating an existing BCM RX filter
after a device recreation silently fail to re-register the CAN receiver?
Looking at bcm_rx_setup():
op = bcm_find_op(&bo->rx_ops, msg_head, ifindex);
if (op) {
...
/* Only an update -> do not call can_rx_register() */
do_rx_register = 0;
Updates to existing ops unconditionally skip CAN registration, even if the
previous registration was torn down by a NETDEV_UNREGISTER event leaving
op->rx_reg_dev as NULL.
Could this cause the application to silently drop incoming frames?
[ ... ]
> @@ -1832,16 +1841,14 @@ static int bcm_release(struct socket *sock)
> * 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.
> */
> - if (op->rx_reg_dev) {
> - struct net_device *dev;
> -
> - dev = dev_get_by_index(net, op->ifindex);
> - if (dev) {
> - bcm_rx_unreg(dev, op);
> - dev_put(dev);
> - }
> - }
> + if (op->rx_reg_dev)
> + bcm_rx_unreg(op->rx_reg_dev, op);
[Severity: High]
This isn't a bug introduced by this patch, but are TX operations properly
cleaned up during NETDEV_UNREGISTER?
When looking at bcm_notify():
case NETDEV_UNREGISTER:
lock_sock(sk);
/* 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);
Only rx_ops are cleaned up upon NETDEV_UNREGISTER. The tx_ops appear to be
completely ignored, and op->ifindex is never cleared to halt transmissions.
This seems to allow the timer to fire endlessly in bcm_tx_timeout_handler():
} else if (tx_ival2) {
bcm_can_tx(op, NULL);
}
return bcm_tx_set_expiry(op, &op->timer) ?
HRTIMER_RESTART : HRTIMER_NORESTART;
The timer handler continues to restart the timer even when the underlying
device has been destroyed. In bcm_can_tx(), dev_get_by_index() silently fails,
allowing the loop to continue endlessly.
Could this lead to cross-device traffic injection if the removed ifindex is
later reused by a new CAN interface?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-bcm_fixes-v6-0-80710a2bc398@hartkopp.net?part=8
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v6 9/9] can: bcm: fix data race on rx_stamp/rx_ifindex in bcm_rx_handler()
2026-07-08 9:22 [PATCH v6 0/9] can: bcm: collected fixes - final Oliver Hartkopp via B4 Relay
` (7 preceding siblings ...)
2026-07-08 9:22 ` [PATCH v6 8/9] can: bcm: add missing device refcount for CAN filter removal Oliver Hartkopp via B4 Relay
@ 2026-07-08 9:22 ` Oliver Hartkopp via B4 Relay
2026-07-08 9:49 ` sashiko-bot
8 siblings, 1 reply; 17+ messages in thread
From: Oliver Hartkopp via B4 Relay @ 2026-07-08 9:22 UTC (permalink / raw)
To: linux-can; +Cc: Oliver Hartkopp, Marc Kleine-Budde, sashiko-bot
From: Oliver Hartkopp <socketcan@hartkopp.net>
For an rx op subscribed on all interfaces (ifindex == 0), the same op
is registered once in the shared per-netns wildcard filter list, so
bcm_rx_handler() can run concurrently on different CPUs for frames
arriving on different net devices.
op->rx_stamp and op->rx_ifindex were written before bcm_rx_update_lock was
taken, allowing concurrent writers to race each other - including a torn
store of the 64-bit rx_stamp on 32-bit platforms.
Beyond a torn store bcm_send_to_user() must report the timestamp/ifindex
of the very same frame whose content it is delivering. So the assignment
is placed in the same unbroken bcm_rx_update_lock section as the content
comparison.
As a side effect, the RTR-request frame feature (which never reach
bcm_send_to_user()) no longer updates rx_stamp/rx_ifindex, since only
the notification path needs them.
Fixes: ffd980f976e7 ("[CAN]: Add broadcast manager (bcm) protocol")
Reported-by: sashiko-bot@kernel.org
Link: https://lore.kernel.org/linux-can/20260707145135.5BC831F00A3A@smtp.kernel.org/
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
net/can/bcm.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/net/can/bcm.c b/net/can/bcm.c
index 4bdca72f9327..73e3a5fc4342 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -768,15 +768,10 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data)
}
/* disable timeout */
hrtimer_cancel(&op->timer);
- /* save rx timestamp */
- op->rx_stamp = skb->tstamp;
- /* save originator for recvfrom() */
- op->rx_ifindex = skb->dev->ifindex;
-
/* snapshot RTR content under lock: op->flags/op->frames may be
* updated concurrently by bcm_rx_setup().
*/
spin_lock_bh(&op->bcm_rx_update_lock);
@@ -803,10 +798,18 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data)
traffic_flags |= RX_OWN;
}
spin_lock_bh(&op->bcm_rx_update_lock);
+ /* save rx timestamp and originator for recvfrom() under lock.
+ * For an op subscribed on all interfaces (ifindex == 0)
+ * bcm_rx_handler() can run concurrently on different CPUs so
+ * the CAN content and the meta data must be bundled correctly.
+ */
+ op->rx_stamp = skb->tstamp;
+ op->rx_ifindex = skb->dev->ifindex;
+
if (op->flags & RX_FILTER_ID) {
/* the easiest case */
bcm_rx_update_and_send(op, op->last_frames, rxframe,
traffic_flags);
goto rx_starttimer;
--
2.53.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v6 9/9] can: bcm: fix data race on rx_stamp/rx_ifindex in bcm_rx_handler()
2026-07-08 9:22 ` [PATCH v6 9/9] can: bcm: fix data race on rx_stamp/rx_ifindex in bcm_rx_handler() Oliver Hartkopp via B4 Relay
@ 2026-07-08 9:49 ` sashiko-bot
0 siblings, 0 replies; 17+ messages in thread
From: sashiko-bot @ 2026-07-08 9:49 UTC (permalink / raw)
To: Oliver Hartkopp
Cc: Vincent Mailhol, Marc Kleine-Budde, Oliver Hartkopp,
Oleksij Rempel, linux-can
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Ignored CAN frames incorrectly overwrite the `rx_stamp` and `rx_ifindex` of pending throttled frames, leading to incorrect metadata delivery to userspace.
--
--- Patch [9]: [PATCH v6 9/9] can: bcm: fix data race on rx_stamp/rx_ifindex in bcm_rx_handler() ---
commit 1f01914b41bcf86b5db9980430a19a39e9075e09
Author: Oliver Hartkopp <socketcan@hartkopp.net>
can: bcm: fix data race on rx_stamp/rx_ifindex in bcm_rx_handler()
This commit fixes a data race on op->rx_stamp and op->rx_ifindex by moving
their assignment inside the bcm_rx_update_lock critical section. It also
stops updating them for RTR-request frames.
Link: https://lore.kernel.org/linux-can/20260707145135.5BC831F00A3A@smtp.kernel.org/
> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index 4bdca72f9327f..73e3a5fc4342f 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
[ ... ]
> @@ -805,6 +800,14 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data)
>
> spin_lock_bh(&op->bcm_rx_update_lock);
>
> + /* save rx timestamp and originator for recvfrom() under lock.
> + * For an op subscribed on all interfaces (ifindex == 0)
> + * bcm_rx_handler() can run concurrently on different CPUs so
> + * the CAN content and the meta data must be bundled correctly.
> + */
> + op->rx_stamp = skb->tstamp;
> + op->rx_ifindex = skb->dev->ifindex;
[Severity: High]
This is a pre-existing issue, but does unconditionally overwriting these
fields corrupt the metadata for pending throttled frames?
If a user configures a BCM socket with a filter (e.g., multiplex nframes > 1),
bound to all interfaces (ifindex == 0), with a throttle timer:
1. A frame arrives on vcan0, matches a mask, and is throttled.
op->rx_ifindex becomes vcan0.
2. A subsequent frame arrives on vcan1 but does not match any multiplex mask.
Since bcm_rx_handler() unconditionally updates op->rx_ifindex here before
applying the filters, doesn't it overwrite the metadata with vcan1 even
though the frame is ignored?
When the throttle timer expires, bcm_send_to_user() will use this corrupted
op->rx_ifindex to deliver the throttled frame from vcan0, making it appear as
if it came from vcan1.
Also, since the bcm_op struct only has one rx_stamp and rx_ifindex field,
doesn't this mean it structurally lacks the capacity to store independent
metadata per multiplex mask if multiple multiplexed frames are throttled
simultaneously?
> +
> if (op->flags & RX_FILTER_ID) {
> /* the easiest case */
> bcm_rx_update_and_send(op, op->last_frames, rxframe,
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-bcm_fixes-v6-0-80710a2bc398@hartkopp.net?part=9
^ permalink raw reply [flat|nested] 17+ messages in thread