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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] In net/can/af_can.c, can_get_unshared_skb_ext() uses the wrong
  test to decide whether the CAN skb extension is shared.
- [Medium] A UID set before transmission survives the new extension
  unsharing, so duplicated or mirrored copies of such frames still share…

--

can: fix unique skb identifier regression under RPS

This patch moves the CAN per-skb unique identifier used for raw_rcv()
duplicate detection from skb->hash into a new can_skb_uid field in
struct can_skb_ext. The UID is now assigned in can_rcv(), canfd_rcv()
and canxl_rcv() after the CAN skb extension is made private for cloned
skbs, and can-gw and vxcan reset it for forwarded frames.

Closes: 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

[ ... ]

> @@ -684,11 +679,22 @@ static void can_receive(struct sk_buff *skb, struct net_device *dev)
>  	}
>  }
>  
> +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)))

[Severity: High]
Is skb_cloned() the right test for whether the extension is shared?

skb_cloned() only tells you whether the packet data is shared through
shinfo->dataref. The skb_ext has its own refcount.
__copy_skb_header() shares the extension through __skb_ext_copy() for
copies as well as clones:

include/linux/skbuff.h:__skb_ext_copy() {
    ...
        refcount_inc(&ext->refcnt);
        dst->extensions = ext;
    ...
}

So results of skb_copy(), pskb_copy() and skb_unshare() have private
data but still share the CAN extension with their source.

One way to reach this is netem with duplicate and corrupt on a CAN
device:

net/sched/sch_netem.c:netem_enqueue() {
    ...
        skb2 = skb_clone(skb, GFP_ATOMIC);
    ...
        skb = skb_unshare(skb, GFP_ATOMIC);
    ...
}

After this, skb2 has dataref == 1 and the copy has private data, so
skb_cloned() is false for both. They still point to the same skb_ext
with refcnt == 2.

Echo skbs from can_create_echo_skb() on IFF_ECHO devices (e.g. vcan
echo=1) can end up in the same state once the parent is consumed.

In that case can_get_unshared_skb_ext() returns the shared csx, and
can_set_skb_uid() writes can_skb_uid in place:

    while (!(csx->can_skb_uid))
        csx->can_skb_uid = atomic_inc_return(&skbcounter);

The second frame then sees a non-zero UID and keeps it. Doesn't this
contradict the commit message statement that "independently received
clones never end up with the same UID"?

With a shared UID, raw_rcv() can again drop a legitimate frame when the
slab reuses the skb address on the same CPU:

    if (this_cpu_ptr(ro->uniq)->skb == oskb &&
        this_cpu_ptr(ro->uniq)->can_skb_uid == csx->can_skb_uid) {

If the two frames go through can_rcv() on different CPUs, the
check-then-store in can_set_skb_uid() is also an unsynchronized write
into memory that both skbs share. The UID could then change while
raw_rcv() on the other CPU is between filter matches. Could that break
the join_filters counting?

skb_ext_add() already does the refcount-based check in
skb_ext_maybe_cow():

    if (refcount_read(&old->refcnt) == 1)
        return old;

Would it be enough to call skb_ext_add() unconditionally here, or to
check the extension refcount, instead of gating on skb_cloned()?

> +		return skb_ext_add(skb, SKB_EXT_CAN);

[Severity: Medium]
Does the private copy really get its own UID if the frame was already
tagged before transmission?

isotp_sendmsg() and isotp_send_cframe() call can_set_skb_uid(csx) on
the tx skb before can_send(). So the UID is already non-zero (say X)
by the time netem duplicate (skb_clone) or tc mirred mirror/redirect
copies the skb. Neither path resets the UID, unlike can-gw and vxcan
in this patch.

When such a clone reaches can_rcv() still cloned, skb_ext_add() goes
through skb_ext_maybe_cow(). That function copies the whole extension,
X included:

    memcpy(new, old, old->chunks * SKB_EXT_ALIGN_VALUE);

can_set_skb_uid() then leaves the non-zero X in place, so the private
copy keeps the same identity as the original.

Once can_receive() consumes the first copy, a later copy (the echo
clone from can_create_echo_skb(), or the mirrored frame) can be
allocated at the freed slab address. On the same CPU, a raw socket
that did not send the frame, such as candump, would then match both
uniq->skb and uniq->can_skb_uid in raw_rcv() and drop the frame as a
duplicate.

The commit message only explains why the UID is kept across the local
loopback path. Should duplicated or mirrored copies of pre-tagged
frames also get a fresh UID, so that the "never end up with the same
UID" claim holds?

> +	else
> +		return csx;
> +}

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260929154125.3298-1-socketcan%40hartkopp.net