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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · 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).

- [Medium] The commit message says that frames sharing an extension with
  another clone (tc mirred, netem duplicate) get a private copy before…

--

can: fix unique skb identifier regression under RPS

This commit moves the CAN per-skb unique identifier that raw_rcv() uses
for duplicate detection out of skb->hash and into a new can_skb_uid field
in struct can_skb_ext. The receive handlers now give a shared extension a
private copy before they assign the UID, and can-gw and vxcan clear the
UID on 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..dc27ace43719d 100644
> --- a/net/can/af_can.c
> +++ b/net/can/af_can.c
> @@ -641,13 +641,10 @@ 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 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);
>  }
>  EXPORT_SYMBOL(can_set_skb_uid);

[ ... ]

> @@ -696,6 +692,14 @@ static int can_rcv(struct sk_buff *skb, struct net_device *dev,
>  		return NET_RX_DROP;
>  	}
>  
> +	/* create unshared CAN skb_extension for netem/mirred skb clones */
> +	csx = skb_ext_add(skb, SKB_EXT_CAN);
> +	if (unlikely(!csx)) {
> +		kfree_skb_reason(skb, SKB_DROP_REASON_NOMEM);
> +		return NET_RX_DROP;
> +	}
> +
> +	can_set_skb_uid(csx);

[Severity: Medium]
Do independently received clones get different UIDs if the UID was already
set before the clone was made? The same question applies to canfd_rcv()
and canxl_rcv().

The commit message says:

  Frames whose extension is still shared with another clone (e.g. via
  tc mirred or netem duplicate) are given a private extension copy
  before the UID is assigned, so that independently received clones
  never end up with the same UID.

skb_ext_add() copies the shared extension through skb_ext_maybe_cow(),
which is a memcpy(). Any non-zero can_skb_uid that is already set ends up
in the private copy. can_set_skb_uid() then keeps that value, because it
only assigns a UID when the value is 0.

isotp sets a non-zero UID before transmission, in both isotp_sendmsg()
and isotp_send_cframe():

	/* set uid in tx skb to identify CF echo frames */
	can_set_skb_uid(csx);
	...
	can_send(skb, 1);

Suppose that TX skb is duplicated on egress. netem_enqueue() calls
skb2 = skb_clone(skb, GFP_ATOMIC) for duplicates, and mirred egress
mirroring also clones. Every copy then has the same UID X. Could the
following happen on an IFF_ECHO device such as vcan with echo=1?

vcan_tx(skb)
  can_create_echo_skb()
    nskb = skb_clone(skb)        /* echo e1, UID X */
    consume_skb(skb)
netif_rx(e1)
  can_rcv(e1)
    raw_rcv()                    /* per-cpu uniq = {e1, X} */
    can_receive()
      consume_skb(e1)            /* e1 slab slot freed */

netem watchdog dequeues skb2
vcan_tx(skb2)
  can_create_echo_skb()
    nskb = skb_clone(skb2)       /* e2 reuses e1's slot, UID X */
netif_rx(e2)
  can_rcv(e2)
    raw_rcv()

The check in raw_rcv() would then match:

	if (this_cpu_ptr(ro->uniq)->skb == oskb &&
	    this_cpu_ptr(ro->uniq)->can_skb_uid == csx->can_skb_uid) {
		if (!ro->join_filters)
			return;

The duplicated frame would be silently dropped for every raw socket that
already received e1. With join_filters set, join_rx_count would be
counted too high instead.

This is the same false duplicate drop that the patch fixes for skb->hash.
Here it is limited to frames with a UID set before transmission. The old
skb->hash scheme behaved the same way in this case, because skb_clone()
copies the hash. can-gw and vxcan clear the UID explicitly, but generic
clone paths such as netem and mirred do not.

Clearing the UID whenever the extension is shared would not fix this
either. For devices without IFF_ECHO, can_send() does the loopback with
newskb = skb_clone(skb) while the original skb is still alive. isotp echo
matching depends on that clone keeping the UID.

Should the commit message be toned down for this case, or is there
another way to tell these clones apart?

>  	can_receive(skb, dev);
>  	return NET_RX_SUCCESS;
>  }

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