[PATCH] can: gw: avoid losing statistics updates on concurrent receive

[PATCH] can: gw: avoid losing statistics updates on concurrent receive

[Zhu Jun]

can_can_gw_rcv() can run on multiple CPUs for the same gateway job. RCU
protects the job's lifetime, but does not serialize its frame counters.
Concurrent plain increments can lose updates, and netlink dumps race
with the counter updates as well.

Use atomic counters for handled, dropped and deleted frames. Read each
counter once into a u32 for the netlink dump, preserving the existing
32-bit attributes and their wraparound behavior.

Fixes: c1aabdf379bc ("can-gw: add netlink based CAN routing")
Signed-off-by: Zhu Jun <zhujun2@cmss.chinamobile.com>
---
 net/can/gw.c | 43 ++++++++++++++++++++++++-------------------
 1 file changed, 24 insertions(+), 19 deletions(-)

diff --git a/net/can/gw.c b/net/can/gw.c
index 8a5327d8a00e..a5233b4606da 100644
--- a/net/can/gw.c
+++ b/net/can/gw.c
@@ -40,6 +40,7 @@
  */
 
 #include <linux/module.h>
+#include <linux/atomic.h>
 #include <linux/init.h>
 #include <linux/types.h>
 #include <linux/kernel.h>
@@ -129,9 +130,9 @@ struct can_can_gw {
 struct cgw_job {
 	struct hlist_node list;
 	struct rcu_head rcu;
-	u32 handled_frames;
-	u32 dropped_frames;
-	u32 deleted_frames;
+	atomic_t handled_frames;
+	atomic_t dropped_frames;
+	atomic_t deleted_frames;
 	struct cf_mod __rcu *cf_mod;
 	union {
 		/* CAN frame data source */
@@ -484,12 +485,12 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data)
 	 */
 	if (csx->can_gw_hops >= max_hops) {
 		/* indicate deleted frames due to misconfiguration */
-		gwj->deleted_frames++;
+		atomic_inc(&gwj->deleted_frames);
 		return;
 	}
 
 	if (!(gwj->dst.dev->flags & IFF_UP)) {
-		gwj->dropped_frames++;
+		atomic_inc(&gwj->dropped_frames);
 		return;
 	}
 
@@ -510,7 +511,7 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data)
 		nskb = skb_clone(skb, GFP_ATOMIC);
 
 	if (!nskb) {
-		gwj->dropped_frames++;
+		atomic_inc(&gwj->dropped_frames);
 		return;
 	}
 
@@ -521,7 +522,7 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data)
 	ncsx = skb_ext_add(nskb, SKB_EXT_CAN);
 	if (!ncsx) {
 		kfree_skb(nskb);
-		gwj->dropped_frames++;
+		atomic_inc(&gwj->dropped_frames);
 		return;
 	}
 
@@ -552,7 +553,7 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data)
 		/* dlc may have changed, make sure it fits to the CAN frame */
 		if (cf->len > max_len) {
 			/* delete frame due to misconfiguration */
-			gwj->deleted_frames++;
+			atomic_inc(&gwj->deleted_frames);
 			kfree_skb(nskb);
 			return;
 		}
@@ -571,9 +572,9 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data)
 
 	/* send to netdevice */
 	if (can_send(nskb, gwj->flags & CGW_FLAGS_CAN_ECHO))
-		gwj->dropped_frames++;
+		atomic_inc(&gwj->dropped_frames);
 	else
-		gwj->handled_frames++;
+		atomic_inc(&gwj->handled_frames);
 }
 
 static inline int cgw_register_filter(struct net *net, struct cgw_job *gwj)
