Re: [PATCH v2 3/3] can: fix unique skb identifier regression under RPS

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] can: silent packet drops due to shared state mutation in skb extension
--

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

can: fix unique skb identifier regression under RPS

This commit addresses an RPS regression where statically generated hashes
by the flow dissector cause legitimate frames to be discarded as duplicates
in raw_rcv(). It attempts to fix this by explicitly tracking the CAN skb UID
via a new CAN_EXT_UID flag in the skb extension (struct can_skb_ext), ensuring
that skb->hash is overwritten only if the flag is not set.

Fixes: d4fb6514ff8e ("can: use skb hash instead of private variable in headroom")
Reported-by: Joerg Willmann <joe@clnt.de>
Closes: https://lore.kernel.org/linux-can/2859AD3D-C805-41A0-9036-C5E8EE152419@clnt.de/
Cc: stable@vger.kernel.org

> diff --git a/net/can/af_can.c b/net/can/af_can.c
> index ef435f22ac933..26ce523693c5e 100644
> --- a/net/can/af_can.c
> +++ b/net/can/af_can.c
> @@ -641,13 +641,18 @@ static int can_rcv_filter(struct can_dev_rcv_lists *dev_rcv_lists, struct sk_buf
>  	return matches;
>  }
>  
> -void can_set_skb_uid(struct sk_buff *skb)
> +void can_set_skb_uid(struct sk_buff *skb, struct can_skb_ext *csx)
>  {
> -	/* create non-zero unique skb identifier together with *skb */
> +	if (csx->can_ext_flags & CAN_EXT_UID)
> +		return;

[Severity: High]
Could reading this shared flag to conditionally initialize a per-SKB local
field (skb->hash) cause legitimate frames to be discarded?

If a CAN frame is cloned on the egress path (e.g., via tc mirred mirroring to
another CAN interface), the original and the clone share the same skb_ext
structure. Both devices call can_put_echo_skb() to create loopback echoes,
which inherit the shared extension.

When these echoes arrive concurrently at can_rcv(), the first CPU sets
CAN_EXT_UID in the shared extension.

The second CPU sees the flag already set and returns without assigning a unique
skb->hash. Because the second clone retains the static RPS software hash, if
the SLAB allocator reuses the memory address of a previously received frame,
raw_rcv()'s per-CPU uniqframe check will falsely identify the new frame as a
duplicate and drop it.

This appears to revive the RPS duplicate-drop regression this patch intended to
fix.

> +
> +	/* Overwrite pre-calculated network hashes with a unique CAN UID */
> +	skb->hash = 0;
>  	while (!(skb->hash))
>  		skb->hash = atomic_inc_return(&skbcounter);
>  
>  	skb->sw_hash = 1;
> +	csx->can_ext_flags |= CAN_EXT_UID;

[Severity: High]
Can this mutate the shared SKB extension in the RX softirq path without
locking or unsharing, causing a data race and altering state for other clones?

Additionally, can_put_echo_skb() mutates csx->can_framelen on the TX path:

drivers/net/can/dev/skb.c:can_put_echo_skb() {
    ...
    csx->can_framelen = frame_len;
    ...
}

Because can_framelen and can_ext_flags share the same 4-byte memory region in
struct can_skb_ext, does this race with can_set_skb_uid() during concurrent
processing of mirrored clones?

>  }
>  EXPORT_SYMBOL(can_set_skb_uid);

[ ... ]

> @@ -687,8 +690,9 @@ static void can_receive(struct sk_buff *skb, struct net_device *dev)
>  static int can_rcv(struct sk_buff *skb, struct net_device *dev,
>  		   struct packet_type *pt, struct net_device *orig_dev)
>  {
> -	if (unlikely(!can_get_ml_priv(dev) ||
> -		     !can_skb_ext_find(skb) || !can_is_can_skb(skb))) {
> +	struct can_skb_ext *csx = can_skb_ext_find(skb);

[Severity: High]
Might retrieving the SKB extension here without ensuring exclusive ownership
(e.g., via skb_ext_maybe_cow()) pass a potentially shared structure to a
mutator?

> +
> +	if (unlikely(!can_get_ml_priv(dev) || !csx || !can_is_can_skb(skb))) {
>  		pr_warn_once("PF_CAN: dropped non conform CAN skbuff: dev type %d, len %d\n",
>  			     dev->type, skb->len);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260928173105.51765-1-socketcan@hartkopp.net?part=3