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(ð_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?
