* [PATCH v3 0/7] can: bcm: collected fixes
@ 2026-07-07 10:16 ` Oliver Hartkopp
0 siblings, 0 replies; 28+ messages in thread
From: Oliver Hartkopp @ 2026-07-07 10:16 UTC (permalink / raw)
To: linux-can
Cc: Oliver Hartkopp, Marc Kleine-Budde, Lee Jones, Ginger,
syzbot+75e5e4ae00c3b4bb544e, sashiko-reviews
Hi Marc,
as there were different patches flying arround to fix CAN_BCM issues and
AI assisted stuff pop's up again and again, I've created this collection
to be applied.
I'm not sure whether sashiko-bot takes the sequence of this series into
account when checking the last patches. So let's see.
Best regards,
Oliver
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
Changes in v3:
- incorporated more sashiko-bot concerns from v2
- patch 3: make snapshot of currframe
- patch 6: add error patch for memcpy_from_msg()
- patch 7: new RTR-reply validation in bcm_rx_setup() from bcm_tx_setup()
- Link to v2: https://patch.msgid.link/20260707-bcm_fixes-v2-0-96620c534b75@hartkopp.net
Changes in v2:
- incorporated all sashiko-bot concerns from v1
- Link to v1: https://patch.msgid.link/20260612-bcm_fixes-v1-0-ca2fa07ee70f@hartkopp.net
---
Lee Jones (1):
can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF
Oliver Hartkopp (6):
can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure
can: bcm: add locking when updating filter and timer values
can: bcm: fix CAN frame rx/tx statistics
can: bcm: add missing rcu list annotations and operations
can: bcm: extend bcm_tx_lock usage for data and timer updates
can: bcm: validate frame length in bcm_rx_setup() for RTR replies
net/can/bcm.c | 416 ++++++++++++++++++++++++++++++++++++++++++++--------------
1 file changed, 314 insertions(+), 102 deletions(-)
---
base-commit: 665159e246749578d4e4bfe106ee3b74edcdab18
change-id: 20260612-bcm_fixes-84fb4aa93ac2
Best regards,
--
Oliver Hartkopp <socketcan@hartkopp.net>
^ permalink raw reply [flat|nested] 28+ messages in thread* [PATCH v3 1/7] can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF
2026-07-07 10:16 ` Oliver Hartkopp
@ 2026-07-07 10:16 ` Oliver Hartkopp
-1 siblings, 0 replies; 28+ messages in thread
From: Oliver Hartkopp via B4 Relay @ 2026-07-07 10:16 UTC (permalink / raw)
To: linux-can; +Cc: Oliver Hartkopp, Marc Kleine-Budde, Lee Jones
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()")
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] 28+ messages in thread* [PATCH v3 1/7] can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF
@ 2026-07-07 10:16 ` Oliver Hartkopp
0 siblings, 0 replies; 28+ messages in thread
From: Oliver Hartkopp @ 2026-07-07 10:16 UTC (permalink / raw)
To: linux-can; +Cc: Oliver Hartkopp, Marc Kleine-Budde, Lee Jones
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()")
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] 28+ messages in thread* Re: [PATCH v3 1/7] can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF
2026-07-07 10:16 ` Oliver Hartkopp
(?)
@ 2026-07-07 10:27 ` sashiko-bot
2026-07-07 11:11 ` Oliver Hartkopp
-1 siblings, 1 reply; 28+ messages in thread
From: sashiko-bot @ 2026-07-07 10:27 UTC (permalink / raw)
To: Oliver Hartkopp
Cc: Oliver Hartkopp, Vincent Mailhol, Marc Kleine-Budde,
Oleksij Rempel, linux-can
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Missing list_add_rcu and list_for_each_entry_rcu for BCM operations lists.
--
--- Patch [1]: [PATCH v3 1/7] can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF ---
commit 3bf6c2fae3452e20dc5bcc0bb3d49f9026358964
Author: Lee Jones <lee@kernel.org>
can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF
This commit replaces direct hrtimer cancellation in softirq context with
a deferred workqueue approach for BCM rx_op deallocation. It queues a work
item from the RCU callback and holds a socket reference until timers are
fully cancelled to avoid a sleep-in-atomic panic.
> 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 since this patch modifies the BCM op
initialization, I noticed a potential race condition with lockless readers.
Later in bcm_tx_setup() and bcm_rx_setup(), the code adds the newly
initialized op to the socket's operation lists using the standard list API:
list_add(&op->list, &bo->tx_ops);
Similarly, bcm_proc_show() iterates over bo->tx_ops locklessly using the
standard list_for_each_entry() macro:
list_for_each_entry(op, &bo->tx_ops, list) {
Because list_add() does not provide smp_store_release() semantics, could a
CPU reorder the initialization of the op's fields or its next pointer?
If this happens, could a concurrent reader in bcm_proc_show() observe a NULL
next pointer or uninitialized object data, potentially causing a crash when
the procfs file is read?
Would it be safer to use list_add_rcu() here and list_for_each_entry_rcu()
for the lockless readers to ensure correct dependency barriers?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260707-bcm_fixes-v3-0-84600ca6d889@hartkopp.net?part=1
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v3 1/7] can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF
2026-07-07 10:27 ` sashiko-bot
@ 2026-07-07 11:11 ` Oliver Hartkopp
0 siblings, 0 replies; 28+ messages in thread
From: Oliver Hartkopp @ 2026-07-07 11:11 UTC (permalink / raw)
To: sashiko-reviews
Cc: Vincent Mailhol, Marc Kleine-Budde, Oleksij Rempel, linux-can
On 07.07.26 12:27, 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 list_add_rcu and list_for_each_entry_rcu for BCM operations lists.
> --
Fixed in Patch 5:
https://lore.kernel.org/linux-can/20260707-bcm_fixes-v3-5-84600ca6d889@hartkopp.net/
>
> --- Patch [1]: [PATCH v3 1/7] can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF ---
> commit 3bf6c2fae3452e20dc5bcc0bb3d49f9026358964
> Author: Lee Jones <lee@kernel.org>
>
> can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF
>
> This commit replaces direct hrtimer cancellation in softirq context with
> a deferred workqueue approach for BCM rx_op deallocation. It queues a work
> item from the RCU callback and holds a socket reference until timers are
> fully cancelled to avoid a sleep-in-atomic panic.
>
>> 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 since this patch modifies the BCM op
> initialization, I noticed a potential race condition with lockless readers.
>
> Later in bcm_tx_setup() and bcm_rx_setup(), the code adds the newly
> initialized op to the socket's operation lists using the standard list API:
>
> list_add(&op->list, &bo->tx_ops);
>
> Similarly, bcm_proc_show() iterates over bo->tx_ops locklessly using the
> standard list_for_each_entry() macro:
>
> list_for_each_entry(op, &bo->tx_ops, list) {
>
> Because list_add() does not provide smp_store_release() semantics, could a
> CPU reorder the initialization of the op's fields or its next pointer?
>
> If this happens, could a concurrent reader in bcm_proc_show() observe a NULL
> next pointer or uninitialized object data, potentially causing a crash when
> the procfs file is read?
>
> Would it be safer to use list_add_rcu() here and list_for_each_entry_rcu()
> for the lockless readers to ensure correct dependency barriers?
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v3 2/7] can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure
2026-07-07 10:16 ` Oliver Hartkopp
@ 2026-07-07 10:16 ` Oliver Hartkopp
-1 siblings, 0 replies; 28+ messages in thread
From: Oliver Hartkopp via B4 Relay @ 2026-07-07 10:16 UTC (permalink / raw)
To: linux-can; +Cc: Oliver Hartkopp, Marc Kleine-Budde, Ginger
From: Oliver Hartkopp <socketcan@hartkopp.net>
bcm_sendmsg() reads bo->ifindex and checks bo->bound before taking
lock_sock(), while bcm_notify(), bcm_connect() and bcm_release() all
mutate both fields under that same lock. Because the lockless reads
and the locked writes are unordered with respect to each other, a
racing bcm_notify() (device unregister) or bcm_connect() (concurrent
bind on another thread sharing the socket) can make bcm_sendmsg()
observe an inconsistent combination, e.g. a stale bound=1 together
with the now-cleared ifindex=0, silently turning a socket bound to a
specific CAN interface into one that also matches "any" interface.
Keep the lockless bo->bound check purely as a fast-path reject, and
move the ifindex read (and a bo->bound re-check) into the locked
section, where every writer already serializes. This removes the
possibility of observing the two fields torn against each other,
rather than trying to fix it with more READ_ONCE()/WRITE_ONCE() pairs
on two independently updated fields. Annotate the now-purely-lockless
bo->bound accesses consistently across all its write sites.
Also fix bcm_rx_setup() silently returning success when the target
device disappears concurrently instead of reporting -ENODEV, so a
broken RX op is no longer left registered as if it had succeeded.
Reported-by: Ginger <ginger.jzllee@gmail.com>
Closes: https://lore.kernel.org/linux-can/CAGp+u1aBK8QVjsvAxM2Ldzep4rEbsP9x_pV3At4g=h1kVEtyhA@mail.gmail.com/
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
net/can/bcm.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++-------------
1 file changed, 51 insertions(+), 14 deletions(-)
diff --git a/net/can/bcm.c b/net/can/bcm.c
index bdf53241bd7b..b612135b017d 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -1321,10 +1321,15 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
bcm_rx_handler, op,
"bcm", sk);
op->rx_reg_dev = dev;
dev_put(dev);
+ } else {
+ /* the requested device is gone - do not
+ * silently succeed without registering
+ */
+ err = -ENODEV;
}
} else
err = can_rx_register(sock_net(sk), NULL, op->can_id,
REGMASK(op->can_id),
@@ -1394,16 +1399,17 @@ static int bcm_tx_send(struct msghdr *msg, int ifindex, struct sock *sk,
*/
static int bcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
{
struct sock *sk = sock->sk;
struct bcm_sock *bo = bcm_sk(sk);
- int ifindex = bo->ifindex; /* default ifindex for this bcm_op */
+ int ifindex;
struct bcm_msg_head msg_head;
int cfsiz;
int ret; /* read bytes or error codes as return value */
- if (!bo->bound)
+ /* Lockless fast-path check for bound socket */
+ if (!READ_ONCE(bo->bound))
return -ENOTCONN;
/* check for valid message length from userspace */
if (size < MHSIZ)
return -EINVAL;
@@ -1415,43 +1421,65 @@ static int bcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
cfsiz = CFSIZ(msg_head.flags);
if ((size - MHSIZ) % cfsiz)
return -EINVAL;
+ lock_sock(sk);
+
+ /* Re-validate under the socket lock: a concurrent bcm_notify()
+ * may have unbound this socket (device removal) after the
+ * lockless fast-path check above. bo->ifindex is only ever
+ * mutated under lock_sock(), so reading it here - instead of
+ * before taking the lock - guarantees it can't be observed
+ * torn against bo->bound.
+ */
+ if (!bo->bound) {
+ ret = -ENOTCONN;
+ goto out_release;
+ }
+
+ /* default ifindex for this bcm_op */
+ ifindex = bo->ifindex;
+
/* check for alternative ifindex for this bcm_op */
if (!ifindex && msg->msg_name) {
/* no bound device as default => check msg_name */
DECLARE_SOCKADDR(struct sockaddr_can *, addr, msg->msg_name);
- if (msg->msg_namelen < BCM_MIN_NAMELEN)
- return -EINVAL;
+ if (msg->msg_namelen < BCM_MIN_NAMELEN) {
+ ret = -EINVAL;
+ goto out_release;
+ }
- if (addr->can_family != AF_CAN)
- return -EINVAL;
+ if (addr->can_family != AF_CAN) {
+ ret = -EINVAL;
+ goto out_release;
+ }
/* ifindex from sendto() */
ifindex = addr->can_ifindex;
if (ifindex) {
struct net_device *dev;
dev = dev_get_by_index(sock_net(sk), ifindex);
- if (!dev)
- return -ENODEV;
+ if (!dev) {
+ ret = -ENODEV;
+ goto out_release;
+ }
if (dev->type != ARPHRD_CAN) {
dev_put(dev);
- return -ENODEV;
+ ret = -ENODEV;
+ goto out_release;
}
dev_put(dev);
}
}
- lock_sock(sk);
-
switch (msg_head.opcode) {
case TX_SETUP:
ret = bcm_tx_setup(&msg_head, msg, ifindex, sk);
break;
@@ -1497,10 +1525,11 @@ static int bcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
default:
ret = -EINVAL;
break;
}
+out_release:
release_sock(sk);
return ret;
}
@@ -1533,11 +1562,16 @@ static void bcm_notify(struct bcm_sock *bo, unsigned long msg,
if (sock_net(sk)->can.bcmproc_dir && bo->bcm_proc_read) {
remove_proc_entry(bo->procname, sock_net(sk)->can.bcmproc_dir);
bo->bcm_proc_read = NULL;
}
#endif
- bo->bound = 0;
+ /* Paired with the lockless fast-path check in
+ * bcm_sendmsg(); bo->ifindex itself is only ever
+ * accessed under lock_sock() so it needs no
+ * annotation.
+ */
+ WRITE_ONCE(bo->bound, 0);
bo->ifindex = 0;
notify_enodev = 1;
}
release_sock(sk);
@@ -1674,11 +1708,11 @@ static int bcm_release(struct socket *sock)
list_for_each_entry_safe(op, next, &bo->rx_ops, list)
bcm_remove_op(op);
/* remove device reference */
if (bo->bound) {
- bo->bound = 0;
+ WRITE_ONCE(bo->bound, 0);
bo->ifindex = 0;
}
sock_orphan(sk);
sock->sk = NULL;
@@ -1744,11 +1778,14 @@ static int bcm_connect(struct socket *sock, struct sockaddr_unsized *uaddr, int
goto fail;
}
}
#endif /* CONFIG_PROC_FS */
- bo->bound = 1;
+ /* bo->ifindex above is fully assigned before this point; pairs
+ * with the lockless fast-path check in bcm_sendmsg()
+ */
+ WRITE_ONCE(bo->bound, 1);
fail:
release_sock(sk);
return ret;
--
2.53.0
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH v3 2/7] can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure
@ 2026-07-07 10:16 ` Oliver Hartkopp
0 siblings, 0 replies; 28+ messages in thread
From: Oliver Hartkopp @ 2026-07-07 10:16 UTC (permalink / raw)
To: linux-can; +Cc: Oliver Hartkopp, Marc Kleine-Budde, Ginger
bcm_sendmsg() reads bo->ifindex and checks bo->bound before taking
lock_sock(), while bcm_notify(), bcm_connect() and bcm_release() all
mutate both fields under that same lock. Because the lockless reads
and the locked writes are unordered with respect to each other, a
racing bcm_notify() (device unregister) or bcm_connect() (concurrent
bind on another thread sharing the socket) can make bcm_sendmsg()
observe an inconsistent combination, e.g. a stale bound=1 together
with the now-cleared ifindex=0, silently turning a socket bound to a
specific CAN interface into one that also matches "any" interface.
Keep the lockless bo->bound check purely as a fast-path reject, and
move the ifindex read (and a bo->bound re-check) into the locked
section, where every writer already serializes. This removes the
possibility of observing the two fields torn against each other,
rather than trying to fix it with more READ_ONCE()/WRITE_ONCE() pairs
on two independently updated fields. Annotate the now-purely-lockless
bo->bound accesses consistently across all its write sites.
Also fix bcm_rx_setup() silently returning success when the target
device disappears concurrently instead of reporting -ENODEV, so a
broken RX op is no longer left registered as if it had succeeded.
Reported-by: Ginger <ginger.jzllee@gmail.com>
Closes: https://lore.kernel.org/linux-can/CAGp+u1aBK8QVjsvAxM2Ldzep4rEbsP9x_pV3At4g=h1kVEtyhA@mail.gmail.com/
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
net/can/bcm.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++-------------
1 file changed, 51 insertions(+), 14 deletions(-)
diff --git a/net/can/bcm.c b/net/can/bcm.c
index bdf53241bd7b..b612135b017d 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -1321,10 +1321,15 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
bcm_rx_handler, op,
"bcm", sk);
op->rx_reg_dev = dev;
dev_put(dev);
+ } else {
+ /* the requested device is gone - do not
+ * silently succeed without registering
+ */
+ err = -ENODEV;
}
} else
err = can_rx_register(sock_net(sk), NULL, op->can_id,
REGMASK(op->can_id),
@@ -1394,16 +1399,17 @@ static int bcm_tx_send(struct msghdr *msg, int ifindex, struct sock *sk,
*/
static int bcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
{
struct sock *sk = sock->sk;
struct bcm_sock *bo = bcm_sk(sk);
- int ifindex = bo->ifindex; /* default ifindex for this bcm_op */
+ int ifindex;
struct bcm_msg_head msg_head;
int cfsiz;
int ret; /* read bytes or error codes as return value */
- if (!bo->bound)
+ /* Lockless fast-path check for bound socket */
+ if (!READ_ONCE(bo->bound))
return -ENOTCONN;
/* check for valid message length from userspace */
if (size < MHSIZ)
return -EINVAL;
@@ -1415,43 +1421,65 @@ static int bcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
cfsiz = CFSIZ(msg_head.flags);
if ((size - MHSIZ) % cfsiz)
return -EINVAL;
+ lock_sock(sk);
+
+ /* Re-validate under the socket lock: a concurrent bcm_notify()
+ * may have unbound this socket (device removal) after the
+ * lockless fast-path check above. bo->ifindex is only ever
+ * mutated under lock_sock(), so reading it here - instead of
+ * before taking the lock - guarantees it can't be observed
+ * torn against bo->bound.
+ */
+ if (!bo->bound) {
+ ret = -ENOTCONN;
+ goto out_release;
+ }
+
+ /* default ifindex for this bcm_op */
+ ifindex = bo->ifindex;
+
/* check for alternative ifindex for this bcm_op */
if (!ifindex && msg->msg_name) {
/* no bound device as default => check msg_name */
DECLARE_SOCKADDR(struct sockaddr_can *, addr, msg->msg_name);
- if (msg->msg_namelen < BCM_MIN_NAMELEN)
- return -EINVAL;
+ if (msg->msg_namelen < BCM_MIN_NAMELEN) {
+ ret = -EINVAL;
+ goto out_release;
+ }
- if (addr->can_family != AF_CAN)
- return -EINVAL;
+ if (addr->can_family != AF_CAN) {
+ ret = -EINVAL;
+ goto out_release;
+ }
/* ifindex from sendto() */
ifindex = addr->can_ifindex;
if (ifindex) {
struct net_device *dev;
dev = dev_get_by_index(sock_net(sk), ifindex);
- if (!dev)
- return -ENODEV;
+ if (!dev) {
+ ret = -ENODEV;
+ goto out_release;
+ }
if (dev->type != ARPHRD_CAN) {
dev_put(dev);
- return -ENODEV;
+ ret = -ENODEV;
+ goto out_release;
}
dev_put(dev);
}
}
- lock_sock(sk);
-
switch (msg_head.opcode) {
case TX_SETUP:
ret = bcm_tx_setup(&msg_head, msg, ifindex, sk);
break;
@@ -1497,10 +1525,11 @@ static int bcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
default:
ret = -EINVAL;
break;
}
+out_release:
release_sock(sk);
return ret;
}
@@ -1533,11 +1562,16 @@ static void bcm_notify(struct bcm_sock *bo, unsigned long msg,
if (sock_net(sk)->can.bcmproc_dir && bo->bcm_proc_read) {
remove_proc_entry(bo->procname, sock_net(sk)->can.bcmproc_dir);
bo->bcm_proc_read = NULL;
}
#endif
- bo->bound = 0;
+ /* Paired with the lockless fast-path check in
+ * bcm_sendmsg(); bo->ifindex itself is only ever
+ * accessed under lock_sock() so it needs no
+ * annotation.
+ */
+ WRITE_ONCE(bo->bound, 0);
bo->ifindex = 0;
notify_enodev = 1;
}
release_sock(sk);
@@ -1674,11 +1708,11 @@ static int bcm_release(struct socket *sock)
list_for_each_entry_safe(op, next, &bo->rx_ops, list)
bcm_remove_op(op);
/* remove device reference */
if (bo->bound) {
- bo->bound = 0;
+ WRITE_ONCE(bo->bound, 0);
bo->ifindex = 0;
}
sock_orphan(sk);
sock->sk = NULL;
@@ -1744,11 +1778,14 @@ static int bcm_connect(struct socket *sock, struct sockaddr_unsized *uaddr, int
goto fail;
}
}
#endif /* CONFIG_PROC_FS */
- bo->bound = 1;
+ /* bo->ifindex above is fully assigned before this point; pairs
+ * with the lockless fast-path check in bcm_sendmsg()
+ */
+ WRITE_ONCE(bo->bound, 1);
fail:
release_sock(sk);
return ret;
--
2.53.0
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v3 2/7] can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure
2026-07-07 10:16 ` Oliver Hartkopp
(?)
@ 2026-07-07 10:29 ` sashiko-bot
2026-07-07 11:16 ` Oliver Hartkopp
-1 siblings, 1 reply; 28+ messages in thread
From: sashiko-bot @ 2026-07-07 10:29 UTC (permalink / raw)
To: Oliver Hartkopp
Cc: Oleksij Rempel, Marc Kleine-Budde, Oliver Hartkopp, linux-can,
Vincent Mailhol
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues:
- [High] Unprotected and non-RCU-safe list manipulations on `bo->tx_ops` and `bo->rx_ops` cause data races with lockless procfs readers.
- [High] Concurrent update of `op->frames` causes a data race with softirq CAN TX processing, sending corrupted frames.
- [Medium] Concurrent update of `op->frames` causes a data race with softirq CAN RX processing.
--
--- Patch [2]: [PATCH v3 2/7] can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure ---
commit 8b8711c2150aeadcb80c48a64d2a2f7206bc9502
Author: Oliver Hartkopp <socketcan@hartkopp.net>
can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure
This commit fixes a race condition where bcm_sendmsg() could read an
inconsistent bo->bound and bo->ifindex state during concurrent socket
operations. It addresses this by moving the ifindex read and bound validation
inside lock_sock(). The patch also fixes bcm_rx_setup() to correctly return
-ENODEV instead of silently succeeding when a device disappears concurrently.
Closes: 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;
> }
[Severity: Medium]
This isn't a bug introduced by this patch, but does the concurrent update of
op->frames cause a data race with softirq CAN RX processing?
In bcm_rx_setup(), when updating an existing RX filter, the code copies from
userspace without excluding the softirq path:
bcm_rx_setup() {
...
err = memcpy_from_msg(op->frames, msg, msg_head->nframes * op->cfsiz);
...
}
Since memcpy_from_msg() can sleep and the CAN RX softirq runs concurrently,
bcm_rx_cmp_to_index() might execute mid-update. It locklessly reads op->frames
and op->last_frames to match multiplexed CAN frames:
bcm_rx_cmp_to_index() {
...
if ((get_u64(cf, i) & get_u64(rxdata, i)) != (get_u64(cf, i) & get_u64(lcf, i))) {
...
}
Can this lead to torn reads where the BCM spuriously triggers or fails to
trigger filter updates based on corrupted matching data?
[ ... ]
> @@ -1436,20 +1463,21 @@ static int bcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
[ ... ]
> switch (msg_head.opcode) {
>
> case TX_SETUP:
[Severity: High]
This is a pre-existing issue, but do these list insertions lack RCU write
barriers?
bcm_proc_show() iterates bo->tx_ops and bo->rx_ops locklessly using
rcu_read_lock(). However, bcm_tx_setup() and bcm_rx_setup() insert new
elements using list_add() instead of list_add_rcu(), which lacks the
smp_store_release() write barrier required to safely publish initialized
bcm_op structs to concurrent RCU readers.
Additionally, bcm_proc_show() appears to use the non-RCU list_for_each_entry()
macro for bo->tx_ops, omitting the required read barrier. Could an unprivileged
local user reading /proc/net/can-bcm/* concurrently trigger a panic?
[Severity: High]
This is another pre-existing issue, but does a similar concurrent update of
op->frames cause a data race with softirq CAN TX processing?
In bcm_tx_setup(), when updating an existing TX operation, memcpy_from_msg()
overwrites the payload in op->frames without acquiring op->bcm_tx_lock or
excluding the softirq:
bcm_tx_setup() {
...
err = memcpy_from_msg((u8 *)cf, msg, op->cfsiz);
...
}
Concurrently, the hrtimer callback bcm_tx_timeout_handler() can fire and invoke
bcm_can_tx(). bcm_can_tx() retrieves the frame pointer under the lock, but
accesses the underlying frame data locklessly outside the lock:
bcm_can_tx() {
...
skb_put_data(skb, cf, op->cfsiz);
...
}
Could this allow the softirq to transmit a partially updated or torn CAN frame
onto the physical CAN bus?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260707-bcm_fixes-v3-0-84600ca6d889@hartkopp.net?part=2
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v3 2/7] can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure
2026-07-07 10:29 ` sashiko-bot
@ 2026-07-07 11:16 ` Oliver Hartkopp
0 siblings, 0 replies; 28+ messages in thread
From: Oliver Hartkopp @ 2026-07-07 11:16 UTC (permalink / raw)
To: sashiko-reviews
Cc: Oleksij Rempel, Marc Kleine-Budde, linux-can, Vincent Mailhol
On 07.07.26 12:29, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] Unprotected and non-RCU-safe list manipulations on `bo->tx_ops` and `bo->rx_ops` cause data races with lockless procfs readers.
Fixed in patch 5:
https://lore.kernel.org/linux-can/20260707-bcm_fixes-v3-5-84600ca6d889@hartkopp.net/
> - [High] Concurrent update of `op->frames` causes a data race with softirq CAN TX processing, sending corrupted frames.
Fixed in Patch 3:
https://lore.kernel.org/linux-can/20260707-bcm_fixes-v3-3-84600ca6d889@hartkopp.net/
> - [Medium] Concurrent update of `op->frames` causes a data race with softirq CAN RX processing.
> --
Fixed in Patch 3:
https://lore.kernel.org/linux-can/20260707-bcm_fixes-v3-3-84600ca6d889@hartkopp.net/
>
> --- Patch [2]: [PATCH v3 2/7] can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure ---
> commit 8b8711c2150aeadcb80c48a64d2a2f7206bc9502
> Author: Oliver Hartkopp <socketcan@hartkopp.net>
>
> can: bcm: fix lockless bound/ifindex race and silent RX_SETUP failure
>
> This commit fixes a race condition where bcm_sendmsg() could read an
> inconsistent bo->bound and bo->ifindex state during concurrent socket
> operations. It addresses this by moving the ifindex read and bound validation
> inside lock_sock(). The patch also fixes bcm_rx_setup() to correctly return
> -ENODEV instead of silently succeeding when a device disappears concurrently.
>
> Closes: 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;
>> }
>
> [Severity: Medium]
> This isn't a bug introduced by this patch, but does the concurrent update of
> op->frames cause a data race with softirq CAN RX processing?
>
> In bcm_rx_setup(), when updating an existing RX filter, the code copies from
> userspace without excluding the softirq path:
>
> bcm_rx_setup() {
> ...
> err = memcpy_from_msg(op->frames, msg, msg_head->nframes * op->cfsiz);
> ...
> }
>
> Since memcpy_from_msg() can sleep and the CAN RX softirq runs concurrently,
> bcm_rx_cmp_to_index() might execute mid-update. It locklessly reads op->frames
> and op->last_frames to match multiplexed CAN frames:
>
> bcm_rx_cmp_to_index() {
> ...
> if ((get_u64(cf, i) & get_u64(rxdata, i)) != (get_u64(cf, i) & get_u64(lcf, i))) {
> ...
> }
>
> Can this lead to torn reads where the BCM spuriously triggers or fails to
> trigger filter updates based on corrupted matching data?
>
> [ ... ]
>> @@ -1436,20 +1463,21 @@ static int bcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
> [ ... ]
>> switch (msg_head.opcode) {
>>
>> case TX_SETUP:
>
> [Severity: High]
> This is a pre-existing issue, but do these list insertions lack RCU write
> barriers?
>
> bcm_proc_show() iterates bo->tx_ops and bo->rx_ops locklessly using
> rcu_read_lock(). However, bcm_tx_setup() and bcm_rx_setup() insert new
> elements using list_add() instead of list_add_rcu(), which lacks the
> smp_store_release() write barrier required to safely publish initialized
> bcm_op structs to concurrent RCU readers.
>
> Additionally, bcm_proc_show() appears to use the non-RCU list_for_each_entry()
> macro for bo->tx_ops, omitting the required read barrier. Could an unprivileged
> local user reading /proc/net/can-bcm/* concurrently trigger a panic?
>
> [Severity: High]
> This is another pre-existing issue, but does a similar concurrent update of
> op->frames cause a data race with softirq CAN TX processing?
>
> In bcm_tx_setup(), when updating an existing TX operation, memcpy_from_msg()
> overwrites the payload in op->frames without acquiring op->bcm_tx_lock or
> excluding the softirq:
>
> bcm_tx_setup() {
> ...
> err = memcpy_from_msg((u8 *)cf, msg, op->cfsiz);
> ...
> }
>
> Concurrently, the hrtimer callback bcm_tx_timeout_handler() can fire and invoke
> bcm_can_tx(). bcm_can_tx() retrieves the frame pointer under the lock, but
> accesses the underlying frame data locklessly outside the lock:
>
> bcm_can_tx() {
> ...
> skb_put_data(skb, cf, op->cfsiz);
> ...
> }
>
> Could this allow the softirq to transmit a partially updated or torn CAN frame
> onto the physical CAN bus?
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v3 3/7] can: bcm: add locking when updating filter and timer values
2026-07-07 10:16 ` Oliver Hartkopp
@ 2026-07-07 10:16 ` Oliver Hartkopp
-1 siblings, 0 replies; 28+ messages in thread
From: Oliver Hartkopp via B4 Relay @ 2026-07-07 10:16 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: CAN loopback on
echo-capable interfaces (e.g. vcan) can deliver the transmitted skb
back into bcm_rx_handler() synchronously from within dev_queue_xmit(),
which would self-deadlock on the per-op lock if it were still held.
Also take a lock-protected snapshot of the currframe in bcm_can_tx()
to avoid partly overwrites by content updates in bcm_tx_setup().
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 | 133 ++++++++++++++++++++++++++++++++++++++++++++--------------
1 file changed, 102 insertions(+), 31 deletions(-)
diff --git a/net/can/bcm.c b/net/can/bcm.c
index b612135b017d..fd548c6eaa91 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,39 @@ 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)) {
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 +745,25 @@ 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(). Do not call bcm_can_tx() while
+ * holding bcm_rx_update_lock: CAN loopback on echo-capable
+ * interfaces (e.g. vcan) can deliver the transmitted skb back into
+ * bcm_rx_handler() synchronously from within dev_queue_xmit().
+ */
+ 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 +771,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 +808,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 +1148,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 +1162,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 +1214,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 +1227,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 +1313,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 +1342,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] 28+ messages in thread* [PATCH v3 3/7] can: bcm: add locking when updating filter and timer values
@ 2026-07-07 10:16 ` Oliver Hartkopp
0 siblings, 0 replies; 28+ messages in thread
From: Oliver Hartkopp @ 2026-07-07 10:16 UTC (permalink / raw)
To: linux-can; +Cc: Oliver Hartkopp, Marc Kleine-Budde, syzbot+75e5e4ae00c3b4bb544e
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: CAN loopback on
echo-capable interfaces (e.g. vcan) can deliver the transmitted skb
back into bcm_rx_handler() synchronously from within dev_queue_xmit(),
which would self-deadlock on the per-op lock if it were still held.
Also take a lock-protected snapshot of the currframe in bcm_can_tx()
to avoid partly overwrites by content updates in bcm_tx_setup().
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 | 133 ++++++++++++++++++++++++++++++++++++++++++++--------------
1 file changed, 102 insertions(+), 31 deletions(-)
diff --git a/net/can/bcm.c b/net/can/bcm.c
index b612135b017d..fd548c6eaa91 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,39 @@ 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)) {
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 +745,25 @@ 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(). Do not call bcm_can_tx() while
+ * holding bcm_rx_update_lock: CAN loopback on echo-capable
+ * interfaces (e.g. vcan) can deliver the transmitted skb back into
+ * bcm_rx_handler() synchronously from within dev_queue_xmit().
+ */
+ 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 +771,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 +808,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 +1148,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 +1162,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 +1214,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 +1227,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 +1313,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 +1342,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] 28+ messages in thread* Re: [PATCH v3 3/7] can: bcm: add locking when updating filter and timer values
2026-07-07 10:16 ` Oliver Hartkopp
(?)
@ 2026-07-07 10:35 ` sashiko-bot
2026-07-07 11:32 ` Oliver Hartkopp
-1 siblings, 1 reply; 28+ messages in thread
From: sashiko-bot @ 2026-07-07 10:35 UTC (permalink / raw)
To: Oliver Hartkopp
Cc: Vincent Mailhol, Oliver Hartkopp, linux-can, Marc Kleine-Budde,
Oleksij Rempel
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] Concurrent timer updates lead to hrtimer_forward_now being called with a zero interval, triggering a WARN_ON.
- [High] Missing locking in the TX path leads to data races and torn reads for CAN frames and timer values.
--
--- Patch [3]: [PATCH v3 3/7] can: bcm: add locking when updating filter and timer values ---
commit 29dee20a547c38c99b2b856375f9e172e6a33181
Author: Oliver Hartkopp <socketcan@hartkopp.net>
can: bcm: add locking when updating filter and timer values
KCSAN detected a simultaneous access to timer values that can be
overwritten in bcm_rx_setup() when updating timer and filter content
while bcm_rx_handler(), bcm_rx_timeout_handler() or bcm_rx_thr_handler()
run concurrently on incoming CAN traffic.
Closes: 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..fd548c6eaa913 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -293,22 +294,26 @@ 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);
> + }
[Severity: High]
This isn't a bug introduced by this patch, but does bcm_tx_lock actually
prevent concurrent overwrites by bcm_tx_setup()?
The commit message claims to "take a lock-protected snapshot of the currframe
in bcm_can_tx() to avoid partly overwrites by content updates in
bcm_tx_setup()".
However, looking at bcm_tx_setup(), it appears to copy user data directly into
op->frames without holding bcm_tx_lock:
net/can/bcm.c:bcm_tx_setup() {
...
for (i = 0; i < msg_head->nframes; i++) {
cf = op->frames + op->cfsiz * i;
err = memcpy_from_msg((u8 *)cf, msg, op->cfsiz);
...
}
If the writer never acquires the lock (since memcpy_from_msg() can sleep),
doesn't this lock in bcm_can_tx() remain ineffective against races?
[ ... ]
> @@ -461,7 +466,7 @@ static enum hrtimer_restart bcm_tx_timeout_handler(struct hrtimer *hrtimer)
> struct bcm_msg_head msg_head;
>
> if (op->kt_ival1 && (op->count > 0)) {
[Severity: High]
This is a pre-existing issue, but is this lockless read of kt_ival1 vulnerable
to torn reads on 32-bit systems?
bcm_tx_setup() appears to update kt_ival1 and kt_ival2 locklessly, and this
handler reads them locklessly. Could this cause the timer to fire at
unintended intervals if a 64-bit ktime_t is partially updated while being read?
> - bcm_can_tx(op);
> + bcm_can_tx(op, NULL);
> if (!op->count && (op->flags & TX_COUNTEVT)) {
[ ... ]
> @@ -686,15 +695,22 @@ static int bcm_rx_thr_flush(struct bcm_op *op)
> 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)) {
> hrtimer_forward_now(hrtimer, op->kt_ival2);
[Severity: High]
This is a pre-existing issue, but could hrtimer_forward_now() be called with a
zero interval, triggering a WARN_ON_ONCE?
If a user updates an RX operation with msg_head->nframes == 0 and ival2 == 0,
bcm_rx_setup() updates op->kt_ival2 = 0 but does not clear op->last_frames.
It drops the lock before cancelling the timer:
net/can/bcm.c:bcm_rx_setup() {
...
spin_lock_bh(&op->bcm_rx_update_lock);
op->ival2 = msg_head->ival2;
op->kt_ival2 = bcm_timeval_to_ktime(msg_head->ival2);
...
spin_unlock_bh(&op->bcm_rx_update_lock);
/* disable an active timer due to zero value? */
if (!op->kt_ival1)
hrtimer_cancel(&op->timer);
hrtimer_cancel(&op->thrtimer);
...
}
Because hrtimer_cancel(&op->thrtimer) is called without the
bcm_rx_update_lock held, could a concurrently expiring bcm_rx_thr_handler()
acquire the lock, flush the still-throttled frame, and call
hrtimer_forward_now(hrtimer, op->kt_ival2) with an interval of 0?
> - 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;
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260707-bcm_fixes-v3-0-84600ca6d889@hartkopp.net?part=3
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v3 3/7] can: bcm: add locking when updating filter and timer values
2026-07-07 10:35 ` sashiko-bot
@ 2026-07-07 11:32 ` Oliver Hartkopp
0 siblings, 0 replies; 28+ messages in thread
From: Oliver Hartkopp @ 2026-07-07 11:32 UTC (permalink / raw)
To: sashiko-reviews
Cc: Vincent Mailhol, linux-can, Marc Kleine-Budde, Oleksij Rempel
On 07.07.26 12:35, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] Concurrent timer updates lead to hrtimer_forward_now being called with a zero interval, triggering a WARN_ON.
Will be fixed in V4
> - [High] Missing locking in the TX path leads to data races and torn reads for CAN frames and timer values.
Fixed in patch 6:
https://lore.kernel.org/linux-can/20260707-bcm_fixes-v3-6-84600ca6d889@hartkopp.net/
> --
>
> --- Patch [3]: [PATCH v3 3/7] can: bcm: add locking when updating filter and timer values ---
> commit 29dee20a547c38c99b2b856375f9e172e6a33181
> Author: Oliver Hartkopp <socketcan@hartkopp.net>
>
> can: bcm: add locking when updating filter and timer values
>
> KCSAN detected a simultaneous access to timer values that can be
> overwritten in bcm_rx_setup() when updating timer and filter content
> while bcm_rx_handler(), bcm_rx_timeout_handler() or bcm_rx_thr_handler()
> run concurrently on incoming CAN traffic.
>
> Closes: 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..fd548c6eaa913 100644
>> --- a/net/can/bcm.c
>> +++ b/net/can/bcm.c
>> @@ -293,22 +294,26 @@ 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);
>> + }
>
> [Severity: High]
> This isn't a bug introduced by this patch, but does bcm_tx_lock actually
> prevent concurrent overwrites by bcm_tx_setup()?
>
> The commit message claims to "take a lock-protected snapshot of the currframe
> in bcm_can_tx() to avoid partly overwrites by content updates in
> bcm_tx_setup()".
>
> However, looking at bcm_tx_setup(), it appears to copy user data directly into
> op->frames without holding bcm_tx_lock:
>
> net/can/bcm.c:bcm_tx_setup() {
> ...
> for (i = 0; i < msg_head->nframes; i++) {
> cf = op->frames + op->cfsiz * i;
> err = memcpy_from_msg((u8 *)cf, msg, op->cfsiz);
> ...
> }
>
> If the writer never acquires the lock (since memcpy_from_msg() can sleep),
> doesn't this lock in bcm_can_tx() remain ineffective against races?
>
> [ ... ]
>
>> @@ -461,7 +466,7 @@ static enum hrtimer_restart bcm_tx_timeout_handler(struct hrtimer *hrtimer)
>> struct bcm_msg_head msg_head;
>>
>> if (op->kt_ival1 && (op->count > 0)) {
>
> [Severity: High]
> This is a pre-existing issue, but is this lockless read of kt_ival1 vulnerable
> to torn reads on 32-bit systems?
>
> bcm_tx_setup() appears to update kt_ival1 and kt_ival2 locklessly, and this
> handler reads them locklessly. Could this cause the timer to fire at
> unintended intervals if a 64-bit ktime_t is partially updated while being read?
>
>> - bcm_can_tx(op);
>> + bcm_can_tx(op, NULL);
>> if (!op->count && (op->flags & TX_COUNTEVT)) {
>
> [ ... ]
>
>> @@ -686,15 +695,22 @@ static int bcm_rx_thr_flush(struct bcm_op *op)
>> 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)) {
>> hrtimer_forward_now(hrtimer, op->kt_ival2);
>
> [Severity: High]
> This is a pre-existing issue, but could hrtimer_forward_now() be called with a
> zero interval, triggering a WARN_ON_ONCE?
>
> If a user updates an RX operation with msg_head->nframes == 0 and ival2 == 0,
> bcm_rx_setup() updates op->kt_ival2 = 0 but does not clear op->last_frames.
> It drops the lock before cancelling the timer:
>
> net/can/bcm.c:bcm_rx_setup() {
> ...
> spin_lock_bh(&op->bcm_rx_update_lock);
> op->ival2 = msg_head->ival2;
> op->kt_ival2 = bcm_timeval_to_ktime(msg_head->ival2);
> ...
> spin_unlock_bh(&op->bcm_rx_update_lock);
>
> /* disable an active timer due to zero value? */
> if (!op->kt_ival1)
> hrtimer_cancel(&op->timer);
>
> hrtimer_cancel(&op->thrtimer);
> ...
> }
>
> Because hrtimer_cancel(&op->thrtimer) is called without the
> bcm_rx_update_lock held, could a concurrently expiring bcm_rx_thr_handler()
> acquire the lock, flush the still-throttled frame, and call
> hrtimer_forward_now(hrtimer, op->kt_ival2) with an interval of 0?
>
>> - 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;
>> }
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v3 4/7] can: bcm: fix CAN frame rx/tx statistics
2026-07-07 10:16 ` Oliver Hartkopp
@ 2026-07-07 10:16 ` Oliver Hartkopp
-1 siblings, 0 replies; 28+ messages in thread
From: Oliver Hartkopp via B4 Relay @ 2026-07-07 10:16 UTC (permalink / raw)
To: linux-can; +Cc: Oliver Hartkopp, Marc Kleine-Budde
From: Oliver Hartkopp <socketcan@hartkopp.net>
KCSAN detected a data race within the bcm_rx_handler() when two CAN frames
have been simultaneously received and processed in a single rx op by two
different CPUs.
Use atomic operations with (signed) long data types to access the
statistics in the hot path to fix the KCSAN complaint.
Additionally simplify the update and check of statistics overflow by
using the atomic operations in a separate bcm_update_stats() function.
Fixes: ffd980f976e7 ("[CAN]: Add broadcast manager (bcm) protocol")
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
net/can/bcm.c | 38 ++++++++++++++++++++++++--------------
1 file changed, 24 insertions(+), 14 deletions(-)
diff --git a/net/can/bcm.c b/net/can/bcm.c
index fd548c6eaa91..0378e31ea10e 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -110,11 +110,11 @@ struct bcm_op {
struct rcu_head rcu;
struct work_struct work;
int ifindex;
canid_t can_id;
u32 flags;
- unsigned long frames_abs, frames_filtered;
+ atomic_long_t frames_abs, frames_filtered;
struct bcm_timeval ival1, ival2;
struct hrtimer timer, thrtimer;
ktime_t rx_stamp, kt_ival1, kt_ival2, kt_lastmsg;
int rx_ifindex;
int cfsiz;
@@ -227,14 +227,17 @@ static int bcm_proc_show(struct seq_file *m, void *v)
rcu_read_lock();
list_for_each_entry_rcu(op, &bo->rx_ops, list) {
- unsigned long reduction;
+ long reduction, frames_filtered, frames_abs;
+
+ frames_filtered = atomic_long_read(&op->frames_filtered);
+ frames_abs = atomic_long_read(&op->frames_abs);
/* print only active entries & prevent division by zero */
- if (!op->frames_abs)
+ if (!frames_abs)
continue;
seq_printf(m, "rx_op: %03X %-5s ", op->can_id,
bcm_proc_getifname(net, ifname, op->ifindex));
@@ -252,13 +255,13 @@ static int bcm_proc_show(struct seq_file *m, void *v)
if (op->kt_ival2)
seq_printf(m, "thr=%lld ",
(long long)ktime_to_us(op->kt_ival2));
seq_printf(m, "# recv %ld (%ld) => reduction: ",
- op->frames_filtered, op->frames_abs);
+ frames_filtered, frames_abs);
- reduction = 100 - (op->frames_filtered * 100) / op->frames_abs;
+ reduction = 100 - (frames_filtered * 100) / frames_abs;
seq_printf(m, "%s%ld%%\n",
(reduction == 100) ? "near " : "", reduction);
}
@@ -278,20 +281,30 @@ static int bcm_proc_show(struct seq_file *m, void *v)
if (op->kt_ival2)
seq_printf(m, "t2=%lld ",
(long long)ktime_to_us(op->kt_ival2));
- seq_printf(m, "# sent %ld\n", op->frames_abs);
+ seq_printf(m, "# sent %ld\n",
+ atomic_long_read(&op->frames_abs));
}
seq_putc(m, '\n');
rcu_read_unlock();
return 0;
}
#endif /* CONFIG_PROC_FS */
+static void bcm_update_stats(struct bcm_op *op)
+{
+ /* prevent statistics overflow */
+ if (atomic_long_inc_return(&op->frames_abs) > LONG_MAX / 100) {
+ atomic_long_set(&op->frames_filtered, 0);
+ atomic_long_set(&op->frames_abs, 0);
+ }
+}
+
/*
* bcm_can_tx - send the (next) CAN frame to the appropriate CAN interface
* of the given bcm tx op
*/
static void bcm_can_tx(struct bcm_op *op, struct canfd_frame *cf)
@@ -342,11 +355,11 @@ static void bcm_can_tx(struct bcm_op *op, struct canfd_frame *cf)
/* update currframe and count under lock protection */
spin_lock_bh(&op->bcm_tx_lock);
if (!err)
- op->frames_abs++;
+ bcm_update_stats(op);
op->currframe++;
/* reached last frame? */
if (op->currframe >= op->nframes)
@@ -495,16 +508,13 @@ static enum hrtimer_restart bcm_tx_timeout_handler(struct hrtimer *hrtimer)
*/
static void bcm_rx_changed(struct bcm_op *op, struct canfd_frame *data)
{
struct bcm_msg_head head;
- /* update statistics */
- op->frames_filtered++;
-
- /* prevent statistics overflow */
- if (op->frames_filtered > ULONG_MAX/100)
- op->frames_filtered = op->frames_abs = 0;
+ /* update statistics (frames_filtered <= frames_abs) */
+ if (atomic_long_read(&op->frames_abs))
+ atomic_long_inc(&op->frames_filtered);
/* this element is not throttled anymore */
data->flags &= ~RX_THR;
memset(&head, 0, sizeof(head));
@@ -743,11 +753,11 @@ 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++;
+ bcm_update_stats(op);
/* snapshot the flag under lock: op->flags/op->frames may be updated
* concurrently by bcm_rx_setup(). Do not call bcm_can_tx() while
* holding bcm_rx_update_lock: CAN loopback on echo-capable
* interfaces (e.g. vcan) can deliver the transmitted skb back into
--
2.53.0
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH v3 4/7] can: bcm: fix CAN frame rx/tx statistics
@ 2026-07-07 10:16 ` Oliver Hartkopp
0 siblings, 0 replies; 28+ messages in thread
From: Oliver Hartkopp @ 2026-07-07 10:16 UTC (permalink / raw)
To: linux-can; +Cc: Oliver Hartkopp, Marc Kleine-Budde
KCSAN detected a data race within the bcm_rx_handler() when two CAN frames
have been simultaneously received and processed in a single rx op by two
different CPUs.
Use atomic operations with (signed) long data types to access the
statistics in the hot path to fix the KCSAN complaint.
Additionally simplify the update and check of statistics overflow by
using the atomic operations in a separate bcm_update_stats() function.
Fixes: ffd980f976e7 ("[CAN]: Add broadcast manager (bcm) protocol")
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
net/can/bcm.c | 38 ++++++++++++++++++++++++--------------
1 file changed, 24 insertions(+), 14 deletions(-)
diff --git a/net/can/bcm.c b/net/can/bcm.c
index fd548c6eaa91..0378e31ea10e 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -110,11 +110,11 @@ struct bcm_op {
struct rcu_head rcu;
struct work_struct work;
int ifindex;
canid_t can_id;
u32 flags;
- unsigned long frames_abs, frames_filtered;
+ atomic_long_t frames_abs, frames_filtered;
struct bcm_timeval ival1, ival2;
struct hrtimer timer, thrtimer;
ktime_t rx_stamp, kt_ival1, kt_ival2, kt_lastmsg;
int rx_ifindex;
int cfsiz;
@@ -227,14 +227,17 @@ static int bcm_proc_show(struct seq_file *m, void *v)
rcu_read_lock();
list_for_each_entry_rcu(op, &bo->rx_ops, list) {
- unsigned long reduction;
+ long reduction, frames_filtered, frames_abs;
+
+ frames_filtered = atomic_long_read(&op->frames_filtered);
+ frames_abs = atomic_long_read(&op->frames_abs);
/* print only active entries & prevent division by zero */
- if (!op->frames_abs)
+ if (!frames_abs)
continue;
seq_printf(m, "rx_op: %03X %-5s ", op->can_id,
bcm_proc_getifname(net, ifname, op->ifindex));
@@ -252,13 +255,13 @@ static int bcm_proc_show(struct seq_file *m, void *v)
if (op->kt_ival2)
seq_printf(m, "thr=%lld ",
(long long)ktime_to_us(op->kt_ival2));
seq_printf(m, "# recv %ld (%ld) => reduction: ",
- op->frames_filtered, op->frames_abs);
+ frames_filtered, frames_abs);
- reduction = 100 - (op->frames_filtered * 100) / op->frames_abs;
+ reduction = 100 - (frames_filtered * 100) / frames_abs;
seq_printf(m, "%s%ld%%\n",
(reduction == 100) ? "near " : "", reduction);
}
@@ -278,20 +281,30 @@ static int bcm_proc_show(struct seq_file *m, void *v)
if (op->kt_ival2)
seq_printf(m, "t2=%lld ",
(long long)ktime_to_us(op->kt_ival2));
- seq_printf(m, "# sent %ld\n", op->frames_abs);
+ seq_printf(m, "# sent %ld\n",
+ atomic_long_read(&op->frames_abs));
}
seq_putc(m, '\n');
rcu_read_unlock();
return 0;
}
#endif /* CONFIG_PROC_FS */
+static void bcm_update_stats(struct bcm_op *op)
+{
+ /* prevent statistics overflow */
+ if (atomic_long_inc_return(&op->frames_abs) > LONG_MAX / 100) {
+ atomic_long_set(&op->frames_filtered, 0);
+ atomic_long_set(&op->frames_abs, 0);
+ }
+}
+
/*
* bcm_can_tx - send the (next) CAN frame to the appropriate CAN interface
* of the given bcm tx op
*/
static void bcm_can_tx(struct bcm_op *op, struct canfd_frame *cf)
@@ -342,11 +355,11 @@ static void bcm_can_tx(struct bcm_op *op, struct canfd_frame *cf)
/* update currframe and count under lock protection */
spin_lock_bh(&op->bcm_tx_lock);
if (!err)
- op->frames_abs++;
+ bcm_update_stats(op);
op->currframe++;
/* reached last frame? */
if (op->currframe >= op->nframes)
@@ -495,16 +508,13 @@ static enum hrtimer_restart bcm_tx_timeout_handler(struct hrtimer *hrtimer)
*/
static void bcm_rx_changed(struct bcm_op *op, struct canfd_frame *data)
{
struct bcm_msg_head head;
- /* update statistics */
- op->frames_filtered++;
-
- /* prevent statistics overflow */
- if (op->frames_filtered > ULONG_MAX/100)
- op->frames_filtered = op->frames_abs = 0;
+ /* update statistics (frames_filtered <= frames_abs) */
+ if (atomic_long_read(&op->frames_abs))
+ atomic_long_inc(&op->frames_filtered);
/* this element is not throttled anymore */
data->flags &= ~RX_THR;
memset(&head, 0, sizeof(head));
@@ -743,11 +753,11 @@ 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++;
+ bcm_update_stats(op);
/* snapshot the flag under lock: op->flags/op->frames may be updated
* concurrently by bcm_rx_setup(). Do not call bcm_can_tx() while
* holding bcm_rx_update_lock: CAN loopback on echo-capable
* interfaces (e.g. vcan) can deliver the transmitted skb back into
--
2.53.0
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v3 4/7] can: bcm: fix CAN frame rx/tx statistics
2026-07-07 10:16 ` Oliver Hartkopp
(?)
@ 2026-07-07 10:37 ` sashiko-bot
2026-07-07 12:23 ` Oliver Hartkopp
-1 siblings, 1 reply; 28+ messages in thread
From: sashiko-bot @ 2026-07-07 10:37 UTC (permalink / raw)
To: Oliver Hartkopp
Cc: Vincent Mailhol, linux-can, Oliver Hartkopp, Oleksij Rempel,
Marc Kleine-Budde
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [Medium] A Time-Of-Check to Time-Of-Use (TOCTOU) race during statistics reset can permanently break the invariant `frames_filtered <= frames_abs`.
Pre-existing issues:
- [High] Missing RCU publish/subscribe barriers for lockless list operations, risking uninitialized memory reads in procfs.
--
--- Patch [4]: [PATCH v3 4/7] can: bcm: fix CAN frame rx/tx statistics ---
commit faeade145d2ba64627aca2f6af5f2b0b8ad0d933
Author: Oliver Hartkopp <socketcan@hartkopp.net>
can: bcm: fix CAN frame rx/tx statistics
KCSAN detected a data race within the bcm_rx_handler() when two CAN frames
have been simultaneously received and processed in a single rx op by two
different CPUs. Use atomic operations with (signed) long data types to
access the statistics in the hot path.
> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index fd548c6eaa913..0378e31ea10ed 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
[ ... ]
> @@ -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 does this function safely traverse
the lockless tx_ops list?
bcm_proc_show() traverses bo->tx_ops via list_for_each_entry() instead of
list_for_each_entry_rcu(), which lacks the necessary subscribe barriers.
Additionally, neither bcm_tx_setup() nor bcm_rx_setup() use list_add_rcu()
when inserting into these lists:
bcm_tx_setup()
list_add(&op->list, &bo->tx_ops);
bcm_rx_setup()
list_add(&op->list, &bo->rx_ops);
Could reading /proc/net/can/bcm/<inode> while another thread concurrently
sets up a BCM message on the same socket result in traversing uninitialized
memory due to missing RCU publish/subscribe barriers?
> seq_putc(m, '\n');
>
> @@ -290,6 +294,15 @@ static int bcm_proc_show(struct seq_file *m, void *v)
> }
> #endif /* CONFIG_PROC_FS */
>
> +static void bcm_update_stats(struct bcm_op *op)
> +{
> + /* prevent statistics overflow */
> + if (atomic_long_inc_return(&op->frames_abs) > LONG_MAX / 100) {
> + atomic_long_set(&op->frames_filtered, 0);
> + atomic_long_set(&op->frames_abs, 0);
> + }
> +}
[Severity: Medium]
Could this non-atomic reset allow other threads to interleave their
increments and permanently break the statistic invariant?
If Thread A hits the LONG_MAX / 100 threshold and begins resetting the stats:
Thread A
bcm_update_stats()
atomic_long_set(&op->frames_filtered, 0);
atomic_long_set(&op->frames_abs, 0);
And Thread B is concurrently processing bcm_rx_changed():
Thread B
bcm_rx_changed()
if (atomic_long_read(&op->frames_abs))
atomic_long_inc(&op->frames_filtered);
If Thread B reads a non-zero frames_abs, then Thread A resets both to 0,
and finally Thread B increments frames_filtered, wouldn't frames_filtered
become 1 while frames_abs is 0?
This would leave frames_filtered greater than frames_abs, causing the procfs
output to display negative reduction percentages.
> +
> /*
> * bcm_can_tx - send the (next) CAN frame to the appropriate CAN interface
> * of the given bcm tx op
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260707-bcm_fixes-v3-0-84600ca6d889@hartkopp.net?part=4
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v3 4/7] can: bcm: fix CAN frame rx/tx statistics
2026-07-07 10:37 ` sashiko-bot
@ 2026-07-07 12:23 ` Oliver Hartkopp
0 siblings, 0 replies; 28+ messages in thread
From: Oliver Hartkopp @ 2026-07-07 12:23 UTC (permalink / raw)
To: sashiko-reviews
Cc: Vincent Mailhol, linux-can, Oleksij Rempel, Marc Kleine-Budde
On 07.07.26 12:37, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> New issues:
> - [Medium] A Time-Of-Check to Time-Of-Use (TOCTOU) race during statistics reset can permanently break the invariant `frames_filtered <= frames_abs`.
Fixed in V4
>
> Pre-existing issues:
> - [High] Missing RCU publish/subscribe barriers for lockless list operations, risking uninitialized memory reads in procfs.
Fixed in patch 5:
https://lore.kernel.org/linux-can/20260707-bcm_fixes-v3-5-84600ca6d889@hartkopp.net/
> --
>
> --- Patch [4]: [PATCH v3 4/7] can: bcm: fix CAN frame rx/tx statistics ---
> commit faeade145d2ba64627aca2f6af5f2b0b8ad0d933
> Author: Oliver Hartkopp <socketcan@hartkopp.net>
>
> can: bcm: fix CAN frame rx/tx statistics
>
> KCSAN detected a data race within the bcm_rx_handler() when two CAN frames
> have been simultaneously received and processed in a single rx op by two
> different CPUs. Use atomic operations with (signed) long data types to
> access the statistics in the hot path.
>
>> diff --git a/net/can/bcm.c b/net/can/bcm.c
>> index fd548c6eaa913..0378e31ea10ed 100644
>> --- a/net/can/bcm.c
>> +++ b/net/can/bcm.c
> [ ... ]
>> @@ -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 does this function safely traverse
> the lockless tx_ops list?
>
> bcm_proc_show() traverses bo->tx_ops via list_for_each_entry() instead of
> list_for_each_entry_rcu(), which lacks the necessary subscribe barriers.
>
> Additionally, neither bcm_tx_setup() nor bcm_rx_setup() use list_add_rcu()
> when inserting into these lists:
>
> bcm_tx_setup()
> list_add(&op->list, &bo->tx_ops);
>
> bcm_rx_setup()
> list_add(&op->list, &bo->rx_ops);
>
> Could reading /proc/net/can/bcm/<inode> while another thread concurrently
> sets up a BCM message on the same socket result in traversing uninitialized
> memory due to missing RCU publish/subscribe barriers?
>
>> seq_putc(m, '\n');
>>
>> @@ -290,6 +294,15 @@ static int bcm_proc_show(struct seq_file *m, void *v)
>> }
>> #endif /* CONFIG_PROC_FS */
>>
>> +static void bcm_update_stats(struct bcm_op *op)
>> +{
>> + /* prevent statistics overflow */
>> + if (atomic_long_inc_return(&op->frames_abs) > LONG_MAX / 100) {
>> + atomic_long_set(&op->frames_filtered, 0);
>> + atomic_long_set(&op->frames_abs, 0);
>> + }
>> +}
>
> [Severity: Medium]
> Could this non-atomic reset allow other threads to interleave their
> increments and permanently break the statistic invariant?
>
> If Thread A hits the LONG_MAX / 100 threshold and begins resetting the stats:
>
> Thread A
> bcm_update_stats()
> atomic_long_set(&op->frames_filtered, 0);
> atomic_long_set(&op->frames_abs, 0);
>
> And Thread B is concurrently processing bcm_rx_changed():
>
> Thread B
> bcm_rx_changed()
> if (atomic_long_read(&op->frames_abs))
> atomic_long_inc(&op->frames_filtered);
>
> If Thread B reads a non-zero frames_abs, then Thread A resets both to 0,
> and finally Thread B increments frames_filtered, wouldn't frames_filtered
> become 1 while frames_abs is 0?
>
> This would leave frames_filtered greater than frames_abs, causing the procfs
> output to display negative reduction percentages.
>
>> +
>> /*
>> * bcm_can_tx - send the (next) CAN frame to the appropriate CAN interface
>> * of the given bcm tx op
> [ ... ]
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v3 5/7] can: bcm: add missing rcu list annotations and operations
2026-07-07 10:16 ` Oliver Hartkopp
@ 2026-07-07 10:16 ` Oliver Hartkopp
-1 siblings, 0 replies; 28+ messages in thread
From: Oliver Hartkopp via B4 Relay @ 2026-07-07 10:16 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 0378e31ea10e..f21d48d99aab 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)
@@ -993,10 +993,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)
@@ -1134,12 +1135,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 */
@@ -1157,10 +1157,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);
@@ -1341,13 +1345,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))) */
@@ -1415,14 +1416,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;
}
@@ -1752,12 +1755,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.
@@ -1784,12 +1789,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] 28+ messages in thread* [PATCH v3 5/7] can: bcm: add missing rcu list annotations and operations
@ 2026-07-07 10:16 ` Oliver Hartkopp
0 siblings, 0 replies; 28+ messages in thread
From: Oliver Hartkopp @ 2026-07-07 10:16 UTC (permalink / raw)
To: linux-can; +Cc: Oliver Hartkopp, Marc Kleine-Budde, sashiko-reviews
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 0378e31ea10e..f21d48d99aab 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)
@@ -993,10 +993,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)
@@ -1134,12 +1135,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 */
@@ -1157,10 +1157,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);
@@ -1341,13 +1345,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))) */
@@ -1415,14 +1416,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;
}
@@ -1752,12 +1755,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.
@@ -1784,12 +1789,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] 28+ messages in thread* Re: [PATCH v3 5/7] can: bcm: add missing rcu list annotations and operations
2026-07-07 10:16 ` Oliver Hartkopp
(?)
@ 2026-07-07 10:42 ` sashiko-bot
2026-07-07 12:26 ` Oliver Hartkopp
-1 siblings, 1 reply; 28+ messages in thread
From: sashiko-bot @ 2026-07-07 10:42 UTC (permalink / raw)
To: Oliver Hartkopp
Cc: linux-can, Marc Kleine-Budde, Oliver Hartkopp, Oleksij Rempel,
Vincent Mailhol
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Lockless reads of 64-bit ktime_t values and TOCTOU races on op->count in bcm_tx_timeout_handler
--
--- Patch [5]: [PATCH v3 5/7] can: bcm: add missing rcu list annotations and operations ---
commit 8310d786e9cc8e49f889c95c47438e852dd137cc
Author: Oliver Hartkopp <socketcan@hartkopp.net>
can: bcm: add missing rcu list annotations and operations
This commit moves list_add_rcu() calls to the end of the setup code to cover
initial settings and fixes the missing list_del_rcu() before bcm_remove_op()
in bcm_release().
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 0378e31ea10ed..f21d48d99aab6 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
[ ... ]
> @@ -1159,6 +1159,10 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
> 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);
> +
[Severity: High]
This is a pre-existing issue, but I have a question about concurrent access
to the timer and count values in bcm_tx_timeout_handler().
When a userspace application continuously updates a BCM TX op via TX_SETUP,
bcm_tx_setup() updates the op's timer and count values.
Concurrently, if the timer fires, bcm_tx_timeout_handler() reads them
locklessly in softirq context:
net/can/bcm.c:bcm_tx_timeout_handler() {
...
if (op->kt_ival1 && (op->count > 0)) {
bcm_can_tx(op, NULL);
if (!op->count && (op->flags & TX_COUNTEVT)) {
...
}
Can this lead to torn reads on 64-bit ktime_t variables (kt_ival1, kt_ival2)
on 32-bit platforms, resulting in a bogus timeout value that breaks cyclic
CAN frame transmission?
Additionally, could a TOCTOU race on op->count cause the handler to
incorrectly miss sending a TX_EXPIRED notification to userspace?
It appears a similar issue for RX_SETUP was previously addressed by adding
locking in commit "can: bcm: add locking when updating filter and timer
values", but the TX_SETUP side might still be exposed.
> if (op->flags & TX_ANNOUNCE)
> bcm_can_tx(op, NULL);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260707-bcm_fixes-v3-0-84600ca6d889@hartkopp.net?part=5
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v3 5/7] can: bcm: add missing rcu list annotations and operations
2026-07-07 10:42 ` sashiko-bot
@ 2026-07-07 12:26 ` Oliver Hartkopp
0 siblings, 0 replies; 28+ messages in thread
From: Oliver Hartkopp @ 2026-07-07 12:26 UTC (permalink / raw)
To: sashiko-reviews
Cc: linux-can, Marc Kleine-Budde, Oleksij Rempel, Vincent Mailhol
On 07.07.26 12:42, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] Lockless reads of 64-bit ktime_t values and TOCTOU races on op->count in bcm_tx_timeout_handler
> --
Fixed in patch 6:
https://lore.kernel.org/linux-can/20260707-bcm_fixes-v3-6-84600ca6d889@hartkopp.net/
>
> --- Patch [5]: [PATCH v3 5/7] can: bcm: add missing rcu list annotations and operations ---
> commit 8310d786e9cc8e49f889c95c47438e852dd137cc
> Author: Oliver Hartkopp <socketcan@hartkopp.net>
>
> can: bcm: add missing rcu list annotations and operations
>
> This commit moves list_add_rcu() calls to the end of the setup code to cover
> initial settings and fixes the missing list_del_rcu() before bcm_remove_op()
> in bcm_release().
>
> 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 0378e31ea10ed..f21d48d99aab6 100644
>> --- a/net/can/bcm.c
>> +++ b/net/can/bcm.c
> [ ... ]
>> @@ -1159,6 +1159,10 @@ static int bcm_tx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>> 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);
>> +
>
> [Severity: High]
> This is a pre-existing issue, but I have a question about concurrent access
> to the timer and count values in bcm_tx_timeout_handler().
>
> When a userspace application continuously updates a BCM TX op via TX_SETUP,
> bcm_tx_setup() updates the op's timer and count values.
>
> Concurrently, if the timer fires, bcm_tx_timeout_handler() reads them
> locklessly in softirq context:
>
> net/can/bcm.c:bcm_tx_timeout_handler() {
> ...
> if (op->kt_ival1 && (op->count > 0)) {
> bcm_can_tx(op, NULL);
> if (!op->count && (op->flags & TX_COUNTEVT)) {
> ...
> }
>
> Can this lead to torn reads on 64-bit ktime_t variables (kt_ival1, kt_ival2)
> on 32-bit platforms, resulting in a bogus timeout value that breaks cyclic
> CAN frame transmission?
>
> Additionally, could a TOCTOU race on op->count cause the handler to
> incorrectly miss sending a TX_EXPIRED notification to userspace?
>
> It appears a similar issue for RX_SETUP was previously addressed by adding
> locking in commit "can: bcm: add locking when updating filter and timer
> values", but the TX_SETUP side might still be exposed.
>
>> if (op->flags & TX_ANNOUNCE)
>> bcm_can_tx(op, NULL);
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v3 6/7] can: bcm: extend bcm_tx_lock usage for data and timer updates
2026-07-07 10:16 ` Oliver Hartkopp
@ 2026-07-07 10:16 ` Oliver Hartkopp
-1 siblings, 0 replies; 28+ messages in thread
From: Oliver Hartkopp via B4 Relay @ 2026-07-07 10:16 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.
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 | 79 ++++++++++++++++++++++++++++++++++++++++++-----------------
1 file changed, 56 insertions(+), 23 deletions(-)
diff --git a/net/can/bcm.c b/net/can/bcm.c
index f21d48d99aab..4605d8d202ae 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -453,16 +453,22 @@ static void bcm_send_to_user(struct bcm_op *op, struct bcm_msg_head *head,
static bool bcm_tx_set_expiry(struct bcm_op *op, struct hrtimer *hrt)
{
ktime_t ival;
+ spin_lock_bh(&op->bcm_tx_lock);
+
if (op->kt_ival1 && op->count)
ival = op->kt_ival1;
- else if (op->kt_ival2)
+ else if (op->kt_ival2) {
ival = op->kt_ival2;
- else
+ } else {
+ spin_unlock_bh(&op->bcm_tx_lock);
return false;
+ }
+
+ spin_unlock_bh(&op->bcm_tx_lock);
hrtimer_set_expires(hrt, ktime_add(ktime_get(), ival));
return true;
}
@@ -475,12 +481,21 @@ static void bcm_tx_start_timer(struct bcm_op *op)
/* bcm_tx_timeout_handler - performs cyclic CAN frame transmissions */
static enum hrtimer_restart bcm_tx_timeout_handler(struct hrtimer *hrtimer)
{
struct bcm_op *op = container_of(hrtimer, struct bcm_op, timer);
struct bcm_msg_head msg_head;
+ bool tx_ival1, tx_ival2;
- if (op->kt_ival1 && (op->count > 0)) {
+ /* snapshot kt_ival1/kt_ival2/count under lock to avoid torn
+ * ktime_t reads racing with concurrent bcm_tx_setup() updates
+ */
+ spin_lock_bh(&op->bcm_tx_lock);
+ tx_ival1 = op->kt_ival1 && (op->count > 0);
+ tx_ival2 = !!op->kt_ival2;
+ spin_unlock_bh(&op->bcm_tx_lock);
+
+ if (tx_ival1) {
bcm_can_tx(op, NULL);
if (!op->count && (op->flags & TX_COUNTEVT)) {
/* create notification to user */
memset(&msg_head, 0, sizeof(msg_head));
@@ -493,11 +508,11 @@ static enum hrtimer_restart bcm_tx_timeout_handler(struct hrtimer *hrtimer)
msg_head.nframes = 0;
bcm_send_to_user(op, &msg_head, NULL, 0);
}
- } else if (op->kt_ival2) {
+ } else if (tx_ival2) {
bcm_can_tx(op, NULL);
}
return bcm_tx_set_expiry(op, &op->timer) ?
HRTIMER_RESTART : HRTIMER_NORESTART;
@@ -1012,64 +1027,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);
@@ -1141,14 +1172,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] 28+ messages in thread* [PATCH v3 6/7] can: bcm: extend bcm_tx_lock usage for data and timer updates
@ 2026-07-07 10:16 ` Oliver Hartkopp
0 siblings, 0 replies; 28+ messages in thread
From: Oliver Hartkopp @ 2026-07-07 10:16 UTC (permalink / raw)
To: linux-can; +Cc: Oliver Hartkopp, Marc Kleine-Budde
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.
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 | 79 ++++++++++++++++++++++++++++++++++++++++++-----------------
1 file changed, 56 insertions(+), 23 deletions(-)
diff --git a/net/can/bcm.c b/net/can/bcm.c
index f21d48d99aab..4605d8d202ae 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -453,16 +453,22 @@ static void bcm_send_to_user(struct bcm_op *op, struct bcm_msg_head *head,
static bool bcm_tx_set_expiry(struct bcm_op *op, struct hrtimer *hrt)
{
ktime_t ival;
+ spin_lock_bh(&op->bcm_tx_lock);
+
if (op->kt_ival1 && op->count)
ival = op->kt_ival1;
- else if (op->kt_ival2)
+ else if (op->kt_ival2) {
ival = op->kt_ival2;
- else
+ } else {
+ spin_unlock_bh(&op->bcm_tx_lock);
return false;
+ }
+
+ spin_unlock_bh(&op->bcm_tx_lock);
hrtimer_set_expires(hrt, ktime_add(ktime_get(), ival));
return true;
}
@@ -475,12 +481,21 @@ static void bcm_tx_start_timer(struct bcm_op *op)
/* bcm_tx_timeout_handler - performs cyclic CAN frame transmissions */
static enum hrtimer_restart bcm_tx_timeout_handler(struct hrtimer *hrtimer)
{
struct bcm_op *op = container_of(hrtimer, struct bcm_op, timer);
struct bcm_msg_head msg_head;
+ bool tx_ival1, tx_ival2;
- if (op->kt_ival1 && (op->count > 0)) {
+ /* snapshot kt_ival1/kt_ival2/count under lock to avoid torn
+ * ktime_t reads racing with concurrent bcm_tx_setup() updates
+ */
+ spin_lock_bh(&op->bcm_tx_lock);
+ tx_ival1 = op->kt_ival1 && (op->count > 0);
+ tx_ival2 = !!op->kt_ival2;
+ spin_unlock_bh(&op->bcm_tx_lock);
+
+ if (tx_ival1) {
bcm_can_tx(op, NULL);
if (!op->count && (op->flags & TX_COUNTEVT)) {
/* create notification to user */
memset(&msg_head, 0, sizeof(msg_head));
@@ -493,11 +508,11 @@ static enum hrtimer_restart bcm_tx_timeout_handler(struct hrtimer *hrtimer)
msg_head.nframes = 0;
bcm_send_to_user(op, &msg_head, NULL, 0);
}
- } else if (op->kt_ival2) {
+ } else if (tx_ival2) {
bcm_can_tx(op, NULL);
}
return bcm_tx_set_expiry(op, &op->timer) ?
HRTIMER_RESTART : HRTIMER_NORESTART;
@@ -1012,64 +1027,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);
@@ -1141,14 +1172,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] 28+ messages in thread* Re: [PATCH v3 6/7] can: bcm: extend bcm_tx_lock usage for data and timer updates
2026-07-07 10:16 ` Oliver Hartkopp
(?)
@ 2026-07-07 12:33 ` Marc Kleine-Budde
2026-07-07 12:38 ` Oliver Hartkopp
-1 siblings, 1 reply; 28+ messages in thread
From: Marc Kleine-Budde @ 2026-07-07 12:33 UTC (permalink / raw)
To: Oliver Hartkopp via B4 Relay; +Cc: linux-can, Oliver Hartkopp
[-- Attachment #1: Type: text/plain, Size: 1226 bytes --]
On 07.07.2026 12:16:13, Oliver Hartkopp via B4 Relay wrote:
> 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.
>
> 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.
Can you update the comment in struct bcm_op::bcm_tx_lock accordingly?
Marc
> Fixes: c2aba69d0c36 ("can: bcm: add locking for bcm_op runtime updates")
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
--
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] 28+ messages in thread* Re: [PATCH v3 6/7] can: bcm: extend bcm_tx_lock usage for data and timer updates
2026-07-07 12:33 ` Marc Kleine-Budde
@ 2026-07-07 12:38 ` Oliver Hartkopp
0 siblings, 0 replies; 28+ messages in thread
From: Oliver Hartkopp @ 2026-07-07 12:38 UTC (permalink / raw)
To: Marc Kleine-Budde, Oliver Hartkopp via B4 Relay; +Cc: linux-can
On 07.07.26 14:33, Marc Kleine-Budde wrote:
> On 07.07.2026 12:16:13, Oliver Hartkopp via B4 Relay wrote:
>> 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.
>>
>> 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.
>
> Can you update the comment in struct bcm_op::bcm_tx_lock accordingly?
Yes, I will go through all the commit messages and add the changes.
Best regards,
Oliver
>
> Marc
>
>> Fixes: c2aba69d0c36 ("can: bcm: add locking for bcm_op runtime updates")
>> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v3 7/7] can: bcm: validate frame length in bcm_rx_setup() for RTR replies
2026-07-07 10:16 ` Oliver Hartkopp
@ 2026-07-07 10:16 ` Oliver Hartkopp
-1 siblings, 0 replies; 28+ messages in thread
From: Oliver Hartkopp via B4 Relay @ 2026-07-07 10:16 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 4605d8d202ae..3fbd08deca50 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -1209,26 +1209,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)
*/
@@ -1287,11 +1302,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;
@@ -1352,20 +1371,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);
@@ -1458,10 +1473,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] 28+ messages in thread* [PATCH v3 7/7] can: bcm: validate frame length in bcm_rx_setup() for RTR replies
@ 2026-07-07 10:16 ` Oliver Hartkopp
0 siblings, 0 replies; 28+ messages in thread
From: Oliver Hartkopp @ 2026-07-07 10:16 UTC (permalink / raw)
To: linux-can; +Cc: Oliver Hartkopp, Marc Kleine-Budde
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 4605d8d202ae..3fbd08deca50 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -1209,26 +1209,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)
*/
@@ -1287,11 +1302,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;
@@ -1352,20 +1371,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);
@@ -1458,10 +1473,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] 28+ messages in thread