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.
quoted hunk ↗ jump to hunk
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?