@@ -640,6 +641,7 @@ static int cgw_put_job(struct sk_buff *skb, struct cgw_job *gwj, int type,
 	struct rtcanmsg *rtcan;
 	struct nlmsghdr *nlh;
 	struct cf_mod *mod;
+	u32 handled, dropped, deleted;
 
 	nlh = nlmsg_put(skb, pid, seq, type, sizeof(*rtcan), flags);
 	if (!nlh)
@@ -652,18 +654,21 @@ static int cgw_put_job(struct sk_buff *skb, struct cgw_job *gwj, int type,
 
 	/* add statistics if available */
 
-	if (gwj->handled_frames) {
-		if (nla_put_u32(skb, CGW_HANDLED, gwj->handled_frames) < 0)
+	handled = atomic_read(&gwj->handled_frames);
+	if (handled) {
+		if (nla_put_u32(skb, CGW_HANDLED, handled) < 0)
 			goto cancel;
 	}
 
-	if (gwj->dropped_frames) {
-		if (nla_put_u32(skb, CGW_DROPPED, gwj->dropped_frames) < 0)
+	dropped = atomic_read(&gwj->dropped_frames);
+	if (dropped) {
+		if (nla_put_u32(skb, CGW_DROPPED, dropped) < 0)
 			goto cancel;
 	}
 
-	if (gwj->deleted_frames) {
-		if (nla_put_u32(skb, CGW_DELETED, gwj->deleted_frames) < 0)
+	deleted = atomic_read(&gwj->deleted_frames);
+	if (deleted) {
+		if (nla_put_u32(skb, CGW_DELETED, deleted) < 0)
 			goto cancel;
 	}
 
@@ -1146,9 +1151,9 @@ static int cgw_create_job(struct sk_buff *skb,  struct nlmsghdr *nlh,
 		goto out_free_cf;
 	}
 
-	gwj->handled_frames = 0;
-	gwj->dropped_frames = 0;
-	gwj->deleted_frames = 0;
+	atomic_set(&gwj->handled_frames, 0);
+	atomic_set(&gwj->dropped_frames, 0);
+	atomic_set(&gwj->deleted_frames, 0);
 	gwj->flags = r->flags;
 	gwj->gwtype = r->gwtype;
 	gwj->limit_hops = limhops;
-- 
2.51.0

Re: [PATCH] can: gw: avoid losing statistics updates on concurrent receive

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [High] can: data race modifying shared SKB in can_send() [1]
--

commit 10078742ee4a6687a5a77083fa056c99b8f26a52
Author: Zhu Jun <zhujun2@cmss.chinamobile.com>

can: gw: avoid losing statistics updates on concurrent receive

This commit converts the frame statistics counters in struct cgw_job to
atomic_t so concurrent receive paths and netlink dumps do not race on
counter updates.

> diff --git a/net/can/gw.c b/net/can/gw.c
> index 8a5327d8a00e8..a5233b4606dad 100644
> --- a/net/can/gw.c
> +++ b/net/can/gw.c

[ ... ]

> @@ -510,7 +511,7 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data)
>  		nskb = skb_clone(skb, GFP_ATOMIC);
>  
>  	if (!nskb) {
> -		gwj->dropped_frames++;
> +		atomic_inc(&gwj->dropped_frames);
>  		return;
>  	}

[ ... ]

> @@ -571,9 +572,9 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data)
>  
>  	/* send to netdevice */
>  	if (can_send(nskb, gwj->flags & CGW_FLAGS_CAN_ECHO))
> -		gwj->dropped_frames++;
> +		atomic_inc(&gwj->dropped_frames);
>  	else
> -		gwj->handled_frames++;
> +		atomic_inc(&gwj->handled_frames);
>  }

[Severity: High]
This problem wasn't introduced by this patch, but can routing an unmodified
CAN FD frame cause a data race [1] on the shared skb payload in
can_send()?

When no modification functions are configured (!mod->modfunc[0]),
can_can_gw_rcv() duplicates the incoming skb with skb_clone(), which shares
skb->data with the original skb, and passes nskb to can_send():

net/can/af_can.c:can_send() {
	...
	} else if (can_is_canfd_skb(skb)) {
		struct canfd_frame *cfd = (struct canfd_frame *)skb->data;

		skb->protocol = htons(ETH_P_CANFD);

		/* set CAN FD flag for CAN FD frames by default */
		cfd->flags |= CANFD_FDF;
	...
}

can_send() modifies cfd->flags in place on the shared buffer while other
receivers (such as raw sockets or packet sniffers) may be reading the
original skb concurrently, causing a data race and corrupting the flags for
concurrent readers.

[1] https://sashiko.dev/#/bug/linux-89b38ada-4acb-4bf7-afe0-3d8d244ba058

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111062907.16857-1-zhujun2@cmss.chinamobile.com?part=1

Re: [PATCH] can: gw: avoid losing statistics updates on concurrent receive

[Oliver Hartkopp]

On 11.11.26 07:33, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found no new issues
> in this patch, but noticed 1 related pre-existing issue it does not
> introduce:
> - [High] can: data race modifying shared SKB in can_send() [1]
> --

No problem here.

Sashiko groundhog day ...

https://lore.kernel.org/linux-can/ac22b59e-129b-4883-b9c6-da5e69e4078d@hartkopp.net/


> 
> commit 10078742ee4a6687a5a77083fa056c99b8f26a52
> Author: Zhu Jun <zhujun2@cmss.chinamobile.com>
> 
> can: gw: avoid losing statistics updates on concurrent receive
> 
> This commit converts the frame statistics counters in struct cgw_job to
> atomic_t so concurrent receive paths and netlink dumps do not race on
> counter updates.
> 
>> diff --git a/net/can/gw.c b/net/can/gw.c
>> index 8a5327d8a00e8..a5233b4606dad 100644
>> --- a/net/can/gw.c
>> +++ b/net/can/gw.c
> 
> [ ... ]
> 
>> @@ -510,7 +511,7 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data)
>>   		nskb = skb_clone(skb, GFP_ATOMIC);
>>   
>>   	if (!nskb) {
>> -		gwj->dropped_frames++;
>> +		atomic_inc(&gwj->dropped_frames);
>>   		return;
>>   	}
> 
> [ ... ]
> 
>> @@ -571,9 +572,9 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data)
>>   
>>   	/* send to netdevice */
>>   	if (can_send(nskb, gwj->flags & CGW_FLAGS_CAN_ECHO))
>> -		gwj->dropped_frames++;
>> +		atomic_inc(&gwj->dropped_frames);
>>   	else
>> -		gwj->handled_frames++;
>> +		atomic_inc(&gwj->handled_frames);
>>   }
> 
> [Severity: High]
> This problem wasn't introduced by this patch, but can routing an unmodified
> CAN FD frame cause a data race [1] on the shared skb payload in
> can_send()?
> 
> When no modification functions are configured (!mod->modfunc[0]),
> can_can_gw_rcv() duplicates the incoming skb with skb_clone(), which shares
> skb->data with the original skb, and passes nskb to can_send():
> 
> net/can/af_can.c:can_send() {
> 	...
> 	} else if (can_is_canfd_skb(skb)) {
> 		struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
> 
> 		skb->protocol = htons(ETH_P_CANFD);
> 
> 		/* set CAN FD flag for CAN FD frames by default */
> 		cfd->flags |= CANFD_FDF;
> 	...
> }
> 
> can_send() modifies cfd->flags in place on the shared buffer while other
> receivers (such as raw sockets or packet sniffers) may be reading the
> original skb concurrently, causing a data race and corrupting the flags for
> concurrent readers.
> 
> [1] https://sashiko.dev/#/bug/linux-89b38ada-4acb-4bf7-afe0-3d8d244ba058
> 

