Re: [PATCH net v2 2/3] net: hsr: clone before updating path and LAN IDs in tagged frames

[Paolo Abeni <pabeni@redhat.com>]

On 8/2/26 10:25 PM, Xin Xie wrote:
> hsr_create_tagged_frame() and prp_create_tagged_frame() update the
> path/LAN ID in the received skb data buffer and then skb_clone() it
> for the egress port. When the same frame is forwarded to both slave
> ports, the second port ID update lands in the same shared buffer the
> first port clone still points at; if that clone is still queued
> (qdisc backlog, NETEM, BQL), it is transmitted with the second port
> ID - a silent on-wire corruption.
> 
> One always-available trigger is the master transmit path: a
> pre-tagged frame transmitted on the master (for example injected
> locally via AF_PACKET with a valid RCT) passes prp_fill_frame_info()
> with frame->skb_prp set, the master-origin gate lets it through to
> both slaves, and both slaves run prp_set_lan_id() + skb_clone() on
> the same buffer. Tagged frames arriving on an HSR RedBox interlink
> take the analogous path through hsr_create_tagged_frame(). Normal
> traffic tests do not expose the race: the window between the first
> dev_queue_xmit() and the second ID write is only a few instructions
> and opens only under egress backpressure. With a 200 ms netem delay
> on one slave, all 200 injected frames left that slave carrying the
> other slave LAN ID in testing.
> 
> Reorder both helpers: clone first, privatize the clone with
> skb_cow(), reacquire the header/trailer pointer, then update the ID.
> skb_cow() is used rather than skb_cow_head() because privacy is
> needed for the whole linear area, not only the header: the HSR tag
> sits in the head, but the PRP RCT sits at the linear tail.
> skb_get_PRP_rct() computes the trailer from skb_tail_pointer(), so
> the returned pointer is always inside the linear area that
> pskb_expand_head() copies; frames with a nonlinear tail are
> mis-parsed by the existing helper regardless and are out of scope
> here. (Both wrappers currently reach pskb_expand_head(); they differ
> in the cloned-data predicate, and correctness here needs full data
> privacy, not only header privacy.) This adds one linear-head copy
> per tagged egress; untagged master traffic keeps the existing
> __pskb_copy() path and pays nothing from this patch.
> 
> Fixes: 451d8123f897 ("net: prp: add packet handling support")
> Reviewed-by: Ali Ahmet Memis <ali@iusegentoo.com>
> Tested-by: Ali Ahmet Memis <ali@iusegentoo.com>
> Signed-off-by: Xin Xie <xiexinet@gmail.com>

Please try to condense a bit your commit message: too much text is alike
no text at all.

> @@ -377,15 +389,28 @@ struct sk_buff *prp_create_tagged_frame(struct hsr_frame_info *frame,
>  	struct sk_buff *skb;
>  
>  	if (frame->skb_prp) {
> -		struct prp_rct *trailer = skb_get_PRP_rct(frame->skb_prp);
> +		struct prp_rct *trailer;
>  
> +		/* Same sharing hazard as above: privatize the clone data
> +		 * before updating the LAN id.
> +		 */
> +		skb = skb_clone(frame->skb_prp, GFP_ATOMIC);
> +		if (!skb)
> +			return NULL;
> +		if (skb_cow(skb, 0)) {
> +			kfree_skb(skb);
> +			return NULL;
> +		}

This is the verbatim copy of the previous chunk; please create a shared
helper instead.

While at it, sashiko notes this leaves the else branch open to the same
issue:

https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260802202504.2962-1-xiexinet%40gmail.com

Note that the data race on the dev stats can be addressed later in
separate series, as there are already many occurrences in the hsr code.

Also be aware of net-next commit c82ff94592fb.

/P