[PATCH v3 0/3] can: bcm: add/fix locking for config updates at runtime

[PATCH v3 0/3] can: bcm: add/fix locking for config updates at runtime

[Oliver Hartkopp via B4 Relay]

Address three locking issues detected by KASAN and KCSAN.

The first patch fixes an unintialized spin_lock in the KASAN fix.
The second patch addresses the update of content in bcm_rx_setup().
The third patch adds a missing atomic conversion in can_rcv_filter().

Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
Changes in v3:
- Add a third patch that fixes a KCSAN issue that showed up while testing
  patch 2 8-)
- Link to v2: https://patch.msgid.link/20260219-bcm_spin_lock_init-v2-0-694352bfca62@hartkopp.net

Changes in v2:
- Add a second patch addressing the KCSAN report
- Link to v1: https://patch.msgid.link/20260218-bcm_spin_lock_init-v1-1-592634c8a5b5@hartkopp.net

---
Oliver Hartkopp (3):
      can: bcm: fix locking for bcm_op runtime updates
      can: bcm: add locking when updating filter and timer values
      can: statistics: add missing atomic access in hot path

 net/can/af_can.c |  4 ++--
 net/can/af_can.h |  2 +-
 net/can/bcm.c    | 40 +++++++++++++++++++++++++++++++++-------
 3 files changed, 36 insertions(+), 10 deletions(-)
---
base-commit: 2961f841b025fb234860bac26dfb7fa7cb0fb122
change-id: 20260218-bcm_spin_lock_init-2518eb8d4501

Best regards,
--  
Oliver Hartkopp <socketcan@hartkopp.net>

[PATCH v3 1/3] can: bcm: fix locking for bcm_op runtime updates

[Oliver Hartkopp via B4 Relay]

From: Oliver Hartkopp <socketcan@hartkopp.net>

Commit c2aba69d0c36 ("can: bcm: add locking for bcm_op runtime updates")
added a locking for some variables that can be modified at runtime when
updating the sending bcm_op with a new TX_SETUP command in bcm_tx_setup().

Usually the RX_SETUP only handles and filters incoming traffic with one
exception: When the RX_RTR_FRAME flag is set a predefined CAN frame is
sent when a specific RTR frame is received. Therefore the rx bcm_op uses
bcm_can_tx() which uses the bcm_tx_lock that was only initialized in
bcm_tx_setup(). Add the missing spin_lock_init() when allocating the
bcm_op in bcm_rx_setup() to handle the RTR case properly.

Fixes: c2aba69d0c36 ("can: bcm: add locking for bcm_op runtime updates")
Reported-by: syzbot+5b11eccc403dd1cea9f8@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/linux-can/699466e4.a70a0220.2c38d7.00ff.GAE@google.com/
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 net/can/bcm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/can/bcm.c b/net/can/bcm.c
index b7324e9c955b..fd9fa072881e 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -1174,10 +1174,11 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
 		/* 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);
 		op->can_id = msg_head->can_id;
 		op->nframes = msg_head->nframes;
 		op->cfsiz = CFSIZ(msg_head->flags);
 		op->flags = msg_head->flags;
 

-- 
2.51.0

[PATCH v3 2/3] can: bcm: add locking when updating filter and timer values

[Oliver Hartkopp via B4 Relay]

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.
This caused no functional issues in the past as the new values might
show up at any time without losing its intended functionality.

Btw. the KCSAN report can be resolved by protecting the 'lockless'
data updates with a spin_lock_bh().

Fixes: ffd980f976e7 ("[CAN]: Add broadcast manager (bcm) protocol")
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 | 39 ++++++++++++++++++++++++++++++++-------
 1 file changed, 32 insertions(+), 7 deletions(-)

diff --git a/net/can/bcm.c b/net/can/bcm.c
index fd9fa072881e..0a3dc5500e14 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -123,10 +123,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 update of filter data */
 };
 
 struct bcm_sock {
 	struct sock sk;
 	int bound;
@@ -1141,10 +1142,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
@@ -1152,33 +1155,53 @@ 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);
+			}
 		}
 
+		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;
 
@@ -1261,24 +1284,26 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
 
 	} 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);
 			bcm_rx_thr_flush(op);
 		}
 
 		if ((op->flags & STARTTIMER) && op->kt_ival1)

-- 
2.51.0

Re: [PATCH v3 2/3] can: bcm: add locking when updating filter and timer values

[Marc Kleine-Budde]

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

The nipa/ai-review bot found these problems:

| https://netdev-ai.bots.linux.dev/ai-review.html?id=873e6dfb-7d8e-4f4d-98ef-a89fcedb701a

commit 6ea1c1c7064ff2b5c2a0434ec61d62fd3463fc12
Author: Oliver Hartkopp <socketcan@hartkopp.net>

can: bcm: add locking when updating filter and timer values

This commit addresses a KCSAN-detected data race in the CAN BCM protocol
by adding locking to protect simultaneous access to timer and filter values
during updates in bcm_rx_setup(). The race was benign functionally but is
resolved by introducing spin_lock_bh() protection.

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 fd9fa072881e..0a3dc5500e14 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -125,6 +125,7 @@ struct bcm_op {
>  	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 update of filter data */
                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

The comment says this lock protects "update of filter data" but the lock
is also used to protect timer data updates (ival1, ival2, kt_ival1,
kt_ival2, kt_lastmsg) in the SETTIMER block later in bcm_rx_setup().

Should the comment be more accurate and say "protect update of filter and
timer data"?

Additionally, this lock differs from bcm_tx_lock in that it's only taken on
the writer side (bcm_rx_setup) while the reader-side softirq handlers
(bcm_rx_handler, bcm_rx_timeout_handler, bcm_rx_thr_handler) never acquire
this lock. The protection is asymmetric -- it provides KCSAN suppression
and local-CPU softirq exclusion through spin_lock_bh(), but doesn't
synchronize against readers on other CPUs.

Is this asymmetric locking strategy intentional, or should the comment
clarify the actual protection model?

>  };
>
>  struct bcm_sock {

[ ... ]

> @@ -1263,10 +1286,13 @@ static int bcm_rx_setup(struct bcm_msg_head *msg_head, struct msghdr *msg,
>  		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);

This is where the lock also protects timer data, which confirms the comment
at the struct field declaration should mention timer data protection as well.

[ ... ]

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 2/3] can: bcm: add locking when updating filter and timer values

