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@xxxxxxxxxxxxxxxxxxxx>

Reviewed-by: Oliver Hartkopp <socketcan@xxxxxxxxxxxx>
Acked-by: Oliver Hartkopp <socketcan@xxxxxxxxxxxx>

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;