Re: [PATCH] can: gw: avoid losing statistics updates on concurrent receive

[Oliver Hartkopp]

On 11.11.26 07:29, Zhu Jun wrote:
> can_can_gw_rcv() can run on multiple CPUs for the same gateway job. RCU
> protects the job's lifetime, but does not serialize its frame counters.
> Concurrent plain increments can lose updates, and netlink dumps race
> with the counter updates as well.
> 
> Use atomic counters for handled, dropped and deleted frames. Read each
> counter once into a u32 for the netlink dump, preserving the existing
> 32-bit attributes and their wraparound behavior.
> 
> Fixes: c1aabdf379bc ("can-gw: add netlink based CAN routing")

No fixes tag here. This is not critical and not stable-relevant content 
that can go via can-next.

@Marc: Can you remove the tag while applying the patch or does it need a 
re-spin?

> Signed-off-by: Zhu Jun <zhujun2@cmss.chinamobile.com>

Reviewed-by: Oliver Hartkopp <socketcan@hartkopp.net>
Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>

Many thanks,
Oliver

> ---
>   net/can/gw.c | 43 ++++++++++++++++++++++++-------------------
>   1 file changed, 24 insertions(+), 19 deletions(-)
> 
> diff --git a/net/can/gw.c b/net/can/gw.c
> index 8a5327d8a00e..a5233b4606da 100644
> --- a/net/can/gw.c
> +++ b/net/can/gw.c
> @@ -40,6 +40,7 @@
>    */
>   
>   #include <linux/module.h>
> +#include <linux/atomic.h>
>   #include <linux/init.h>
>   #include <linux/types.h>
>   #include <linux/kernel.h>
> @@ -129,9 +130,9 @@ struct can_can_gw {
>   struct cgw_job {
>   	struct hlist_node list;
>   	struct rcu_head rcu;
> -	u32 handled_frames;
> -	u32 dropped_frames;
> -	u32 deleted_frames;
> +	atomic_t handled_frames;
> +	atomic_t dropped_frames;
> +	atomic_t deleted_frames;
>   	struct cf_mod __rcu *cf_mod;
>   	union {
>   		/* CAN frame data source */
> @@ -484,12 +485,12 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data)
>   	 */
>   	if (csx->can_gw_hops >= max_hops) {
>   		/* indicate deleted frames due to misconfiguration */
> -		gwj->deleted_frames++;
> +		atomic_inc(&gwj->deleted_frames);
>   		return;
>   	}
>   
>   	if (!(gwj->dst.dev->flags & IFF_UP)) {
> -		gwj->dropped_frames++;
> +		atomic_inc(&gwj->dropped_frames);
>   		return;
>   	}
>   
> @@ -510,7 +511,7 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data)
>   		nskb = skb_clone(skb, GFP_ATOMIC);
>   
>   	if (!nskb) {
> -		gwj->dropped_frames++;
> +		atomic_inc(&gwj->dropped_frames);
>   		return;
>   	}
>   
> @@ -521,7 +522,7 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data)
>   	ncsx = skb_ext_add(nskb, SKB_EXT_CAN);
>   	if (!ncsx) {
>   		kfree_skb(nskb);
> -		gwj->dropped_frames++;
> +		atomic_inc(&gwj->dropped_frames);
>   		return;
>   	}
>   
> @@ -552,7 +553,7 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data)
>   		/* dlc may have changed, make sure it fits to the CAN frame */
>   		if (cf->len > max_len) {
>   			/* delete frame due to misconfiguration */
> -			gwj->deleted_frames++;
> +			atomic_inc(&gwj->deleted_frames);
>   			kfree_skb(nskb);
>   			return;
>   		}
> @@ -571,9 +572,9 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data)
>   
>   	/* send to netdevice */
>   	if (can_send(nskb, gwj->flags & CGW_FLAGS_CAN_ECHO))
> -		gwj->dropped_frames++;
> +		atomic_inc(&gwj->dropped_frames);
>   	else
> -		gwj->handled_frames++;
> +		atomic_inc(&gwj->handled_frames);
>   }
>   
>   static inline int cgw_register_filter(struct net *net, struct cgw_job *gwj)
> @@ -640,6 +641,7 @@ static int cgw_put_job(struct sk_buff *skb, struct cgw_job *gwj, int type,
>   	struct rtcanmsg *rtcan;
>   	struct nlmsghdr *nlh;
>   	struct cf_mod *mod;
> +	u32 handled, dropped, deleted;
>   
>   	nlh = nlmsg_put(skb, pid, seq, type, sizeof(*rtcan), flags);
>   	if (!nlh)
> @@ -652,18 +654,21 @@ static int cgw_put_job(struct sk_buff *skb, struct cgw_job *gwj, int type,
>   
>   	/* add statistics if available */
>   
> -	if (gwj->handled_frames) {
> -		if (nla_put_u32(skb, CGW_HANDLED, gwj->handled_frames) < 0)
> +	handled = atomic_read(&gwj->handled_frames);
> +	if (handled) {
> +		if (nla_put_u32(skb, CGW_HANDLED, handled) < 0)
>   			goto cancel;
>   	}
>   
> -	if (gwj->dropped_frames) {
> -		if (nla_put_u32(skb, CGW_DROPPED, gwj->dropped_frames) < 0)
> +	dropped = atomic_read(&gwj->dropped_frames);
> +	if (dropped) {
> +		if (nla_put_u32(skb, CGW_DROPPED, dropped) < 0)
>   			goto cancel;
>   	}
>   
> -	if (gwj->deleted_frames) {
> -		if (nla_put_u32(skb, CGW_DELETED, gwj->deleted_frames) < 0)
> +	deleted = atomic_read(&gwj->deleted_frames);
> +	if (deleted) {
> +		if (nla_put_u32(skb, CGW_DELETED, deleted) < 0)
>   			goto cancel;
>   	}
>   
> @@ -1146,9 +1151,9 @@ static int cgw_create_job(struct sk_buff *skb,  struct nlmsghdr *nlh,
>   		goto out_free_cf;
>   	}
>   
> -	gwj->handled_frames = 0;
> -	gwj->dropped_frames = 0;
> -	gwj->deleted_frames = 0;
> +	atomic_set(&gwj->handled_frames, 0);
> +	atomic_set(&gwj->dropped_frames, 0);
> +	atomic_set(&gwj->deleted_frames, 0);
>   	gwj->flags = r->flags;
>   	gwj->gwtype = r->gwtype;
>   	gwj->limit_hops = limhops;