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

[sashiko-bot@kernel.org]

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