Re: [PATCH v4 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: data race and extension corruption due to incorrect skb_cloned() check
--

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

can: fix unique skb identifier regression under RPS

This commit moves the per-skb unique identifier used for duplicate detection
into a CAN skb extension instead of using skb->hash. It aims to fix an issue
where RPS assigned a non-zero hash to CAN frames, preventing a unique ID from
being generated and causing legitimate frames to be dropped as duplicates.

Link: https://lore.kernel.org/linux-can/2859AD3D-C805-41A0-9036-C5E8EE152419@clnt.de/

> diff --git a/net/can/af_can.c b/net/can/af_can.c
> index ef435f22ac933..4aa233d7e8a1d 100644
> --- a/net/can/af_can.c
> +++ b/net/can/af_can.c
[ ... ]
> -void can_set_skb_uid(struct sk_buff *skb)
> +void can_set_skb_uid(struct can_skb_ext *csx)
>  {
> -	/* create non-zero unique skb identifier together with *skb */
> -	while (!(skb->hash))
> -		skb->hash = atomic_inc_return(&skbcounter);
> -
> -	skb->sw_hash = 1;
> +	while (!(csx->can_skb_uid))
> +		csx->can_skb_uid = atomic_inc_return(&skbcounter);
>  }

[Severity: High]
If the extension block passed here is shared with another SKB, will this write
to csx->can_skb_uid cause a data race and corrupt the clone's UID?

[ ... ]
> +static struct can_skb_ext *can_get_unshared_skb_ext(struct sk_buff *skb,
> +						    struct can_skb_ext *csx)
> +{
> +	/* create unshared CAN skb_extension for netem/mirred skb clones */
> +	if (unlikely(skb_cloned(skb)))
> +		return skb_ext_add(skb, SKB_EXT_CAN);
> +	else
> +		return csx;
> +}

[Severity: High]
Is skb_cloned() the correct check to determine if the extension block is shared?

If a CAN SKB is cloned (for example, via tc mirred) and its data buffer is
later unshared via skb_unshare() or pskb_expand_head(), the data buffer is
copied but the extension block remains shared because __skb_ext_copy() only
increments its refcount.

In this scenario, skb_cloned(skb) returns false because the data buffer itself
is no longer shared. Returning csx directly here would bypass the COW refcount
logic inside skb_ext_add() and pass a shared extension block to
can_set_skb_uid().

This seems like it could cause raw_rcv() to mistakenly identify legitimate
frames as duplicates and silently drop them. Should this rely directly on the
refcount handling within skb_ext_add() instead?

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