Re: [PATCH net v3 2/3] net: hsr: return private clones from the tagged-frame helpers

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: hsr: return private clones from the tagged-frame helpers

This adds hsr_clone_private() (skb_clone() followed by skb_cow()) and uses
it in hsr_create_tagged_frame() and prp_create_tagged_frame(), including
both NETIF_F_HW_HSR_TAG_INS branches, so that per-egress path/LAN id writes
land in private linear data. Tag and trailer pointers are reacquired after
the copy.

> diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
> index 67aaf5a8622b..efcbf3cf26f9 100644
> --- a/net/hsr/hsr_forward.c
> +++ b/net/hsr/hsr_forward.c
> @@ -325,8 +325,29 @@ static struct sk_buff *hsr_fill_tag(struct sk_buff *skb,
>  	return skb;
>  }
>  
> -/* If the original frame was an HSR tagged frame, just clone it to be sent
> - * unchanged. Otherwise, create a private frame especially tagged for 'port'.
> +/* Clone an skb and make the clone's data private, so that per-egress
> + * writes cannot corrupt the original skb or other clones of it.
> + * Returns NULL on allocation failure.
> + */
> +static struct sk_buff *hsr_clone_private(struct sk_buff *skb)
> +{
> +	struct sk_buff *clone;
> +
> +	clone = skb_clone(skb, GFP_ATOMIC);
> +	if (!clone)
> +		return NULL;
> +	if (skb_cow(clone, 0)) {
> +		kfree_skb(clone);
> +		return NULL;
> +	}
> +
> +	return clone;
> +}
> +
> +/* If the original frame was an HSR tagged frame, return a private clone
> + * of it with the path id updated for 'port'. Otherwise, return a private
> + * clone for hardware tag insertion, or create a private frame especially
> + * tagged for 'port'.
>   */
>  struct sk_buff *hsr_create_tagged_frame(struct hsr_frame_info *frame,
>  					struct hsr_port *port)
> @@ -336,14 +357,18 @@ struct sk_buff *hsr_create_tagged_frame(struct hsr_frame_info *frame,
>  	int movelen;
>  
>  	if (frame->skb_hsr) {
> -		struct hsr_ethhdr *hsr_ethhdr =
> -			(struct hsr_ethhdr *)skb_mac_header(frame->skb_hsr);
> +		struct hsr_ethhdr *hsr_ethhdr;
> +
> +		skb = hsr_clone_private(frame->skb_hsr);
> +		if (!skb)
> +			return NULL;
>  
>  		/* set the lane id properly */
> +		hsr_ethhdr = (struct hsr_ethhdr *)skb_mac_header(skb);
>  		hsr_set_path_id(frame, hsr_ethhdr, port);
> -		return skb_clone(frame->skb_hsr, GFP_ATOMIC);
> +		return skb;
>  	} else if (port->dev->features & NETIF_F_HW_HSR_TAG_INS) {
> -		return skb_clone(frame->skb_std, GFP_ATOMIC);
> +		return hsr_clone_private(frame->skb_std);
>  	}
>  
>  	/* Create the new skb with enough headroom to fit the HSR tag */

[Severity: High]
This isn't a bug introduced by this patch, but should the sibling
get_untagged_frame() callbacks be privatized the same way?

hsr_get_untagged_frame() and prp_get_untagged_frame() still end with:

	return skb_clone(frame->skb_std, GFP_ATOMIC);

and their consumer writes into that still-shared linear data:

net/hsr/hsr_forward.c:hsr_deliver_master() {
	was_multicast_frame = (skb->pkt_type == PACKET_MULTICAST);
	hsr_addr_subst_source(node_src, skb);
	skb_pull(skb, ETH_HLEN);
	...
}

net/hsr/hsr_framereg.c:hsr_addr_subst_source() {
	...
	memcpy(&eth_hdr(skb)->h_source, node->macaddress_A, ETH_ALEN);
}

Since that clone shares data with frame->skb_std, can the substituted
source address leak into the ring egress built later in the same
hsr_forward_do() loop?  For an untagged frame frame->skb_hsr is NULL, so
the slave iterations take either

	skb = __pskb_copy(frame->skb_std,
			  skb_headroom(frame->skb_std) + HSR_HLEN, GFP_ATOMIC);

or, with NETIF_F_HW_HSR_TAG_INS, the new hsr_clone_private(frame->skb_std)
above, both of which copy the already-mutated bytes.

Port order looks like it puts master first: hsr_dev_finalize() calls
hsr_add_port() for MASTER, then SLAVE_A/SLAVE_B (and INTERLINK), and
hsr_add_port() uses list_add_tail_rcu().

The substitution is only a no-op when node->macaddress_A already equals
h_source.  hsr_get_node() also matches on macaddress_B:

net/hsr/hsr_framereg.c:hsr_get_node() {
	...
		if (ether_addr_equal(node->macaddress_B, ethhdr->h_source)) {
	...
}

and hsr_handle_sup_frame() populates macaddress_B from a supervision
frame's source, so macaddress_A and h_source can differ.

The comment this series added to hsr_xmit() states the rule:

	/* An interlink-bound skb from get_untagged_frame() can still alias
	 * another live consumer: ...
	 * Privatize before any address mutation.
	 */

but the skb_cow() there is applied only for port->type == HSR_PT_INTERLINK,
which leaves the master delivery path out.  Would it be simpler to make
the get_untagged_frame() callbacks return private clones too, so the
contract holds for every consumer?

One more consumer of the same bytes: an AF_PACKET listener on the slave
device gets its clone in packet_rcv() before hsr_handle_frame() runs, so
does it also observe the rewritten h_source?