[Marc Kleine-Budde]

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

On 02.03.2026 11:21:38, Marc Kleine-Budde wrote:
> The nipa/ai-review bot found these problems:
>
> | https://netdev-ai.bots.linux.dev/ai-review.html?id=873e6dfb-7d8e-4f4d-98ef-a89fcedb701a

I think this patch is the only one of this series, which is still open,
right?

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 2/3] can: bcm: add locking when updating filter and timer values

[Oliver Hartkopp]

On 23.03.26 10:42, Marc Kleine-Budde wrote:
> On 02.03.2026 11:21:38, Marc Kleine-Budde wrote:
>> The nipa/ai-review bot found these problems:
>>
>> | https://netdev-ai.bots.linux.dev/ai-review.html?id=873e6dfb-7d8e-4f4d-98ef-a89fcedb701a
> 
> I think this patch is the only one of this series, which is still open,
> right?
> 

Yes, it is still open.
I'll look at it soon, but it should not delay the other fixes that are 
currently agreed.

Best regards,
Oliver

[PATCH v3 3/3] can: statistics: add missing atomic access in hot path

[Oliver Hartkopp via B4 Relay]

From: Oliver Hartkopp <socketcan@hartkopp.net>

Commit 80b5f90158d1 ("can: statistics: use atomic access in hot path")
fixed a KCSAN issue in can_receive() but missed to convert the 'matches'
variable used in can_rcv_filter().

Fixes: 80b5f90158d1 ("can: statistics: use atomic access in hot path")
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 net/can/af_can.c | 4 ++--
 net/can/af_can.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/can/af_can.c b/net/can/af_can.c
index 22c65a014861..31a5eab3fab3 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -467,11 +467,11 @@ int can_rx_register(struct net *net, struct net_device *dev, canid_t can_id,
 	dev_rcv_lists = can_dev_rcv_lists_find(net, dev);
 	rcv_list = can_rcv_list_find(&can_id, &mask, dev_rcv_lists);
 
 	rcv->can_id = can_id;
 	rcv->mask = mask;
-	rcv->matches = 0;
+	atomic_long_set(&rcv->matches, 0);
 	rcv->func = func;
 	rcv->data = data;
 	rcv->ident = ident;
 	rcv->sk = sk;
 
@@ -571,11 +571,11 @@ void can_rx_unregister(struct net *net, struct net_device *dev, canid_t can_id,
 EXPORT_SYMBOL(can_rx_unregister);
 
 static inline void deliver(struct sk_buff *skb, struct receiver *rcv)
 {
 	rcv->func(skb, rcv->data);
-	rcv->matches++;
+	atomic_long_inc(&rcv->matches);
 }
 
 static int can_rcv_filter(struct can_dev_rcv_lists *dev_rcv_lists, struct sk_buff *skb)
 {
 	struct receiver *rcv;
diff --git a/net/can/af_can.h b/net/can/af_can.h
index 22f3352c77fe..87887014f562 100644
--- a/net/can/af_can.h
+++ b/net/can/af_can.h
@@ -50,11 +50,11 @@
 
 struct receiver {
 	struct hlist_node list;
 	canid_t can_id;
 	canid_t mask;
-	unsigned long matches;
+	atomic_long_t matches;
 	void (*func)(struct sk_buff *skb, void *data);
 	void *data;
 	char *ident;
 	struct sock *sk;
 	struct rcu_head rcu;

-- 
2.51.0

Re: [PATCH v3 3/3] can: statistics: add missing atomic access in hot path

[Marc Kleine-Budde]

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

The nipa/ai-review bot found these problems:

| https://netdev-ai.bots.linux.dev/ai-review.html?id=873e6dfb-7d8e-4f4d-98ef-a89fcedb701a

commit 083c819fbd59ad774cbf0662b6530ae8cf926328
Author: Oliver Hartkopp <socketcan@hartkopp.net>

can: statistics: add missing atomic access in hot path

This patch converts the matches field in struct receiver from unsigned
long to atomic_long_t to fix a KCSAN race that was missed by commit
80b5f90158d1.

> diff --git a/net/can/af_can.h b/net/can/af_can.h
> --- a/net/can/af_can.h
> +++ b/net/can/af_can.h
> @@ -52,7 +52,7 @@ struct receiver {
>  	struct hlist_node list;
>  	canid_t can_id;
>  	canid_t mask;
> -	unsigned long matches;
> +	atomic_long_t matches;
>  	void (*func)(struct sk_buff *skb, void *data);
>  	void *data;
>  	char *ident;

With this type change, is can_print_rcvlist() in net/can/proc.c also
missing a conversion?  It still accesses r->matches directly:

net/can/proc.c:can_print_rcvlist() {
    ...
    seq_printf(m, fmt, DNAME(dev), r->can_id, r->mask,
               r->func, r->data, r->matches, r->ident);
    ...
}

Should this be atomic_long_read(&r->matches) to match the other
conversion sites?

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