* [PATCH v13 01/11] can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF
2026-07-14 10:12 [PATCH v13 00/11] can: bcm: collected fixes - final final Oliver Hartkopp via B4 Relay
@ 2026-07-14 10:12 ` Oliver Hartkopp via B4 Relay
2026-07-14 10:39 ` sashiko-bot
2026-07-14 10:12 ` [PATCH v13 02/11] can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure Oliver Hartkopp via B4 Relay
` (9 subsequent siblings)
10 siblings, 1 reply; 32+ messages in thread
From: Oliver Hartkopp via B4 Relay @ 2026-07-14 10:12 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] 32+ messages in thread* Re: [PATCH v13 01/11] can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF
2026-07-14 10:12 ` [PATCH v13 01/11] can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF Oliver Hartkopp via B4 Relay
@ 2026-07-14 10:39 ` sashiko-bot
2026-07-14 10:42 ` Oliver Hartkopp
0 siblings, 1 reply; 32+ messages in thread
From: sashiko-bot @ 2026-07-14 10:39 UTC (permalink / raw)
To: Oliver Hartkopp
Cc: Marc Kleine-Budde, Oleksij Rempel, Vincent Mailhol,
Oliver Hartkopp, linux-can
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [High] Module exit sequence destroys the socket slab cache before flushing the deferred workqueue, causing a kernel warning and slab cache leak.
Pre-existing issues:
- [High] Missing RCU-safe list primitives for rx_ops and tx_ops list modifications and traversals, causing potential crashes during procfs reads.
--
--- Patch [1]: [PATCH v13 01/11] can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF ---
commit 9beb61cd8491cca022797eead8556214843e4d99
Author: Lee Jones <lee@kernel.org>
can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF
This commit fixes a use-after-free by deferring the deallocation of
bcm_op to a dedicated workqueue instead of using call_rcu(). It ensures
that hrtimer cancellation and memory freeing occur in a sleepable context.
It also takes a reference on the socket while the work is pending to
prevent the socket from being freed prematurely.
> 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;
>
> /* initialize uninitialized (kzalloc) structure */
[Severity: High]
This is a pre-existing issue, but can traversing the RCU-protected tx_ops
and rx_ops lists without RCU-safe primitives cause crashes during procfs
reads?
Looking at bcm_tx_setup() and bcm_rx_setup(), list_add() is used instead
of list_add_rcu(), which lacks the smp_store_release() memory barriers:
bcm_tx_setup() {
...
/* add this bcm_op to the list of the tx_ops */
list_add(&op->list, &bo->tx_ops);
...
}
Concurrently, if another thread reads /proc/net/can-bcm, bcm_proc_show()
traverses bo->tx_ops locklessly under rcu_read_lock() using the non-RCU
list_for_each_entry():
bcm_proc_show() {
...
list_for_each_entry(op, &bo->tx_ops, list) {
...
}
Can weakly ordered CPUs reorder the initialization of the bcm_op structure
after its insertion, allowing lockless RCU readers to observe uninitialized
memory and crash?
[ ... ]
> @@ -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]
Does this module exit sequence destroy the socket slab cache before flushing
the deferred workqueue?
Looking at bcm_free_op_work(), the deferred work item drops the final socket
reference:
bcm_free_op_work() {
...
sock_put(op->sk);
kfree(op);
}
If there are pending work items in bcm_wq during module unload,
can_proto_unregister() will call kmem_cache_destroy() on the protocol's
slab cache while sockets are still allocated.
Will this cause a kernel warning and a slab cache leak, since
destroy_workqueue() runs after can_proto_unregister() and will attempt to
free sockets into the destroyed slab?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260714-bcm_fixes-v13-0-fd667c61099a@hartkopp.net?part=1
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH v13 01/11] can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF
2026-07-14 10:39 ` sashiko-bot
@ 2026-07-14 10:42 ` Oliver Hartkopp
0 siblings, 0 replies; 32+ messages in thread
From: Oliver Hartkopp @ 2026-07-14 10:42 UTC (permalink / raw)
To: sashiko-reviews
Cc: Marc Kleine-Budde, Oleksij Rempel, Vincent Mailhol, linux-can
On 14.07.26 12:39, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> New issues:
> - [High] Module exit sequence destroys the socket slab cache before flushing the deferred workqueue, causing a kernel warning and slab cache leak.
>
Fixed later in patch 5.
> Pre-existing issues:
> - [High] Missing RCU-safe list primitives for rx_ops and tx_ops list modifications and traversals, causing potential crashes during procfs reads.
> --
>
> --- Patch [1]: [PATCH v13 01/11] can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF ---
> commit 9beb61cd8491cca022797eead8556214843e4d99
> Author: Lee Jones <lee@kernel.org>
>
> can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF
>
> This commit fixes a use-after-free by deferring the deallocation of
> bcm_op to a dedicated workqueue instead of using call_rcu(). It ensures
> that hrtimer cancellation and memory freeing occur in a sleepable context.
> It also takes a reference on the socket while the work is pending to
> prevent the socket from being freed prematurely.
>
>> 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;
>>
>> /* initialize uninitialized (kzalloc) structure */
>
> [Severity: High]
> This is a pre-existing issue, but can traversing the RCU-protected tx_ops
> and rx_ops lists without RCU-safe primitives cause crashes during procfs
> reads?
>
> Looking at bcm_tx_setup() and bcm_rx_setup(), list_add() is used instead
> of list_add_rcu(), which lacks the smp_store_release() memory barriers:
>
> bcm_tx_setup() {
> ...
> /* add this bcm_op to the list of the tx_ops */
> list_add(&op->list, &bo->tx_ops);
> ...
> }
>
> Concurrently, if another thread reads /proc/net/can-bcm, bcm_proc_show()
> traverses bo->tx_ops locklessly under rcu_read_lock() using the non-RCU
> list_for_each_entry():
>
> bcm_proc_show() {
> ...
> list_for_each_entry(op, &bo->tx_ops, list) {
> ...
> }
>
> Can weakly ordered CPUs reorder the initialization of the bcm_op structure
> after its insertion, allowing lockless RCU readers to observe uninitialized
> memory and crash?
>
> [ ... ]
>
>> @@ -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]
> Does this module exit sequence destroy the socket slab cache before flushing
> the deferred workqueue?
>
> Looking at bcm_free_op_work(), the deferred work item drops the final socket
> reference:
>
> bcm_free_op_work() {
> ...
> sock_put(op->sk);
> kfree(op);
> }
>
> If there are pending work items in bcm_wq during module unload,
> can_proto_unregister() will call kmem_cache_destroy() on the protocol's
> slab cache while sockets are still allocated.
>
> Will this cause a kernel warning and a slab cache leak, since
> destroy_workqueue() runs after can_proto_unregister() and will attempt to
> free sockets into the destroyed slab?
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v13 02/11] can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure
2026-07-14 10:12 [PATCH v13 00/11] can: bcm: collected fixes - final final Oliver Hartkopp via B4 Relay
2026-07-14 10:12 ` [PATCH v13 01/11] can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF Oliver Hartkopp via B4 Relay
@ 2026-07-14 10:12 ` Oliver Hartkopp via B4 Relay
2026-07-14 10:33 ` sashiko-bot
2026-07-14 10:12 ` [PATCH v13 03/11] can: bcm: add locking when updating filter and timer values Oliver Hartkopp via B4 Relay
` (8 subsequent siblings)
10 siblings, 1 reply; 32+ messages in thread
From: Oliver Hartkopp via B4 Relay @ 2026-07-14 10:12 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.
Fixes: ffd980f976e7 ("[CAN]: Add broadcast manager (bcm) protocol")
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] 32+ messages in thread* Re: [PATCH v13 02/11] can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure
2026-07-14 10:12 ` [PATCH v13 02/11] can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure Oliver Hartkopp via B4 Relay
@ 2026-07-14 10:33 ` sashiko-bot
2026-07-14 10:41 ` Oliver Hartkopp
0 siblings, 1 reply; 32+ messages in thread
From: sashiko-bot @ 2026-07-14 10:33 UTC (permalink / raw)
To: Oliver Hartkopp
Cc: Vincent Mailhol, Oleksij Rempel, Marc Kleine-Budde, linux-can,
Oliver Hartkopp
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues:
- [High] Unsafe lockless iteration of `bo->tx_ops` and `bo->rx_ops` in procfs, using `list_add()` instead of `list_add_rcu()` for list updates and `list_for_each_entry()` instead of `list_for_each_entry_rcu()`.
- [High] `bcm_rx_setup()` modifies an active `rx_op`'s data and state from process context without any synchronization with `bcm_rx_handler()` running in softirq context.
- [High] `bcm_tx_setup()` modifies an active `tx_op`'s frames and flags without holding `bcm_tx_lock`, racing with the softirq timer and transmission paths.
--
--- Patch [2]: [PATCH v13 02/11] can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure ---
commit 79233a73945b83eac56e8c17536d2f019c104f50
Author: Oliver Hartkopp <socketcan@hartkopp.net>
can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure
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.
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: High]
This isn't a bug introduced by this patch, but does bcm_rx_setup() safely
modify an active rx_op's data and state from process context without
synchronization with bcm_rx_handler() running in softirq context?
bcm_rx_setup() directly overwrites op->frames via memcpy_from_msg() and
clears op->last_frames while only holding lock_sock(sk):
net/can/bcm.c:bcm_rx_setup() {
...
err = memcpy_from_msg(op->frames, msg, msg_head->nframes * op->cfsiz);
...
}
Concurrently, bcm_rx_handler() executes locklessly in the softirq RX path
and reads these frames:
net/can/bcm.c:bcm_rx_handler() {
...
if ((get_u64(op->frames, 0) & get_u64(rxframe, 0)) == ...
...
}
Since memcpy_from_msg() copies from userspace and can block or be preempted,
could the softirq observe partially updated frames, mismatched lengths, or
torn flags, leading to incorrect CAN message filtering and potential data
corruption?
[ ... ]
> @@ -1436,20 +1463,21 @@ static int bcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
> 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:
[Severity: High]
This is a pre-existing issue, but does bcm_tx_setup() modify an active
tx_op's frames and flags without holding bcm_tx_lock, racing with the softirq
timer and transmission paths?
When updating an existing tx op, bcm_tx_setup() calls memcpy_from_msg() to
update op->frames before acquiring bcm_tx_lock:
net/can/bcm.c:bcm_tx_setup() {
...
err = memcpy_from_msg((u8 *)cf, msg, op->cfsiz);
...
}
Meanwhile, bcm_can_tx() drops the lock and calls skb_put_data() to copy the
frame into an skb:
net/can/bcm.c:bcm_can_tx() {
...
skb_put_data(skb, cf, op->cfsiz);
...
}
Could this data race allow the softirq to transmit partially updated CAN
frames onto the bus? Because memcpy_from_msg() accesses userspace and might
block, the race window appears wide enough to result in deterministic
corruption of transmitted frames under concurrent updates.
[Severity: High]
This is also a pre-existing issue, but is it safe for bcm_proc_show() to
iterate over bo->tx_ops and bo->rx_ops locklessly in procfs, while
bcm_tx_setup() and bcm_rx_setup() use list_add() instead of list_add_rcu()?
bcm_tx_setup() and bcm_rx_setup() insert new operations using the plain
list_add() macro, which lacks the smp_store_release() memory barrier:
net/can/bcm.c:bcm_tx_setup() {
...
list_add(&op->list, &bo->tx_ops);
...
}
bcm_proc_show() traverses tx_ops locklessly under rcu_read_lock(), but uses
the unsafe list_for_each_entry() macro entirely bypassing RCU read primitives:
net/can/bcm.c:bcm_proc_show() {
...
list_for_each_entry(op, &bo->tx_ops, list) {
...
}
Could a concurrent procfs reader observe the list pointer before the bcm_op
structure is fully visible in memory, causing a dereference of uninitialized
memory or a crash?
> ret = bcm_tx_setup(&msg_head, msg, ifindex, sk);
> break;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260714-bcm_fixes-v13-0-fd667c61099a@hartkopp.net?part=2
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH v13 02/11] can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure
2026-07-14 10:33 ` sashiko-bot
@ 2026-07-14 10:41 ` Oliver Hartkopp
0 siblings, 0 replies; 32+ messages in thread
From: Oliver Hartkopp @ 2026-07-14 10:41 UTC (permalink / raw)
To: sashiko-reviews
Cc: Vincent Mailhol, Oleksij Rempel, Marc Kleine-Budde, linux-can
On 14.07.26 12:33, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] Unsafe lockless iteration of `bo->tx_ops` and `bo->rx_ops` in procfs, using `list_add()` instead of `list_add_rcu()` for list updates and `list_for_each_entry()` instead of `list_for_each_entry_rcu()`.
> - [High] `bcm_rx_setup()` modifies an active `rx_op`'s data and state from process context without any synchronization with `bcm_rx_handler()` running in softirq context.
> - [High] `bcm_tx_setup()` modifies an active `tx_op`'s frames and flags without holding `bcm_tx_lock`, racing with the softirq timer and transmission paths.
> --
>
Fixed later.
> --- Patch [2]: [PATCH v13 02/11] can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure ---
> commit 79233a73945b83eac56e8c17536d2f019c104f50
> Author: Oliver Hartkopp <socketcan@hartkopp.net>
>
> can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure
>
> 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.
>
> 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: High]
> This isn't a bug introduced by this patch, but does bcm_rx_setup() safely
> modify an active rx_op's data and state from process context without
> synchronization with bcm_rx_handler() running in softirq context?
>
> bcm_rx_setup() directly overwrites op->frames via memcpy_from_msg() and
> clears op->last_frames while only holding lock_sock(sk):
>
> net/can/bcm.c:bcm_rx_setup() {
> ...
> err = memcpy_from_msg(op->frames, msg, msg_head->nframes * op->cfsiz);
> ...
> }
>
> Concurrently, bcm_rx_handler() executes locklessly in the softirq RX path
> and reads these frames:
>
> net/can/bcm.c:bcm_rx_handler() {
> ...
> if ((get_u64(op->frames, 0) & get_u64(rxframe, 0)) == ...
> ...
> }
>
> Since memcpy_from_msg() copies from userspace and can block or be preempted,
> could the softirq observe partially updated frames, mismatched lengths, or
> torn flags, leading to incorrect CAN message filtering and potential data
> corruption?
>
> [ ... ]
>> @@ -1436,20 +1463,21 @@ static int bcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
>> 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:
>
> [Severity: High]
> This is a pre-existing issue, but does bcm_tx_setup() modify an active
> tx_op's frames and flags without holding bcm_tx_lock, racing with the softirq
> timer and transmission paths?
>
> When updating an existing tx op, bcm_tx_setup() calls memcpy_from_msg() to
> update op->frames before acquiring bcm_tx_lock:
>
> net/can/bcm.c:bcm_tx_setup() {
> ...
> err = memcpy_from_msg((u8 *)cf, msg, op->cfsiz);
> ...
> }
>
> Meanwhile, bcm_can_tx() drops the lock and calls skb_put_data() to copy the
> frame into an skb:
>
> net/can/bcm.c:bcm_can_tx() {
> ...
> skb_put_data(skb, cf, op->cfsiz);
> ...
> }
>
> Could this data race allow the softirq to transmit partially updated CAN
> frames onto the bus? Because memcpy_from_msg() accesses userspace and might
> block, the race window appears wide enough to result in deterministic
> corruption of transmitted frames under concurrent updates.
>
> [Severity: High]
> This is also a pre-existing issue, but is it safe for bcm_proc_show() to
> iterate over bo->tx_ops and bo->rx_ops locklessly in procfs, while
> bcm_tx_setup() and bcm_rx_setup() use list_add() instead of list_add_rcu()?
>
> bcm_tx_setup() and bcm_rx_setup() insert new operations using the plain
> list_add() macro, which lacks the smp_store_release() memory barrier:
>
> net/can/bcm.c:bcm_tx_setup() {
> ...
> list_add(&op->list, &bo->tx_ops);
> ...
> }
>
> bcm_proc_show() traverses tx_ops locklessly under rcu_read_lock(), but uses
> the unsafe list_for_each_entry() macro entirely bypassing RCU read primitives:
>
> net/can/bcm.c:bcm_proc_show() {
> ...
> list_for_each_entry(op, &bo->tx_ops, list) {
> ...
> }
>
> Could a concurrent procfs reader observe the list pointer before the bcm_op
> structure is fully visible in memory, causing a dereference of uninitialized
> memory or a crash?
>
>> ret = bcm_tx_setup(&msg_head, msg, ifindex, sk);
>> break;
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v13 03/11] can: bcm: add locking when updating filter and timer values
2026-07-14 10:12 [PATCH v13 00/11] can: bcm: collected fixes - final final Oliver Hartkopp via B4 Relay
2026-07-14 10:12 ` [PATCH v13 01/11] can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF Oliver Hartkopp via B4 Relay
2026-07-14 10:12 ` [PATCH v13 02/11] can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure Oliver Hartkopp via B4 Relay
@ 2026-07-14 10:12 ` Oliver Hartkopp via B4 Relay
2026-07-14 10:27 ` sashiko-bot
2026-07-14 10:12 ` [PATCH v13 04/11] can: bcm: fix CAN frame rx/tx statistics Oliver Hartkopp via B4 Relay
` (7 subsequent siblings)
10 siblings, 1 reply; 32+ messages in thread
From: Oliver Hartkopp via B4 Relay @ 2026-07-14 10:12 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] 32+ messages in thread* Re: [PATCH v13 03/11] can: bcm: add locking when updating filter and timer values
2026-07-14 10:12 ` [PATCH v13 03/11] can: bcm: add locking when updating filter and timer values Oliver Hartkopp via B4 Relay
@ 2026-07-14 10:27 ` sashiko-bot
2026-07-14 10:34 ` Oliver Hartkopp
0 siblings, 1 reply; 32+ messages in thread
From: sashiko-bot @ 2026-07-14 10:27 UTC (permalink / raw)
To: Oliver Hartkopp
Cc: Marc Kleine-Budde, Oliver Hartkopp, Oleksij Rempel,
Vincent Mailhol, linux-can
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Dropping bcm_rx_update_lock in bcm_rx_handler creates a TOCTOU race that can erroneously process a received frame as a filter and restart timers for an RX_RTR_FRAME operation.
--
--- Patch [3]: [PATCH v13 03/11] can: bcm: add locking when updating filter and timer values ---
commit 72b0ae7b8598826552a4ceb6840f50d02ba50085
Author: Oliver Hartkopp <socketcan@hartkopp.net>
can: bcm: add locking when updating filter and timer values
This commit adds a new bcm_rx_update_lock to protect timer and filter
updates from concurrent access in bcm_rx_setup() and various RX handlers.
It also fixes a race by normalizing the RTR flag on the staged buffer
before registering the frames.
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
[ ... ]
> @@ -729,9 +751,18 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data)
> /* 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;
> }
>
> @@ -743,6 +774,8 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data)
> traffic_flags |= RX_OWN;
> }
>
> + spin_lock_bh(&op->bcm_rx_update_lock);
> +
[Severity: Medium]
Could this lock split cause a race condition if a concurrent bcm_rx_setup()
updates the operation to an RX_RTR_FRAME?
If bcm_rx_handler() initially sees rtr_frame == false and drops the lock:
CPU1 (bcm_rx_handler)
spin_lock_bh(&op->bcm_rx_update_lock);
rtr_frame = op->flags & RX_RTR_FRAME; // false
spin_unlock_bh(&op->bcm_rx_update_lock);
CPU2 (bcm_rx_setup)
spin_lock_bh(&op->bcm_rx_update_lock);
op->flags |= RX_RTR_FRAME;
// updates op->frames but does not clear op->kt_ival1
spin_unlock_bh(&op->bcm_rx_update_lock);
CPU1 (bcm_rx_handler)
spin_lock_bh(&op->bcm_rx_update_lock);
// Proceeds down the non-RTR path with op->flags now having RX_RTR_FRAME set
...
bcm_rx_starttimer(op);
When bcm_rx_handler() reacquires the lock and proceeds along the non-RTR path,
it will incorrectly compare the received frame against the new RTR reply frame,
potentially sending a spurious RX_CHANGED.
Furthermore, since op->kt_ival1 is not cleared when switching to RTR mode, could
this erroneously start a timer for an RTR operation and result in a spurious
RX_TIMEOUT?
> if (op->flags & RX_FILTER_ID) {
> /* the easiest case */
> bcm_rx_update_and_send(op, op->last_frames, rxframe,
> @@ -778,6 +811,8 @@ 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);
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260714-bcm_fixes-v13-0-fd667c61099a@hartkopp.net?part=3
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH v13 03/11] can: bcm: add locking when updating filter and timer values
2026-07-14 10:27 ` sashiko-bot
@ 2026-07-14 10:34 ` Oliver Hartkopp
0 siblings, 0 replies; 32+ messages in thread
From: Oliver Hartkopp @ 2026-07-14 10:34 UTC (permalink / raw)
To: sashiko-reviews
Cc: Marc Kleine-Budde, Oleksij Rempel, Vincent Mailhol, linux-can
On 14.07.26 12:27, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] Dropping bcm_rx_update_lock in bcm_rx_handler creates a TOCTOU race that can erroneously process a received frame as a filter and restart timers for an RX_RTR_FRAME operation.
> --
Fixed in patch 4
>
> --- Patch [3]: [PATCH v13 03/11] can: bcm: add locking when updating filter and timer values ---
> commit 72b0ae7b8598826552a4ceb6840f50d02ba50085
> Author: Oliver Hartkopp <socketcan@hartkopp.net>
>
> can: bcm: add locking when updating filter and timer values
>
> This commit adds a new bcm_rx_update_lock to protect timer and filter
> updates from concurrent access in bcm_rx_setup() and various RX handlers.
> It also fixes a race by normalizing the RTR flag on the staged buffer
> before registering the frames.
>
> 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
>
> [ ... ]
>
>> @@ -729,9 +751,18 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data)
>> /* 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;
>> }
>>
>> @@ -743,6 +774,8 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data)
>> traffic_flags |= RX_OWN;
>> }
>>
>> + spin_lock_bh(&op->bcm_rx_update_lock);
>> +
>
> [Severity: Medium]
> Could this lock split cause a race condition if a concurrent bcm_rx_setup()
> updates the operation to an RX_RTR_FRAME?
>
> If bcm_rx_handler() initially sees rtr_frame == false and drops the lock:
>
> CPU1 (bcm_rx_handler)
> spin_lock_bh(&op->bcm_rx_update_lock);
> rtr_frame = op->flags & RX_RTR_FRAME; // false
> spin_unlock_bh(&op->bcm_rx_update_lock);
>
> CPU2 (bcm_rx_setup)
> spin_lock_bh(&op->bcm_rx_update_lock);
> op->flags |= RX_RTR_FRAME;
> // updates op->frames but does not clear op->kt_ival1
> spin_unlock_bh(&op->bcm_rx_update_lock);
>
> CPU1 (bcm_rx_handler)
> spin_lock_bh(&op->bcm_rx_update_lock);
> // Proceeds down the non-RTR path with op->flags now having RX_RTR_FRAME set
> ...
> bcm_rx_starttimer(op);
>
> When bcm_rx_handler() reacquires the lock and proceeds along the non-RTR path,
> it will incorrectly compare the received frame against the new RTR reply frame,
> potentially sending a spurious RX_CHANGED.
>
> Furthermore, since op->kt_ival1 is not cleared when switching to RTR mode, could
> this erroneously start a timer for an RTR operation and result in a spurious
> RX_TIMEOUT?
>
>> if (op->flags & RX_FILTER_ID) {
>> /* the easiest case */
>> bcm_rx_update_and_send(op, op->last_frames, rxframe,
>> @@ -778,6 +811,8 @@ 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);
>> }
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v13 04/11] can: bcm: fix CAN frame rx/tx statistics
2026-07-14 10:12 [PATCH v13 00/11] can: bcm: collected fixes - final final Oliver Hartkopp via B4 Relay
` (2 preceding siblings ...)
2026-07-14 10:12 ` [PATCH v13 03/11] can: bcm: add locking when updating filter and timer values Oliver Hartkopp via B4 Relay
@ 2026-07-14 10:12 ` Oliver Hartkopp via B4 Relay
2026-07-14 10:33 ` sashiko-bot
2026-07-14 10:12 ` [PATCH v13 05/11] can: bcm: add missing rcu list annotations and operations Oliver Hartkopp via B4 Relay
` (6 subsequent siblings)
10 siblings, 1 reply; 32+ messages in thread
From: Oliver Hartkopp via B4 Relay @ 2026-07-14 10:12 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 separate bcm_update_[rx|tx]_stats()
functions. The rx variant runs under bcm_rx_update_lock to prevent
races when resetting the two rx counters; the tx variant runs under
bcm_tx_lock and only needs to guard its own counter's overflow.
As the rx path resets its values already at LONG_MAX / 100, there is
no conflict between the two locking domains (bcm_rx_update_lock vs.
bcm_tx_lock) even for ops that use both paths.
The rx statistics update and the frames_filtered update in
bcm_rx_changed() were previously performed in two separate
bcm_rx_update_lock sections. For an rx op subscribed on all interfaces
(ifindex == 0), bcm_rx_handler() can run concurrently on different
CPUs, so a counter reset by one CPU between these two sections could
leave frames_filtered larger than frames_abs on another CPU, producing
a bogus (even negative) reduction percentage in procfs. Update the
statistics in the same critical section as bcm_rx_changed() to close
this gap, which also removes the now unneeded extra lock/unlock pair
around the traffic_flags calculation.
Fixes: ffd980f976e7 ("[CAN]: Add broadcast manager (bcm) protocol")
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
net/can/bcm.c | 69 +++++++++++++++++++++++++++++++++++++++--------------------
1 file changed, 46 insertions(+), 23 deletions(-)
diff --git a/net/can/bcm.c b/net/can/bcm.c
index af4bb4182bc2..38c3f690c1cd 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,39 @@ 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_rx_stats(struct bcm_op *op)
+{
+ /* prevent overflow of the reduction% calculation in bcm_proc_show() */
+ 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);
+ }
+}
+
+static void bcm_update_tx_stats(struct bcm_op *op)
+{
+ /* tx_op has no reduction% calculation - use the full range and
+ * just keep the displayed counter non-negative on overflow
+ */
+ if (atomic_long_inc_return(&op->frames_abs) == LONG_MAX)
+ 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 +364,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_tx_stats(op);
op->currframe++;
/* reached last frame? */
if (op->currframe >= op->nframes)
@@ -495,16 +517,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,38 +765,42 @@ 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().
- */
+ /* 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)
+ if (rtr_frame) {
+ bcm_update_rx_stats(op);
+ /* snapshot RTR content under lock */
memcpy(&rtrframe, op->frames, op->cfsiz);
- spin_unlock_bh(&op->bcm_rx_update_lock);
+ 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);
return;
}
+ /* update statistics in the same critical section as bcm_rx_changed()
+ * below: frames_filtered must never be checked/incremented against a
+ * frames_abs snapshot from a concurrent bcm_rx_handler() call on
+ * another CPU for the same (wildcard) op, or frames_filtered can end
+ * up larger than frames_abs.
+ */
+ bcm_update_rx_stats(op);
+
/* compute flags to distinguish between own/local/remote CAN traffic */
traffic_flags = 0;
if (skb->sk) {
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;
--
2.53.0
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH v13 04/11] can: bcm: fix CAN frame rx/tx statistics
2026-07-14 10:12 ` [PATCH v13 04/11] can: bcm: fix CAN frame rx/tx statistics Oliver Hartkopp via B4 Relay
@ 2026-07-14 10:33 ` sashiko-bot
2026-07-14 10:35 ` Oliver Hartkopp
0 siblings, 1 reply; 32+ messages in thread
From: sashiko-bot @ 2026-07-14 10:33 UTC (permalink / raw)
To: Oliver Hartkopp
Cc: Oleksij Rempel, Vincent Mailhol, Oliver Hartkopp, linux-can,
Marc Kleine-Budde
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Missing RCU list primitives lead to potential NULL pointer dereference or memory corruption during lockless /proc/net/can-bcm/... traversal.
--
--- Patch [4]: [PATCH v13 04/11] can: bcm: fix CAN frame rx/tx statistics ---
commit acb95cd7e83f916c374a2a2f75ed2b971c9fa9d5
Author: Oliver Hartkopp <socketcan@hartkopp.net>
can: bcm: fix CAN frame rx/tx statistics
This commit fixes a data race detected by KCSAN when two CAN frames are
simultaneously received and processed by different CPUs. It transitions
the statistics counters to use atomic operations to prevent races.
Additionally, it simplifies overflow checks and consolidates rx statistics
updates into a single critical section.
> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index af4bb4182bc29..38c3f690c1cd0 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -229,10 +229,13 @@ static int bcm_proc_show(struct seq_file *m, void *v)
>
> list_for_each_entry_rcu(op, &bo->rx_ops, list) {
>
> - unsigned long reduction;
> + long reduction, frames_filtered, frames_abs;
> +
[ ... ]
> @@ -280,7 +283,8 @@ static int bcm_proc_show(struct seq_file *m, void *v)
> 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));
> }
[Severity: High]
This is a pre-existing issue, but while reviewing this code we noticed a
potential missing RCU primitive issue.
In bcm_proc_show(), the code traverses the bo->tx_ops list (which contains
the loop printing the tx stats above) using list_for_each_entry() instead
of list_for_each_entry_rcu(). This omits the READ_ONCE() dependency ordering
required for safe lockless traversal under rcu_read_lock().
Additionally, when adding to these lists in bcm_tx_setup() and
bcm_rx_setup(), the code uses the non-RCU list_add() primitive instead of
list_add_rcu(), which omits the required smp_store_release() semantics.
Since a user can repeatedly read the procfs file for a CAN_BCM socket while
another thread concurrently adds filters on the same socket, could this
expose lockless readers to uninitialized memory and lead to memory corruption
or wild pointer dereferences?
> seq_putc(m, '\n');
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260714-bcm_fixes-v13-0-fd667c61099a@hartkopp.net?part=4
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH v13 04/11] can: bcm: fix CAN frame rx/tx statistics
2026-07-14 10:33 ` sashiko-bot
@ 2026-07-14 10:35 ` Oliver Hartkopp
0 siblings, 0 replies; 32+ messages in thread
From: Oliver Hartkopp @ 2026-07-14 10:35 UTC (permalink / raw)
To: sashiko-reviews
Cc: Oleksij Rempel, Vincent Mailhol, linux-can, Marc Kleine-Budde
On 14.07.26 12:33, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] Missing RCU list primitives lead to potential NULL pointer dereference or memory corruption during lockless /proc/net/can-bcm/... traversal.
> --
Fixed in patch 5
>
> --- Patch [4]: [PATCH v13 04/11] can: bcm: fix CAN frame rx/tx statistics ---
> commit acb95cd7e83f916c374a2a2f75ed2b971c9fa9d5
> Author: Oliver Hartkopp <socketcan@hartkopp.net>
>
> can: bcm: fix CAN frame rx/tx statistics
>
> This commit fixes a data race detected by KCSAN when two CAN frames are
> simultaneously received and processed by different CPUs. It transitions
> the statistics counters to use atomic operations to prevent races.
> Additionally, it simplifies overflow checks and consolidates rx statistics
> updates into a single critical section.
>
>> diff --git a/net/can/bcm.c b/net/can/bcm.c
>> index af4bb4182bc29..38c3f690c1cd0 100644
>> --- a/net/can/bcm.c
>> +++ b/net/can/bcm.c
>> @@ -229,10 +229,13 @@ static int bcm_proc_show(struct seq_file *m, void *v)
>>
>> list_for_each_entry_rcu(op, &bo->rx_ops, list) {
>>
>> - unsigned long reduction;
>> + long reduction, frames_filtered, frames_abs;
>> +
>
> [ ... ]
>
>> @@ -280,7 +283,8 @@ static int bcm_proc_show(struct seq_file *m, void *v)
>> 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));
>> }
>
> [Severity: High]
> This is a pre-existing issue, but while reviewing this code we noticed a
> potential missing RCU primitive issue.
>
> In bcm_proc_show(), the code traverses the bo->tx_ops list (which contains
> the loop printing the tx stats above) using list_for_each_entry() instead
> of list_for_each_entry_rcu(). This omits the READ_ONCE() dependency ordering
> required for safe lockless traversal under rcu_read_lock().
>
> Additionally, when adding to these lists in bcm_tx_setup() and
> bcm_rx_setup(), the code uses the non-RCU list_add() primitive instead of
> list_add_rcu(), which omits the required smp_store_release() semantics.
>
> Since a user can repeatedly read the procfs file for a CAN_BCM socket while
> another thread concurrently adds filters on the same socket, could this
> expose lockless readers to uninitialized memory and lead to memory corruption
> or wild pointer dereferences?
>
>> seq_putc(m, '\n');
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v13 05/11] can: bcm: add missing rcu list annotations and operations
2026-07-14 10:12 [PATCH v13 00/11] can: bcm: collected fixes - final final Oliver Hartkopp via B4 Relay
` (3 preceding siblings ...)
2026-07-14 10:12 ` [PATCH v13 04/11] can: bcm: fix CAN frame rx/tx statistics Oliver Hartkopp via B4 Relay
@ 2026-07-14 10:12 ` Oliver Hartkopp via B4 Relay
2026-07-14 10:32 ` sashiko-bot
2026-07-14 10:12 ` [PATCH v13 06/11] can: bcm: extend bcm_tx_lock usage for data and timer updates Oliver Hartkopp via B4 Relay
` (5 subsequent siblings)
10 siblings, 1 reply; 32+ messages in thread
From: Oliver Hartkopp via B4 Relay @ 2026-07-14 10:12 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 38c3f690c1cd..940cd9df1cca 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)
@@ -1007,10 +1007,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)
@@ -1148,12 +1149,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 */
@@ -1171,10 +1171,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);
@@ -1355,13 +1359,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))) */
@@ -1429,14 +1430,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;
}
@@ -1766,12 +1769,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.
@@ -1798,12 +1803,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] 32+ messages in thread* Re: [PATCH v13 05/11] can: bcm: add missing rcu list annotations and operations
2026-07-14 10:12 ` [PATCH v13 05/11] can: bcm: add missing rcu list annotations and operations Oliver Hartkopp via B4 Relay
@ 2026-07-14 10:32 ` sashiko-bot
2026-07-14 10:46 ` Oliver Hartkopp
0 siblings, 1 reply; 32+ messages in thread
From: sashiko-bot @ 2026-07-14 10:32 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 2 potential issue(s) to consider:
Pre-existing issues:
- [High] In `bcm_tx_setup()`, when updating an existing `bcm_op`, the CAN frames array (`op->frames`) is overwritten directly from userspace data using `memcpy_from_msg` without holding `bcm_tx_lock` and without using a staging buffer. This introduces a data race with concurrent transmission. Crucially, the direct overwrite stores user-provided frames into the active array before length validation. If an invalid length is provided, the function subsequently returns an error, but the corrupted frame remains active in `op->frames`. Concurrent or subsequent timer executions will transmit this malformed frame to the CAN driver, leading to out-of-bounds accesses and kernel panics.
- [Medium] Timer values in `bcm_tx_setup()` (`op->kt_ival1` and `op->kt_ival2`) are updated directly without holding `bcm_tx_lock`. Concurrently, `bcm_tx_timeout_handler()` running in softirq context reads these 64-bit `ktime_t` values without locks. On 32-bit architectures, this leads to torn reads, causing unpredictable timer behavior.
--
--- Patch [5]: [PATCH v13 05/11] can: bcm: add missing rcu list annotations and operations ---
commit 74409df5d31bac1c2081de9ebaae45419726cd9c
Author: Oliver Hartkopp <socketcan@hartkopp.net>
can: bcm: add missing rcu list annotations and operations
This commit addresses missing list_add_rcu() and list_del_rcu() calls in
the CAN broadcast manager to properly initialize and clean up bcm_op
structures. The operations are updated to protect concurrent procfs
traversals that use rcu_read_lock().
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 38c3f690c1cd0..940cd9df1cca3 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -1009,6 +1009,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: High]
This is a pre-existing issue, but does this code overwrite the CAN frames
directly from userspace before validating their lengths?
In bcm_tx_setup() when updating an existing bcm_op, the frames array is
updated directly without holding bcm_tx_lock and without a staging buffer:
bcm_tx_setup() {
...
err = memcpy_from_msg((u8 *)cf, msg, op->cfsiz);
if (op->flags & CAN_FD_FRAME) {
if (cf->len > 64)
err = -EINVAL;
...
}
If an invalid length is provided, the function returns -EINVAL but the
corrupted frame remains active in op->frames.
Could concurrent or subsequent timer executions transmit this malformed frame
to the CAN driver, potentially triggering out-of-bounds accesses and panics?
[Severity: Medium]
This is a pre-existing issue, but are the timer values updated locklessly?
bcm_tx_setup() updates the 64-bit ktime_t variables kt_ival1 and kt_ival2
directly without holding bcm_tx_lock:
bcm_tx_setup() {
...
op->kt_ival1 = bcm_timeval_to_ktime(msg_head->ival1);
op->kt_ival2 = bcm_timeval_to_ktime(msg_head->ival2);
...
}
Concurrently, bcm_tx_timeout_handler() in softirq context reads these
values locklessly:
bcm_tx_timeout_handler() {
...
if (op->kt_ival1 && (op->count > 0)) {
...
}
On 32-bit architectures, a 64-bit read/write is not atomic.
Could this result in torn reads where the timer handler sees a mix of old
and new 32-bit halves, leading to unpredictable timer behavior and erratic
transmission intervals?
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260714-bcm_fixes-v13-0-fd667c61099a@hartkopp.net?part=5
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH v13 05/11] can: bcm: add missing rcu list annotations and operations
2026-07-14 10:32 ` sashiko-bot
@ 2026-07-14 10:46 ` Oliver Hartkopp
0 siblings, 0 replies; 32+ messages in thread
From: Oliver Hartkopp @ 2026-07-14 10:46 UTC (permalink / raw)
To: sashiko-reviews
Cc: Marc Kleine-Budde, linux-can, Vincent Mailhol, Oleksij Rempel
On 14.07.26 12:32, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] In `bcm_tx_setup()`, when updating an existing `bcm_op`, the CAN frames array (`op->frames`) is overwritten directly from userspace data using `memcpy_from_msg` without holding `bcm_tx_lock` and without using a staging buffer. This introduces a data race with concurrent transmission. Crucially, the direct overwrite stores user-provided frames into the active array before length validation. If an invalid length is provided, the function subsequently returns an error, but the corrupted frame remains active in `op->frames`. Concurrent or subsequent timer executions will transmit this malformed frame to the CAN driver, leading to out-of-bounds accesses and kernel panics.
> - [Medium] Timer values in `bcm_tx_setup()` (`op->kt_ival1` and `op->kt_ival2`) are updated directly without holding `bcm_tx_lock`. Concurrently, `bcm_tx_timeout_handler()` running in softirq context reads these 64-bit `ktime_t` values without locks. On 32-bit architectures, this leads to torn reads, causing unpredictable timer behavior.
> --
>
Fixed in patch 6
> --- Patch [5]: [PATCH v13 05/11] can: bcm: add missing rcu list annotations and operations ---
> commit 74409df5d31bac1c2081de9ebaae45419726cd9c
> Author: Oliver Hartkopp <socketcan@hartkopp.net>
>
> can: bcm: add missing rcu list annotations and operations
>
> This commit addresses missing list_add_rcu() and list_del_rcu() calls in
> the CAN broadcast manager to properly initialize and clean up bcm_op
> structures. The operations are updated to protect concurrent procfs
> traversals that use rcu_read_lock().
>
> 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 38c3f690c1cd0..940cd9df1cca3 100644
>> --- a/net/can/bcm.c
>> +++ b/net/can/bcm.c
>> @@ -1009,6 +1009,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: High]
> This is a pre-existing issue, but does this code overwrite the CAN frames
> directly from userspace before validating their lengths?
>
> In bcm_tx_setup() when updating an existing bcm_op, the frames array is
> updated directly without holding bcm_tx_lock and without a staging buffer:
>
> bcm_tx_setup() {
> ...
> err = memcpy_from_msg((u8 *)cf, msg, op->cfsiz);
> if (op->flags & CAN_FD_FRAME) {
> if (cf->len > 64)
> err = -EINVAL;
> ...
> }
>
> If an invalid length is provided, the function returns -EINVAL but the
> corrupted frame remains active in op->frames.
>
> Could concurrent or subsequent timer executions transmit this malformed frame
> to the CAN driver, potentially triggering out-of-bounds accesses and panics?
>
> [Severity: Medium]
> This is a pre-existing issue, but are the timer values updated locklessly?
>
> bcm_tx_setup() updates the 64-bit ktime_t variables kt_ival1 and kt_ival2
> directly without holding bcm_tx_lock:
>
> bcm_tx_setup() {
> ...
> op->kt_ival1 = bcm_timeval_to_ktime(msg_head->ival1);
> op->kt_ival2 = bcm_timeval_to_ktime(msg_head->ival2);
> ...
> }
>
> Concurrently, bcm_tx_timeout_handler() in softirq context reads these
> values locklessly:
>
> bcm_tx_timeout_handler() {
> ...
> if (op->kt_ival1 && (op->count > 0)) {
> ...
> }
>
> On 32-bit architectures, a 64-bit read/write is not atomic.
>
> Could this result in torn reads where the timer handler sees a mix of old
> and new 32-bit halves, leading to unpredictable timer behavior and erratic
> transmission intervals?
>
> [ ... ]
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v13 06/11] can: bcm: extend bcm_tx_lock usage for data and timer updates
2026-07-14 10:12 [PATCH v13 00/11] can: bcm: collected fixes - final final Oliver Hartkopp via B4 Relay
` (4 preceding siblings ...)
2026-07-14 10:12 ` [PATCH v13 05/11] can: bcm: add missing rcu list annotations and operations Oliver Hartkopp via B4 Relay
@ 2026-07-14 10:12 ` Oliver Hartkopp via B4 Relay
2026-07-14 10:34 ` sashiko-bot
2026-07-14 10:12 ` [PATCH v13 07/11] can: bcm: validate frame length in bcm_rx_setup() for RTR replies Oliver Hartkopp via B4 Relay
` (4 subsequent siblings)
10 siblings, 1 reply; 32+ messages in thread
From: Oliver Hartkopp via B4 Relay @ 2026-07-14 10:12 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 940cd9df1cca..18601d6fdf76 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;
@@ -462,16 +462,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;
}
@@ -484,12 +490,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));
@@ -502,11 +517,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;
@@ -1026,64 +1041,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);
@@ -1155,14 +1186,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] 32+ messages in thread* Re: [PATCH v13 06/11] can: bcm: extend bcm_tx_lock usage for data and timer updates
2026-07-14 10:12 ` [PATCH v13 06/11] can: bcm: extend bcm_tx_lock usage for data and timer updates Oliver Hartkopp via B4 Relay
@ 2026-07-14 10:34 ` sashiko-bot
2026-07-14 10:56 ` Oliver Hartkopp
0 siblings, 1 reply; 32+ messages in thread
From: sashiko-bot @ 2026-07-14 10:34 UTC (permalink / raw)
To: Oliver Hartkopp
Cc: linux-can, Marc Kleine-Budde, Vincent Mailhol, Oliver Hartkopp,
Oleksij Rempel
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Torn reads of ival1, ival2, and count lead to corrupted TX_EXPIRED API notifications.
--
--- Patch [6]: [PATCH v13 06/11] can: bcm: extend bcm_tx_lock usage for data and timer updates ---
commit 69a20e6fcc24540387b82e9501090dfb55f155cf
Author: Oliver Hartkopp <socketcan@hartkopp.net>
can: bcm: extend bcm_tx_lock usage for data and timer updates
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.
> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index 940cd9df1cca3..18601d6fdf763 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
[ ... ]
> @@ -486,8 +492,17 @@ 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)) {
>
[Severity: High]
This isn't a bug introduced by this patch, but the lockless read of
op->count here can see a newly written value after bcm_can_tx() drops the
lock.
If bcm_tx_setup() runs concurrently via sendmsg() and mutates op->count,
could this condition falsely fail and drop the TX_EXPIRED event?
Additionally, in the lines immediately following this (when setting up
msg_head), op->ival1, op->ival2, and op->count are read locklessly:
msg_head.count = op->count;
msg_head.ival1 = op->ival1;
msg_head.ival2 = op->ival2;
Since bcm_tx_setup() modifies these multi-word structures under
op->bcm_tx_lock, does this lockless read risk tearing the tv_sec and
tv_usec halves on 32-bit systems, sending corrupted timevals to userspace?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260714-bcm_fixes-v13-0-fd667c61099a@hartkopp.net?part=6
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH v13 06/11] can: bcm: extend bcm_tx_lock usage for data and timer updates
2026-07-14 10:34 ` sashiko-bot
@ 2026-07-14 10:56 ` Oliver Hartkopp
0 siblings, 0 replies; 32+ messages in thread
From: Oliver Hartkopp @ 2026-07-14 10:56 UTC (permalink / raw)
To: sashiko-reviews
Cc: linux-can, Marc Kleine-Budde, Vincent Mailhol, Oleksij Rempel
On 14.07.26 12:34, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] Torn reads of ival1, ival2, and count lead to corrupted TX_EXPIRED API notifications.
> --
>
Well yes :-/
V14 candidate
> --- Patch [6]: [PATCH v13 06/11] can: bcm: extend bcm_tx_lock usage for data and timer updates ---
> commit 69a20e6fcc24540387b82e9501090dfb55f155cf
> Author: Oliver Hartkopp <socketcan@hartkopp.net>
>
> can: bcm: extend bcm_tx_lock usage for data and timer updates
>
> 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.
>
>> diff --git a/net/can/bcm.c b/net/can/bcm.c
>> index 940cd9df1cca3..18601d6fdf763 100644
>> --- a/net/can/bcm.c
>> +++ b/net/can/bcm.c
>
> [ ... ]
>
>> @@ -486,8 +492,17 @@ 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)) {
>>
>
> [Severity: High]
> This isn't a bug introduced by this patch, but the lockless read of
> op->count here can see a newly written value after bcm_can_tx() drops the
> lock.
>
> If bcm_tx_setup() runs concurrently via sendmsg() and mutates op->count,
> could this condition falsely fail and drop the TX_EXPIRED event?
>
> Additionally, in the lines immediately following this (when setting up
> msg_head), op->ival1, op->ival2, and op->count are read locklessly:
>
> msg_head.count = op->count;
> msg_head.ival1 = op->ival1;
> msg_head.ival2 = op->ival2;
>
> Since bcm_tx_setup() modifies these multi-word structures under
> op->bcm_tx_lock, does this lockless read risk tearing the tv_sec and
> tv_usec halves on 32-bit systems, sending corrupted timevals to userspace?
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v13 07/11] can: bcm: validate frame length in bcm_rx_setup() for RTR replies
2026-07-14 10:12 [PATCH v13 00/11] can: bcm: collected fixes - final final Oliver Hartkopp via B4 Relay
` (5 preceding siblings ...)
2026-07-14 10:12 ` [PATCH v13 06/11] can: bcm: extend bcm_tx_lock usage for data and timer updates Oliver Hartkopp via B4 Relay
@ 2026-07-14 10:12 ` Oliver Hartkopp via B4 Relay
2026-07-14 10:12 ` [PATCH v13 08/11] can: bcm: add missing device refcount for CAN filter removal Oliver Hartkopp via B4 Relay
` (3 subsequent siblings)
10 siblings, 0 replies; 32+ messages in thread
From: Oliver Hartkopp via B4 Relay @ 2026-07-14 10:12 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 18601d6fdf76..3cc3deb98a98 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -1223,26 +1223,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)
*/
@@ -1301,11 +1316,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;
@@ -1366,20 +1385,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);
@@ -1472,10 +1487,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] 32+ messages in thread* [PATCH v13 08/11] can: bcm: add missing device refcount for CAN filter removal
2026-07-14 10:12 [PATCH v13 00/11] can: bcm: collected fixes - final final Oliver Hartkopp via B4 Relay
` (6 preceding siblings ...)
2026-07-14 10:12 ` [PATCH v13 07/11] can: bcm: validate frame length in bcm_rx_setup() for RTR replies Oliver Hartkopp via B4 Relay
@ 2026-07-14 10:12 ` Oliver Hartkopp via B4 Relay
2026-07-14 10:12 ` [PATCH v13 09/11] can: bcm: fix stale rx/tx ops after device removal Oliver Hartkopp via B4 Relay
` (2 subsequent siblings)
10 siblings, 0 replies; 32+ messages in thread
From: Oliver Hartkopp via B4 Relay @ 2026-07-14 10:12 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 3cc3deb98a98..5660165cdf0e 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 {
@@ -914,10 +915,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);
}
@@ -944,21 +946,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);
@@ -1463,11 +1462,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
*/
@@ -1840,20 +1849,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] 32+ messages in thread* [PATCH v13 09/11] can: bcm: fix stale rx/tx ops after device removal
2026-07-14 10:12 [PATCH v13 00/11] can: bcm: collected fixes - final final Oliver Hartkopp via B4 Relay
` (7 preceding siblings ...)
2026-07-14 10:12 ` [PATCH v13 08/11] can: bcm: add missing device refcount for CAN filter removal Oliver Hartkopp via B4 Relay
@ 2026-07-14 10:12 ` Oliver Hartkopp via B4 Relay
2026-07-14 10:44 ` sashiko-bot
2026-07-14 10:12 ` [PATCH v13 10/11] can: bcm: fix data race on rx_stamp/rx_ifindex in bcm_rx_handler() Oliver Hartkopp via B4 Relay
2026-07-14 10:12 ` [PATCH v13 11/11] can: bcm: track a single source interface for ANYDEV timeout/throttle ops Oliver Hartkopp via B4 Relay
10 siblings, 1 reply; 32+ messages in thread
From: Oliver Hartkopp via B4 Relay @ 2026-07-14 10:12 UTC (permalink / raw)
To: linux-can; +Cc: Oliver Hartkopp, Marc Kleine-Budde, sashiko-bot
From: Oliver Hartkopp <socketcan@hartkopp.net>
RX: an RX_SETUP update(!) for an existing op skipped can_rx_register()
unconditionally, even when a concurrent NETDEV_UNREGISTER had already
torn down its registration (op->rx_reg_dev == NULL). This silently
did not re-enable frame delivery for that updated filter. bcm_rx_setup()
now re-registers in that case, while leaving rx_ops with ifindex = 0
(all CAN devices) which never carry a tracked rx_reg_dev registered as-is.
TX: bcm_notify() only handled bo->rx_ops on NETDEV_UNREGISTER, leaving
tx_ops with an active cyclic transmission re-arming its hrtimer
indefinitely to execute bcm_tx_timeout_handler(). Cancelling the hrtimer
prevents the runaway timer and any injection into a later reused ifindex,
since nothing else calls bcm_can_tx() for the op until an explicit
TX_SETUP update re-arms it.
Unlike bcm_rx_unreg(), which clears the tracked rx_reg_dev for rx_ops,
the ifindex is intentionally left unchanged for tx_ops. bcm_tx_setup()
always rejects ifindex 0, so clearing it would strand the op: neither a
later TX_SETUP (bcm_find_op()) nor TX_DELETE (bcm_delete_tx_op()) could
ever find it again, since both require an exact ifindex match.
Reported-by: sashiko-bot@kernel.org
Link: https://lore.kernel.org/linux-can/20260708094536.DDF821F00A3A@smtp.kernel.org/
Link: https://lore.kernel.org/linux-can/20260708154039.347ED1F000E9@smtp.kernel.org/
Fixes: ffd980f976e7 ("[CAN]: Add broadcast manager (bcm) protocol")
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
net/can/bcm.c | 52 +++++++++++++++++++++++++++++++++++++++++++---------
1 file changed, 43 insertions(+), 9 deletions(-)
diff --git a/net/can/bcm.c b/net/can/bcm.c
index 5660165cdf0e..10dd99ecce70 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -1264,10 +1264,11 @@ static int bcm_rx_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;
int do_rx_register;
+ int new_op = 0;
int err = 0;
if ((msg_head->flags & RX_FILTER_ID) || (!(msg_head->nframes))) {
/* be robust against wrong usage ... */
msg_head->flags |= RX_FILTER_ID;
@@ -1340,12 +1341,19 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
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;
+ /* Don't register a new CAN filter for the rx_op update unless
+ * a concurrent NETDEV_UNREGISTER notifier already tore down
+ * the previous registration. In this case the receiver needs
+ * to be re-registered here so that this update doesn't
+ * silently stop delivering frames for the given ifindex.
+ * Ops with ifindex = 0 (all CAN interfaces) never carry a
+ * tracked rx_reg_dev and stay registered as-is.
+ */
+ do_rx_register = (ifindex && !op->rx_reg_dev) ? 1 : 0;
} else {
/* insert new BCM operation for the given can_id */
op = kzalloc(OPSIZ, GFP_KERNEL);
if (!op)
@@ -1408,10 +1416,11 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
hrtimer_setup(&op->thrtimer, bcm_rx_thr_handler, CLOCK_MONOTONIC,
HRTIMER_MODE_REL_SOFT);
/* call can_rx_register() */
do_rx_register = 1;
+ new_op = 1;
} /* if ((op = bcm_find_op(&bo->rx_ops, msg_head->can_id, ifindex))) */
/* check flags */
@@ -1449,11 +1458,14 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
if ((op->flags & STARTTIMER) && op->kt_ival1)
hrtimer_start(&op->timer, op->kt_ival1,
HRTIMER_MODE_REL_SOFT);
}
- /* now we can register for can_ids, if we added a new bcm_op */
+ /* now we can register for can_ids, if we added a new bcm_op
+ * or need to re-register after a NETDEV_UNREGISTER tore down
+ * the previous registration of an existing op
+ */
if (do_rx_register) {
if (ifindex) {
struct net_device *dev;
dev = dev_get_by_index(sock_net(sk), ifindex);
@@ -1481,22 +1493,36 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
* silently succeed without registering
*/
err = -ENODEV;
}
- } else
+ } else {
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 */
- bcm_remove_op(op);
+ /* newly created bcm rx op is broken -> remove it */
+ if (new_op) {
+ bcm_remove_op(op);
+ return err;
+ }
+
+ /* an existing op just stays unregistered.
+ * Cancel op->timer and (defensively) op->thrtimer.
+ * Other settings can't be reached until the next
+ * successful RX_SETUP.
+ */
+ hrtimer_cancel(&op->timer);
+ hrtimer_cancel(&op->thrtimer);
return err;
}
- /* add this bcm_op to the list of the rx_ops */
- list_add_rcu(&op->list, &bo->rx_ops);
+ /* add a new bcm_op to the list of the rx_ops */
+ if (new_op)
+ list_add_rcu(&op->list, &bo->rx_ops);
}
return msg_head->nframes * op->cfsiz + MHSIZ;
free_op:
@@ -1712,15 +1738,23 @@ static void bcm_notify(struct bcm_sock *bo, unsigned long msg,
switch (msg) {
case NETDEV_UNREGISTER:
lock_sock(sk);
- /* remove device specific receive entries */
+ /* 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);
+ /* tx_ops: stop device specific cyclic transmissions on the
+ * vanishing ifindex. 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)
+ hrtimer_cancel(&op->timer);
+
/* remove device reference, if this is our bound device */
if (bo->bound && bo->ifindex == dev->ifindex) {
#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);
--
2.53.0
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH v13 09/11] can: bcm: fix stale rx/tx ops after device removal
2026-07-14 10:12 ` [PATCH v13 09/11] can: bcm: fix stale rx/tx ops after device removal Oliver Hartkopp via B4 Relay
@ 2026-07-14 10:44 ` sashiko-bot
2026-07-14 11:24 ` Oliver Hartkopp
0 siblings, 1 reply; 32+ messages in thread
From: sashiko-bot @ 2026-07-14 10:44 UTC (permalink / raw)
To: Oliver Hartkopp
Cc: Marc Kleine-Budde, Oliver Hartkopp, Oleksij Rempel, linux-can,
Vincent Mailhol
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Unprotected concurrent access to op->rx_stamp and op->rx_ifindex in bcm_rx_handler causes data races and torn 64-bit reads on 32-bit architectures.
--
--- Patch [9]: [PATCH v13 09/11] can: bcm: fix stale rx/tx ops after device removal ---
commit 7ac0666664aa8f0bbda8367494a6678c948d2082
Author: Oliver Hartkopp <socketcan@hartkopp.net>
can: bcm: fix stale rx/tx ops after device removal
This commit fixes issues where an RX_SETUP update could silently skip
registering if a concurrent NETDEV_UNREGISTER occurred. It also ensures
that cyclic transmissions are stopped when a device is removed.
Link: https://lore.kernel.org/linux-can/20260708094536.DDF821F00A3A@smtp.kernel.org/
Link: https://lore.kernel.org/linux-can/20260708154039.347ED1F000E9@smtp.kernel.org/
> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index 5660165cdf0eb..10dd99ecce70e 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
[ ... ]
> @@ -1483,18 +1495,32 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
> err = -ENODEV;
> }
>
> - } else
> + } else {
> err = can_rx_register(sock_net(sk), NULL, op->can_id,
> REGMASK(op->can_id),
> bcm_rx_handler, op, "bcm", sk);
> + }
[Severity: High]
This is a pre-existing issue, but is there a data race when bcm_rx_handler()
accesses op->rx_stamp and op->rx_ifindex?
When a BCM socket is bound to ifindex 0 (all CAN devices), multiple CAN
frames arriving simultaneously on different CAN interfaces trigger
bcm_rx_handler() concurrently. The handler unconditionally writes to
op->rx_stamp and op->rx_ifindex outside of op->bcm_rx_update_lock:
net/can/bcm.c:bcm_rx_handler() {
...
/* save rx timestamp */
op->rx_stamp = skb->tstamp;
/* save originator for recvfrom() */
op->rx_ifindex = skb->dev->ifindex;
...
}
These fields are then read in bcm_send_to_user(), sometimes locklessly (e.g.
when called from bcm_rx_timeout_handler()):
net/can/bcm.c:bcm_send_to_user() {
...
/* restore rx timestamp */
skb->tstamp = op->rx_stamp;
...
addr->can_ifindex = op->rx_ifindex;
...
}
Does this cause torn 64-bit reads of op->rx_stamp on 32-bit architectures,
and can the interface index be logically swapped with that of a concurrent
frame?
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260714-bcm_fixes-v13-0-fd667c61099a@hartkopp.net?part=9
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH v13 09/11] can: bcm: fix stale rx/tx ops after device removal
2026-07-14 10:44 ` sashiko-bot
@ 2026-07-14 11:24 ` Oliver Hartkopp
2026-07-14 11:41 ` Marc Kleine-Budde
0 siblings, 1 reply; 32+ messages in thread
From: Oliver Hartkopp @ 2026-07-14 11:24 UTC (permalink / raw)
To: sashiko-reviews
Cc: Marc Kleine-Budde, Oleksij Rempel, linux-can, Vincent Mailhol
On 14.07.26 12:44, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] Unprotected concurrent access to op->rx_stamp and op->rx_ifindex in bcm_rx_handler causes data races and torn 64-bit reads on 32-bit architectures.
Fixed in patch 10
> --
>
> --- Patch [9]: [PATCH v13 09/11] can: bcm: fix stale rx/tx ops after device removal ---
> commit 7ac0666664aa8f0bbda8367494a6678c948d2082
> Author: Oliver Hartkopp <socketcan@hartkopp.net>
>
> can: bcm: fix stale rx/tx ops after device removal
>
> This commit fixes issues where an RX_SETUP update could silently skip
> registering if a concurrent NETDEV_UNREGISTER occurred. It also ensures
> that cyclic transmissions are stopped when a device is removed.
>
> Link: https://lore.kernel.org/linux-can/20260708094536.DDF821F00A3A@smtp.kernel.org/
> Link: https://lore.kernel.org/linux-can/20260708154039.347ED1F000E9@smtp.kernel.org/
>
>> diff --git a/net/can/bcm.c b/net/can/bcm.c
>> index 5660165cdf0eb..10dd99ecce70e 100644
>> --- a/net/can/bcm.c
>> +++ b/net/can/bcm.c
> [ ... ]
>> @@ -1483,18 +1495,32 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>> err = -ENODEV;
>> }
>>
>> - } else
>> + } else {
>> err = can_rx_register(sock_net(sk), NULL, op->can_id,
>> REGMASK(op->can_id),
>> bcm_rx_handler, op, "bcm", sk);
>> + }
>
> [Severity: High]
> This is a pre-existing issue, but is there a data race when bcm_rx_handler()
> accesses op->rx_stamp and op->rx_ifindex?
>
> When a BCM socket is bound to ifindex 0 (all CAN devices), multiple CAN
> frames arriving simultaneously on different CAN interfaces trigger
> bcm_rx_handler() concurrently. The handler unconditionally writes to
> op->rx_stamp and op->rx_ifindex outside of op->bcm_rx_update_lock:
>
> net/can/bcm.c:bcm_rx_handler() {
> ...
> /* save rx timestamp */
> op->rx_stamp = skb->tstamp;
> /* save originator for recvfrom() */
> op->rx_ifindex = skb->dev->ifindex;
> ...
> }
>
> These fields are then read in bcm_send_to_user(), sometimes locklessly (e.g.
> when called from bcm_rx_timeout_handler()):
>
> net/can/bcm.c:bcm_send_to_user() {
> ...
> /* restore rx timestamp */
> skb->tstamp = op->rx_stamp;
> ...
> addr->can_ifindex = op->rx_ifindex;
> ...
> }
>
> Does this cause torn 64-bit reads of op->rx_stamp on 32-bit architectures,
> and can the interface index be logically swapped with that of a concurrent
> frame?
>
> [ ... ]
>
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH v13 09/11] can: bcm: fix stale rx/tx ops after device removal
2026-07-14 11:24 ` Oliver Hartkopp
@ 2026-07-14 11:41 ` Marc Kleine-Budde
2026-07-14 11:47 ` Oliver Hartkopp
0 siblings, 1 reply; 32+ messages in thread
From: Marc Kleine-Budde @ 2026-07-14 11:41 UTC (permalink / raw)
To: Oliver Hartkopp
Cc: sashiko-reviews, Oleksij Rempel, linux-can, Vincent Mailhol
[-- Attachment #1: Type: text/plain, Size: 922 bytes --]
On 14.07.2026 13:24:17, Oliver Hartkopp wrote:
> On 14.07.26 12:44, sashiko-bot@kernel.org wrote:
> > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> >
> > Pre-existing issues:
> > - [High] Unprotected concurrent access to op->rx_stamp and op->rx_ifindex in bcm_rx_handler causes data races and torn 64-bit reads on 32-bit architectures.
>
> Fixed in patch 10
Pre-existing issues are not a shot stopper, _new_ issues for a patch
series are...usually are.
Hopefully upstream will accept that you fix issues in a later patch that
sashiko claim to be introduced by a patch in the series.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v13 09/11] can: bcm: fix stale rx/tx ops after device removal
2026-07-14 11:41 ` Marc Kleine-Budde
@ 2026-07-14 11:47 ` Oliver Hartkopp
2026-07-14 12:36 ` Marc Kleine-Budde
0 siblings, 1 reply; 32+ messages in thread
From: Oliver Hartkopp @ 2026-07-14 11:47 UTC (permalink / raw)
To: Marc Kleine-Budde
Cc: sashiko-reviews, Oleksij Rempel, linux-can, Vincent Mailhol
On 14.07.26 13:41, Marc Kleine-Budde wrote:
> On 14.07.2026 13:24:17, Oliver Hartkopp wrote:
>> On 14.07.26 12:44, sashiko-bot@kernel.org wrote:
>>> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>>>
>>> Pre-existing issues:
>>> - [High] Unprotected concurrent access to op->rx_stamp and op->rx_ifindex in bcm_rx_handler causes data races and torn 64-bit reads on 32-bit architectures.
>>
>> Fixed in patch 10
>
> Pre-existing issues are not a shot stopper, _new_ issues for a patch
> series are...usually are.
>
> Hopefully upstream will accept that you fix issues in a later patch that
> sashiko claim to be introduced by a patch in the series.
Is it?
Patch 10 Fixes: ffd980f976e7 ("[CAN]: Add broadcast manager (bcm) protocol")
not anything in patch 9.
Best regards,
Oliver
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH v13 09/11] can: bcm: fix stale rx/tx ops after device removal
2026-07-14 11:47 ` Oliver Hartkopp
@ 2026-07-14 12:36 ` Marc Kleine-Budde
2026-07-14 13:07 ` Oliver Hartkopp
0 siblings, 1 reply; 32+ messages in thread
From: Marc Kleine-Budde @ 2026-07-14 12:36 UTC (permalink / raw)
To: Oliver Hartkopp
Cc: sashiko-reviews, Oleksij Rempel, linux-can, Vincent Mailhol
[-- Attachment #1: Type: text/plain, Size: 1340 bytes --]
On 14.07.2026 13:47:35, Oliver Hartkopp wrote:
> > > > Pre-existing issues:
> > > > - [High] Unprotected concurrent access to op->rx_stamp and op->rx_ifindex in bcm_rx_handler causes data races and torn 64-bit reads on 32-bit architectures.
> > >
> > > Fixed in patch 10
> >
> > Pre-existing issues are not a shot stopper, _new_ issues for a patch
> > series are...usually are.
> >
> > Hopefully upstream will accept that you fix issues in a later patch that
> > sashiko claim to be introduced by a patch in the series.
Sorry, let me clarify: This was a general statement.
I don't want to imply that _this_ patch introduces a new issue.
If you remove sashiko from this sentence it was valid before the LLM
review bots. Rule of thumb: You should not introduce "bad" code and fix
it later in a series. - As this is a huge amount of fixes that requires
a lot of code changes, I hope upstream doesn't insist.
> Is it?
>
> Patch 10 Fixes: ffd980f976e7 ("[CAN]: Add broadcast manager (bcm) protocol")
>
> not anything in patch 9.
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH v13 09/11] can: bcm: fix stale rx/tx ops after device removal
2026-07-14 12:36 ` Marc Kleine-Budde
@ 2026-07-14 13:07 ` Oliver Hartkopp
2026-07-14 13:41 ` Marc Kleine-Budde
0 siblings, 1 reply; 32+ messages in thread
From: Oliver Hartkopp @ 2026-07-14 13:07 UTC (permalink / raw)
To: Marc Kleine-Budde
Cc: sashiko-reviews, Oleksij Rempel, linux-can, Vincent Mailhol
On 14.07.26 14:36, Marc Kleine-Budde wrote:
> On 14.07.2026 13:47:35, Oliver Hartkopp wrote:
>>>>> Pre-existing issues:
>>>>> - [High] Unprotected concurrent access to op->rx_stamp and op->rx_ifindex in bcm_rx_handler causes data races and torn 64-bit reads on 32-bit architectures.
>>>>
>>>> Fixed in patch 10
>>>
>>> Pre-existing issues are not a shot stopper, _new_ issues for a patch
>>> series are...usually are.
>>>
>>> Hopefully upstream will accept that you fix issues in a later patch that
>>> sashiko claim to be introduced by a patch in the series.
>
> Sorry, let me clarify: This was a general statement.
>
> I don't want to imply that _this_ patch introduces a new issue.
>
> If you remove sashiko from this sentence it was valid before the LLM
> review bots. Rule of thumb: You should not introduce "bad" code and fix
> it later in a series. - As this is a huge amount of fixes that requires
> a lot of code changes, I hope upstream doesn't insist.
When there was a new issue pointed out by sashiko I fixed it inside the
patch with --amend when it belonged to the topic of the patch.
So I tried to follow the one problem, one patch pattern. And every patch
makes things better and introduces no new regressions (which sashiko-bot
would have detected) - while probably not fixing everything at once.
Best regards,
Oliver
>
>> Is it?
>>
>> Patch 10 Fixes: ffd980f976e7 ("[CAN]: Add broadcast manager (bcm) protocol")
>>
>> not anything in patch 9.
>
> regards,
> Marc
>
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH v13 09/11] can: bcm: fix stale rx/tx ops after device removal
2026-07-14 13:07 ` Oliver Hartkopp
@ 2026-07-14 13:41 ` Marc Kleine-Budde
2026-07-14 14:40 ` Oliver Hartkopp
0 siblings, 1 reply; 32+ messages in thread
From: Marc Kleine-Budde @ 2026-07-14 13:41 UTC (permalink / raw)
To: Oliver Hartkopp
Cc: sashiko-reviews, Oleksij Rempel, linux-can, Vincent Mailhol
[-- Attachment #1: Type: text/plain, Size: 835 bytes --]
On 14.07.2026 15:07:48, Oliver Hartkopp wrote:
> When there was a new issue pointed out by sashiko I fixed it inside the
> patch with --amend when it belonged to the topic of the patch.
You've probably already working on this one:
https://lore.kernel.org/all/20260714115254.D9C2B1F000E9@smtp.kernel.org/
> So I tried to follow the one problem, one patch pattern. And every patch
> makes things better and introduces no new regressions (which sashiko-bot
> would have detected) - while probably not fixing everything at once.
Perfect!
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v13 09/11] can: bcm: fix stale rx/tx ops after device removal
2026-07-14 13:41 ` Marc Kleine-Budde
@ 2026-07-14 14:40 ` Oliver Hartkopp
0 siblings, 0 replies; 32+ messages in thread
From: Oliver Hartkopp @ 2026-07-14 14:40 UTC (permalink / raw)
To: Marc Kleine-Budde
Cc: sashiko-reviews, Oleksij Rempel, linux-can, Vincent Mailhol
On 14.07.26 15:41, Marc Kleine-Budde wrote:
> On 14.07.2026 15:07:48, Oliver Hartkopp wrote:
>> When there was a new issue pointed out by sashiko I fixed it inside the
>> patch with --amend when it belonged to the topic of the patch.
>
> You've probably already working on this one:
> https://lore.kernel.org/all/20260714115254.D9C2B1F000E9@smtp.kernel.org/
>
>> So I tried to follow the one problem, one patch pattern. And every patch
>> makes things better and introduces no new regressions (which sashiko-bot
>> would have detected) - while probably not fixing everything at once.
>
> Perfect!
>
> regards,
> Marc
>
In fact that was still a problem. working on it for patch 3. Easy thing.
Thanks,
Oliver
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v13 10/11] can: bcm: fix data race on rx_stamp/rx_ifindex in bcm_rx_handler()
2026-07-14 10:12 [PATCH v13 00/11] can: bcm: collected fixes - final final Oliver Hartkopp via B4 Relay
` (8 preceding siblings ...)
2026-07-14 10:12 ` [PATCH v13 09/11] can: bcm: fix stale rx/tx ops after device removal Oliver Hartkopp via B4 Relay
@ 2026-07-14 10:12 ` Oliver Hartkopp via B4 Relay
2026-07-14 10:12 ` [PATCH v13 11/11] can: bcm: track a single source interface for ANYDEV timeout/throttle ops Oliver Hartkopp via B4 Relay
10 siblings, 0 replies; 32+ messages in thread
From: Oliver Hartkopp via B4 Relay @ 2026-07-14 10:12 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 10dd99ecce70..0ada2402c24c 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -777,15 +777,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;
-
/* 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) {
@@ -813,10 +808,18 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data)
traffic_flags |= RX_LOCAL;
if (skb->sk == op->sk)
traffic_flags |= RX_OWN;
}
+ /* 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] 32+ messages in thread* [PATCH v13 11/11] can: bcm: track a single source interface for ANYDEV timeout/throttle ops
2026-07-14 10:12 [PATCH v13 00/11] can: bcm: collected fixes - final final Oliver Hartkopp via B4 Relay
` (9 preceding siblings ...)
2026-07-14 10:12 ` [PATCH v13 10/11] can: bcm: fix data race on rx_stamp/rx_ifindex in bcm_rx_handler() Oliver Hartkopp via B4 Relay
@ 2026-07-14 10:12 ` Oliver Hartkopp via B4 Relay
10 siblings, 0 replies; 32+ messages in thread
From: Oliver Hartkopp via B4 Relay @ 2026-07-14 10:12 UTC (permalink / raw)
To: linux-can; +Cc: Oliver Hartkopp, Marc Kleine-Budde, sashiko-bot
From: Oliver Hartkopp <socketcan@hartkopp.net>
An ANYDEV rx op (ifindex == 0) with an active RX timeout and/or
throttle timer has no defined semantics when matching frames arrive
from several interfaces: bcm_rx_handler() can run concurrently for
the same op on different CPUs, racing hrtimer_cancel()/
bcm_rx_starttimer() against bcm_rx_timeout_handler() and causing
spurious RX_TIMEOUT notifications and last_frames corruption. The
same concurrency lets throttled multiplex frames from different
interfaces clobber the single rx_ifindex/rx_stamp fields shared by
the op.
Add op->if_detected to track the first interface that delivers a
matching frame while a timeout/throttle timer is configured, and
reject frames from any other interface for that op. The claim is
decided in bcm_rx_handler() before hrtimer_cancel() touches
op->timer, so a rejected frame can never disturb the claimed
interface's watchdog. RTR-mode ops are excluded via RX_RTR_FRAME,
independent of kt_ival1/kt_ival2, since those may briefly hold a
stale value from an earlier non-RTR configuration.
The claim is released in bcm_notify() on NETDEV_UNREGISTER and in
bcm_rx_setup() when SETTIMER reconfigures the timer values.
A (re-)claim is only possible on CAN devices in NETREG_REGISTERED
dev->reg_state to cover the release in bcm_notify() where reg_state
becomes NETREG_UNREGISTERING until synchronize_net().
Fixes: ffd980f976e7 ("[CAN]: Add broadcast manager (bcm) protocol")
Reported-by: sashiko-bot@kernel.org
Link: https://lore.kernel.org/linux-can/20260709105031.1A39C1F000E9@smtp.kernel.org/
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
net/can/bcm.c | 49 ++++++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 44 insertions(+), 5 deletions(-)
diff --git a/net/can/bcm.c b/net/can/bcm.c
index 0ada2402c24c..a3ce0d49d952 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -115,10 +115,11 @@ struct bcm_op {
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 if_detected; /* first received ifindex in ANYDEV rx_op mode */
int cfsiz;
u32 count;
u32 nframes;
u32 currframe;
/* void pointers to arrays of struct can[fd]_frame */
@@ -774,10 +775,37 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data)
} else {
if (!can_is_can_skb(skb))
return;
}
+ /* An ANYDEV op with an active RX timeout and/or throttle timer
+ * tracks a single source interface: claim the first interface that
+ * delivers a matching frame and reject frames from any other one,
+ * before hrtimer_cancel() below can touch op->timer - this avoids
+ * racing bcm_rx_timeout_handler() across concurrent interfaces.
+ * RX_RTR_FRAME ops are excluded, as kt_ival1/kt_ival2 may briefly
+ * hold a stale value from an earlier non-RTR configuration.
+ */
+ if (!op->ifindex) {
+ spin_lock_bh(&op->bcm_rx_update_lock);
+
+ if (!(op->flags & RX_RTR_FRAME) &&
+ (op->kt_ival1 || op->kt_ival2)) {
+ /* don't claim to vanishing interface */
+ if (!op->if_detected &&
+ READ_ONCE(skb->dev->reg_state) == NETREG_REGISTERED)
+ op->if_detected = skb->dev->ifindex;
+
+ if (op->if_detected != skb->dev->ifindex) {
+ spin_unlock_bh(&op->bcm_rx_update_lock);
+ return;
+ }
+ }
+
+ spin_unlock_bh(&op->bcm_rx_update_lock);
+ }
+
/* disable timeout */
hrtimer_cancel(&op->timer);
/* op->flags/op->frames may be updated concurrently by bcm_rx_setup() */
spin_lock_bh(&op->bcm_rx_update_lock);
@@ -808,14 +836,13 @@ static void bcm_rx_handler(struct sk_buff *skb, void *data)
traffic_flags |= RX_LOCAL;
if (skb->sk == op->sk)
traffic_flags |= RX_OWN;
}
- /* 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.
+ /* save rx timestamp and originator for recvfrom() under lock: an
+ * ANYDEV op without an active timer can still run concurrently on
+ * different CPUs, so content and meta data must be bundled here.
*/
op->rx_stamp = skb->tstamp;
op->rx_ifindex = skb->dev->ifindex;
if (op->flags & RX_FILTER_ID) {
@@ -1439,10 +1466,11 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
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;
+ op->if_detected = 0; /* reclaim ifindex in ANYDEV mode */
spin_unlock_bh(&op->bcm_rx_update_lock);
/* disable an active timer due to zero value? */
if (!op->kt_ival1)
hrtimer_cancel(&op->timer);
@@ -1742,14 +1770,25 @@ static void bcm_notify(struct bcm_sock *bo, unsigned long msg,
case NETDEV_UNREGISTER:
lock_sock(sk);
/* rx_ops: remove device specific receive entries */
- list_for_each_entry(op, &bo->rx_ops, list)
+ 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) {
+ 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
* cyclic bcm_can_tx() calls as there is no re-arming.
*/
list_for_each_entry(op, &bo->tx_ops, list)
--
2.53.0
^ permalink raw reply related [flat|nested] 32+ messages in thread