[PATCH net v3 0/3] net: hsr: fix shared-skb mutations in the forwarding path

[PATCH net v3 0/3] net: hsr: fix shared-skb mutations in the forwarding path

[Xin Xie]

Two existing HSR/PRP forwarding paths modify data shared by multiple
skbs. One changes RedBox interlink source addresses, and the other
updates per-egress HSR path or PRP LAN IDs. A later consumer can then
observe another consumer's value.

Patch 1 makes interlink address substitution private. Patch 2 uses one
clone/COW helper for tagged HSR/PRP frames and both hardware
tag-insertion branches. Patch 3 adds regressions for both corruptions.

The series is independent of the posted GRO/GSO v5 series:
https://lore.kernel.org/all/20260807140751.1351-1-xiexinet@gmail.com/
Both orders apply to current net. On current net-next, patch 3 needs
git am -3 because hsr_prp_redbox.sh is already in the same Makefile
block; the merged list remains sorted and validates.

Validation:
- the targeted test fails both cases on the base and passes on the
  candidate
- the affected HSR/PRP regression tests pass
- base-versus-candidate W=1 allmodconfig and allyesconfig add no
  diagnostics
- no TAG_INS hardware was available; those branches have source
  review only

Changes since v2, including official Sashiko dispositions:
- use a shared clone/COW helper and cover both TAG_INS branches
- correct the helper comment and interlink eligibility wording
- fix HSR LSDU encoding, capture draining, and result aggregation
- defer the existing dev->stats races per Paolo
- leave VLAN/non-linear receive parsing and receive-side tap aliases
  unchanged

Previous postings:
v2: https://lore.kernel.org/all/20260802202504.2962-1-xiexinet@gmail.com/
v1: https://lore.kernel.org/all/20260728143604.26-1-xiexinet@gmail.com/

Xin Xie (3):
  net: hsr: privatize interlink-bound skbs before address mutation
  net: hsr: return private clones from the tagged-frame helpers
  selftests: net: hsr: add shared-mutation regression test

 net/hsr/hsr_forward.c                         |  65 ++++-
 tools/testing/selftests/net/hsr/Makefile      |   1 +
 .../selftests/net/hsr/hsr_shared_mutation.sh  | 242 ++++++++++++++++++
 3 files changed, 299 insertions(+), 9 deletions(-)
 create mode 100755 tools/testing/selftests/net/hsr/hsr_shared_mutation.sh


base-commit: 594d905195024b228c962627ae5ae7c17bd582a4
-- 
2.43.0

[PATCH net v3 1/3] net: hsr: privatize interlink-bound skbs before address mutation

[Xin Xie]

An skb sent to a RedBox interlink can share data with master delivery
or the original TX skb. hsr_deliver_master() and hsr_xmit() then write
different source addresses, so one consumer can observe the other's
address.

Use skb_cow() before interlink address substitution when those paths
can share the data. Drop that egress on COW failure. Other interlink
traffic keeps its zero-copy behavior.

Fixes: 5055cccfc2d1 ("net: hsr: Provide RedBox support (HSR-SAN)")
Reviewed-by: Ali Ahmet Memis <ali@iusegentoo.com>
Tested-by: Ali Ahmet Memis <ali@iusegentoo.com>
Signed-off-by: Xin Xie <xiexinet@gmail.com>
---
 net/hsr/hsr_forward.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
index 0774981a65c1..67aaf5a8622b 100644
--- a/net/hsr/hsr_forward.c
+++ b/net/hsr/hsr_forward.c
@@ -420,6 +420,22 @@ static void hsr_deliver_master(struct sk_buff *skb, struct net_device *dev,
 static int hsr_xmit(struct sk_buff *skb, struct hsr_port *port,
 		    struct hsr_frame_info *frame)
 {
+	/* An interlink-bound skb from get_untagged_frame() can still alias
+	 * another live consumer: for master-originated frames the clone
+	 * shares the original TX skb (which taps or the TX path may still
+	 * hold); for ring frames the master also consumes them when they
+	 * are destined to the local node without being exclusive to it.
+	 * Privatize before any address mutation.
+	 */
+	if (port->type == HSR_PT_INTERLINK &&
+	    (frame->port_rcv->type == HSR_PT_MASTER ||
+	     (frame->is_local_dest && !frame->is_local_exclusive)) &&
+	    skb_cow(skb, 0)) {
+		frame->port_rcv->dev->stats.rx_dropped++;
+		kfree_skb(skb);
+		return NET_XMIT_DROP;
+	}
+
 	if (frame->port_rcv->type == HSR_PT_MASTER) {
 		hsr_addr_subst_dest(frame->node_src, skb, port);
 
-- 
2.43.0

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

[Xin Xie]

The tagged-frame helpers can return skbs whose data is still shared.
Later path, LAN ID, or source-address updates may then modify another
queued clone or the original TX skb.

Add hsr_clone_private() and use it for HSR, PRP, and both hardware
tag-insertion branches. Reacquire tag and trailer pointers after COW;
the PRP RCT is in the copied linear area.

This adds one linear-data copy per affected egress. Untagged software
forwarding is unchanged.

Fixes: 451d8123f897 ("net: prp: add packet handling support")
Fixes: dcf0cd1cc58b ("net: hsr: add offloading support")
Signed-off-by: Xin Xie <xiexinet@gmail.com>
---
 net/hsr/hsr_forward.c | 49 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 40 insertions(+), 9 deletions(-)

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 */
@@ -377,17 +402,23 @@ 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;
 
+		skb = hsr_clone_private(frame->skb_prp);
+		if (!skb)
+			return NULL;
+
+		trailer = skb_get_PRP_rct(skb);
 		if (trailer) {
 			prp_set_lan_id(trailer, port);
 		} else {
 			WARN_ONCE(!trailer, "errored PRP skb");
+			kfree_skb(skb);
 			return NULL;
 		}
-		return skb_clone(frame->skb_prp, 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);
 	}
 
 	skb = skb_copy_expand(frame->skb_std, skb_headroom(frame->skb_std),
-- 
2.43.0

[PATCH net v3 3/3] selftests: net: hsr: add shared-mutation regression test

[Xin Xie]

Add regression coverage for both shared-data corruptions. Delay one
PRP egress and verify that queued clones retain their LAN IDs. For HSR
RedBox, deliver a tagged multicast to the master and interlink and
verify each source address.

Drain captures while injecting and merge both subtest results so
packet-socket pressure or a skip cannot hide a failure.

Signed-off-by: Xin Xie <xiexinet@gmail.com>
---
 tools/testing/selftests/net/hsr/Makefile      |   1 +
 .../selftests/net/hsr/hsr_shared_mutation.sh  | 242 ++++++++++++++++++
 2 files changed, 243 insertions(+)
 create mode 100755 tools/testing/selftests/net/hsr/hsr_shared_mutation.sh

diff --git a/tools/testing/selftests/net/hsr/Makefile b/tools/testing/selftests/net/hsr/Makefile
index 31fb9326cf53..87fe34951b8b 100644
--- a/tools/testing/selftests/net/hsr/Makefile
+++ b/tools/testing/selftests/net/hsr/Makefile
@@ -5,6 +5,7 @@ top_srcdir = ../../../../..
 TEST_PROGS := \
 	hsr_ping.sh \
 	hsr_redbox.sh \
+	hsr_shared_mutation.sh \
 	link_faults.sh \
 	prp_ping.sh \
 # end of TEST_PROGS
diff --git a/tools/testing/selftests/net/hsr/hsr_shared_mutation.sh b/tools/testing/selftests/net/hsr/hsr_shared_mutation.sh
new file mode 100755
index 000000000000..0b8b8791190d
--- /dev/null
+++ b/tools/testing/selftests/net/hsr/hsr_shared_mutation.sh
@@ -0,0 +1,242 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+#
+# Verify that per-egress mutations of shared skb data are private:
+#
+# F2 (path/LAN ID): on an affected kernel the second slave's LAN-ID write
+# lands in the first slave's still-queued clone; with a netem delay on
+# slave A, injected frames leave A carrying B's LAN ID.
+#
+# F1 (RedBox source MAC): on an affected kernel an HSR-tagged multicast
+# frame received on a RedBox slave is cloned for master and interlink,
+# and the interlink's RedBox-MAC rewrite lands in the master clone's
+# buffer, so the local stack receives the RedBox MAC instead of the
+# originating node's MAC.
+
+ipv6=false
+
+source ./hsr_common.sh
+
+DUR=5
+
+require()
+{
+	command -v "$1" >/dev/null 2>&1 && return 0
+	echo "SKIP: $1 not available"
+	exit $ksft_skip
+}
+
+require ip
+require tc
+require python3
+
+trap cleanup_all_ns EXIT
+
+# ------------------------------------------------------- F2: LAN-ID isolation
+# PRP DANP (proto 1), AF_PACKET pre-tagged injection, netem on slave A.
+run_f2()
+{
+	setup_ns ns 2>/dev/null || return $ksft_skip
+	nsx() { ip netns exec "$ns" "$@"; }
+
+	# Probe sch_netem inside the disposable namespace only.
+	if ! nsx tc qdisc add dev lo root netem delay 1ms 2>/dev/null; then
+		echo "SKIP: sch_netem not available"
+		return $ksft_skip
+	fi
+	nsx tc qdisc del dev lo root 2>/dev/null
+
+	# Capability probes end here; setup or runtime failure below is FAIL.
+	nsx ip link add vA type veth peer name vAp ||
+		{ echo "FAIL: veth A"; return 1; }
+	nsx ip link add vB type veth peer name vBp ||
+		{ echo "FAIL: veth B"; return 1; }
+	for i in vA vB vAp vBp; do
+		nsx ip link set "$i" up || { echo "FAIL: $i up"; return 1; }
+	done
+	if ! nsx ip link add name prp0 type hsr slave1 vA slave2 vB \
+		supervision 45 proto 1 2>/dev/null; then
+		echo "SKIP: HSR/PRP not supported by this kernel"
+		return $ksft_skip
+	fi
+	nsx ip link set prp0 up || { echo "FAIL: prp0 up"; return 1; }
+	nsx tc qdisc add dev vA root netem delay 200ms ||
+		{ echo "FAIL: netem"; return 1; }
+
+	nsx python3 /dev/stdin "$DUR" <<'PYF2'
+import socket, struct, select, sys, time
+
+dur = int(sys.argv[1])
+def lanid(pkt):
+    if len(pkt) < 20 or pkt[-2:] != b"\x88\xfb":
+        return None
+    return (pkt[-4] >> 4) & 0xF
+
+SRC = bytes.fromhex(open("/sys/class/net/prp0/address").read().replace(":", ""))
+DST = bytes.fromhex("02aabbccdd01")
+PAY = bytes(range(46))
+rct0 = struct.pack(">H", 0) + struct.pack(">H", 52 & 0x0FFF) + b"\x88\xfb"
+frame = DST + SRC + b"\x08\x00" + PAY + rct0
+
+tx = socket.socket(socket.AF_PACKET, socket.SOCK_RAW); tx.bind(("prp0", 0))
+sA = socket.socket(socket.AF_PACKET, socket.SOCK_RAW,
+        socket.ntohs(0x0003))
+sA.bind(("vAp", 0))
+sB = socket.socket(socket.AF_PACKET, socket.SOCK_RAW,
+        socket.ntohs(0x0003))
+sB.bind(("vBp", 0))
+sA.setblocking(False); sB.setblocking(False)
+
+a, b = [], []
+
+def match(pkt):
+    return (pkt[:6] == DST and pkt[6:12] == SRC
+            and pkt[12:14] == b"\x08\x00" and pkt[14:14 + len(PAY)] == PAY)
+
+def drain(timeout):
+    wait = timeout
+    while True:
+        r, _, _ = select.select([sA, sB], [], [], wait)
+        if not r:
+            return
+        for s in r:
+            pkt = s.recv(65535)
+            if not match(pkt):
+                continue
+            lid = lanid(pkt)
+            if lid is not None:
+                (a if s is sA else b).append(lid)
+        wait = 0
+
+# Drain while sending so the burst cannot overflow the capture sockets'
+# receive buffers; the netem-delayed A-side frames arrive afterwards and
+# are collected below.
+for _ in range(200):
+    tx.send(frame)
+    drain(0)
+    time.sleep(0.001)
+
+end = time.time() + dur
+while time.time() < end:
+    drain(0.3)
+
+print("A-side count=%d lan ids=%s" % (len(a), sorted(set(a))))
+print("B-side count=%d lan ids=%s" % (len(b), sorted(set(b))))
+if len(a) < 150 or len(b) < 150:
+    print("FAIL: too few injected frames captured (A=%d B=%d, sent 200)"
+          % (len(a), len(b)))
+    sys.exit(1)
+bad_a = [x for x in a if (x & 1) != 0]
+bad_b = [x for x in b if (x & 1) != 1]
+if bad_a or bad_b:
+    print("FAIL: shared-mutation corruption - A: %d/%d wrong-lan,"
+          " B: %d/%d wrong-lan"
+          % (len(bad_a), len(a), len(bad_b), len(b)))
+    sys.exit(1)
+print("PASS: per-egress LAN IDs isolated (A all bit0=0, B all bit0=1)")
+sys.exit(0)
+PYF2
+}
+
+# --------------------------------------------- F1: RedBox source-MAC privacy
+# HSR RedBox (proto 0), tagged multicast from a slave: master must keep
+# the node MAC, interlink must carry the RedBox MAC.
+run_f1()
+{
+	setup_ns ns 2>/dev/null || return $ksft_skip
+	nsx() { ip netns exec "$ns" "$@"; }
+
+	nsx ip link add vA type veth peer name vAp ||
+		{ echo "FAIL: veth A"; return 1; }
+	nsx ip link add vB type veth peer name vBp ||
+		{ echo "FAIL: veth B"; return 1; }
+	nsx ip link add vI type veth peer name vIp ||
+		{ echo "FAIL: veth I"; return 1; }
+	for i in vA vB vI vAp vBp vIp; do
+		nsx ip link set "$i" up || { echo "FAIL: $i up"; return 1; }
+	done
+	if ! nsx ip link add name hsr0 type hsr slave1 vA slave2 vB \
+		interlink vI supervision 45 proto 0 2>/dev/null; then
+		echo "SKIP: HSR RedBox not supported by this kernel"
+		return $ksft_skip
+	fi
+	nsx ip link set hsr0 up || { echo "FAIL: hsr0 up"; return 1; }
+
+	nsx python3 /dev/stdin <<'PYF1'
+import socket, select, sys, time
+
+NODE  = bytes.fromhex("021122334455")
+MCAST = bytes.fromhex("01005e000001")
+RB    = bytes.fromhex(open("/sys/class/net/vI/address").read().replace(":", ""))
+PAY   = bytes(range(46))
+
+def frame(seq):
+    # LSDU size = payload + HSR tag (HSR_HLEN), as hsr_fill_tag() computes it
+    tag = (((1 << 12) | (len(PAY) + 6)).to_bytes(2, "big")
+           + seq.to_bytes(2, "big") + b"\x08\x00")
+    return MCAST + NODE + b"\x89\x2f" + tag + PAY
+
+tx = socket.socket(socket.AF_PACKET, socket.SOCK_RAW); tx.bind(("vAp", 0))
+sm = socket.socket(socket.AF_PACKET, socket.SOCK_RAW,
+        socket.ntohs(0x0003))
+sm.bind(("hsr0", 0))
+si = socket.socket(socket.AF_PACKET, socket.SOCK_RAW,
+        socket.ntohs(0x0003))
+si.bind(("vIp", 0))
+sm.setblocking(False); si.setblocking(False)
+
+for i in range(3):
+    tx.send(frame(i + 1)); time.sleep(0.05)
+
+m_src = i_src = None
+end = time.time() + 4
+while time.time() < end and (m_src is None or i_src is None):
+    r, _, _ = select.select([sm, si], [], [], 0.3)
+    for s in r:
+        pkt = s.recv(65535)
+        # exact flow: dst, post-strip EtherType, exact payload, min length;
+        # h_source is the asserted value and must NOT be filtered on
+        if (len(pkt) < 60 or pkt[:6] != MCAST or pkt[12:14] != b"\x08\x00"
+                or pkt[14:14 + len(PAY)] != PAY):
+            continue
+        if s is sm and m_src is None:
+            m_src = pkt[6:12]
+        elif s is si and i_src is None:
+            i_src = pkt[6:12]
+
+print("master h_source    =", m_src.hex() if m_src else None)
+print("node MAC           =", NODE.hex())
+print("interlink h_source =", i_src.hex() if i_src else None)
+print("redbox MAC         =", RB.hex())
+if i_src != RB:
+    print("FAIL: interlink did not carry the RedBox MAC")
+    sys.exit(1)
+if m_src != NODE:
+    print("FAIL: master received %s instead of the node MAC "
+          "(shared-mutation corruption)"
+          % (m_src.hex() if m_src else "nothing"))
+    sys.exit(1)
+print("PASS: master kept node MAC, interlink kept RedBox MAC")
+sys.exit(0)
+PYF1
+}
+
+rc=0
+
+run_f2
+ret=$?
+rc=$(ksft_status_merge "$rc" "$ret")
+
+run_f1
+ret=$?
+rc=$(ksft_status_merge "$rc" "$ret")
+
+if [ "$rc" -eq 0 ]; then
+	echo "hsr_shared_mutation: per-egress mutation isolation (F1+F2) [ OK ]"
+elif [ "$rc" -eq "$ksft_skip" ]; then
+	echo "hsr_shared_mutation: subtests skipped (capabilities missing)"
+else
+	echo "hsr_shared_mutation: per-egress mutation isolation [ FAIL ]" \
+		"rc=$rc" 1>&2
+fi
+exit "$rc"
-- 
2.43.0

Re: [PATCH net v3 3/3] selftests: net: hsr: add shared-mutation regression test

[Xin Xie]

Dispositions for the AI-review findings on this patch (the review is
published on the web only, not mailed to the list):

> The header calls this subtest "F2 (path/LAN ID)" and the changelog
> says "Add regression coverage for both shared-data corruptions",
> but is any path ID actually checked anywhere in the script?
>
> This link is created with proto 1, so only the PRP path is
> exercised, and the checks below only decode the PRP RCT LAN-ID
> nibble.  That covers prp_create_tagged_frame()'s frame->skb_prp
> branch.
>
> The sibling branches in hsr_create_tagged_frame() are not touched
> by either subtest: [...]
>
> Would it be worth adding an HSR-tagged (proto 0, version 1) case
> with captures on the slave peers so those branches are covered too?

The analysis is correct: only the PRP RCT branch is exercised.  Note
the F1 link is HSR_V0, which assigns the same path ID to both slaves,
so a path-ID check would indeed need a version-1 link as suggested;
the NETIF_F_HW_HSR_TAG_INS branches additionally need offload
hardware that a veth topology does not provide.

> Every other capability this script needs is probed and turned into
> a skip: ip/tc/python3 via require(), sch_netem via the tc qdisc
> probe, and HSR/PRP plus HSR RedBox via the ip link add probes.
> AF_PACKET is the exception.
>
> tools/testing/selftests/net/hsr/config lists only: [...]
>
> and CONFIG_PACKET in net/packet/Kconfig is a plain tristate with no
> default y.  On a kernel built from this fragment,
> socket(AF_PACKET, ...) raises OSError(EAFNOSUPPORT), python3 exits
> 1, run_f2() returns 1 and the merge reports a hard FAIL rather than
> a skip.
>
> Should CONFIG_PACKET be added to the hsr config fragment?

Correct.  In practice a kselftest-merge picks up CONFIG_PACKET=y
from other net selftests' fragments, so this only bites on a kernel
built from the hsr fragment alone.

> The capture loop above also exits when the four second deadline
> expires, leaving i_src as None.  Since None != RB is true, does
> that make a missed vIp capture report as "interlink did not carry
> the RedBox MAC"?
>
> The m_src check that follows is the assertion this subtest exists
> for [...] and it is skipped because the i_src check already called
> sys.exit(1).  [...]
>
> Could F1 get an equivalent explicit check for m_src is None / i_src
> is None before the value comparisons, so a timeout is
> distinguishable from a wrong MAC?

Yes, the subtest still fails in that case, but the printed reason is
misleading and the primary assertion never runs; an explicit None
check would separate the two outcomes.

> The changelog says the results are merged "so packet-socket
> pressure or a skip cannot hide a failure".  Does the merge hold for
> statuses that are not one of the kselftest constants?
>
> Both run_f2() and run_f1() end with nsx python3 /dev/stdin, so ret
> is the raw exit status of ip netns exec python3.  A signal-killed
> interpreter gives 128+N (137 for a SIGKILL/OOM, 139 for SIGSEGV),
> and an ip netns exec failure gives 255.
>
> ksft_status_merge() only ranks four values: [...]
>
> With ret=137, ${weights[137]} expands to the empty string, which
> bash evaluates as 0 inside [[ ]], so 0 -ge 0 succeeds and the
> function echoes a, i.e. 0.  rc stays 0, the branch above prints
> "[ OK ]" and exit "$rc" returns 0 for a subtest that was killed
> mid-run.  [...]
>
> Would normalising ret to $ksft_fail for anything outside
> 0/$ksft_xfail/$ksft_skip/$ksft_fail before merging address this?

Yes, normalising ret to the ksft constants before merging addresses
it.  This only affects abnormal exits; normal outcomes always return
the ksft codes, which merge correctly.

All four are test-code remarks; none changes the verdict direction on
the configurations the test runs (it fails on the unfixed kernel and
passes with the series), so no respin is planned for them.

-- 
Xin

Re: [PATCH net v3 1/3] net: hsr: privatize interlink-bound skbs before address mutation

[Jakub Kicinski]

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: privatize interlink-bound skbs before address mutation

This adds an skb_cow() call in hsr_xmit() for interlink egress when the
frame either came in on the master port or is locally destined without
being local-exclusive, dropping the egress if the copy fails. The stated
goal is to stop hsr_deliver_master() and hsr_xmit() from writing
different source addresses into shared data.

> diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
> index 0774981a65c1..67aaf5a8622b 100644
> --- a/net/hsr/hsr_forward.c
> +++ b/net/hsr/hsr_forward.c
> @@ -420,6 +420,22 @@ static void hsr_deliver_master(struct sk_buff *skb, struct net_device *dev,
>  static int hsr_xmit(struct sk_buff *skb, struct hsr_port *port,
>  		    struct hsr_frame_info *frame)
>  {
> +	/* An interlink-bound skb from get_untagged_frame() can still alias
> +	 * another live consumer: for master-originated frames the clone
> +	 * shares the original TX skb (which taps or the TX path may still
> +	 * hold); for ring frames the master also consumes them when they
> +	 * are destined to the local node without being exclusive to it.
> +	 * Privatize before any address mutation.
> +	 */
> +	if (port->type == HSR_PT_INTERLINK &&
> +	    (frame->port_rcv->type == HSR_PT_MASTER ||
> +	     (frame->is_local_dest && !frame->is_local_exclusive)) &&
> +	    skb_cow(skb, 0)) {
> +		frame->port_rcv->dev->stats.rx_dropped++;
> +		kfree_skb(skb);
> +		return NET_XMIT_DROP;
> +	}
> +

[Severity: Medium]
Is the enumeration in this comment complete?  The unconditional interlink
source-address rewrite is pre-existing and was not introduced by this
patch, but the new guard and the commit message claim ("Other interlink
traffic keeps its zero-copy behavior") assert that the uncovered cases are
safe, and there looks to be one that isn't.

For an untagged ring frame, handle_std_frame() keeps the received skb:

net/hsr/hsr_forward.c:handle_std_frame() {
	frame->skb_hsr = NULL;
	frame->skb_prp = NULL;
	frame->skb_std = skb;
	...
}

and hsr_get_untagged_frame() hands the interlink a plain clone of that
skb.  So the interlink egress skb shares the receive head with any clone
already taken by a ptype_all tap, since __netif_receive_skb_core() runs
the deliver_skb() loop over skb->dev->ptype_all before it dereferences
skb->dev->rx_handler, and packet_rcv() then queues its skb_share_check()
clone on sk_receive_queue where the bytes are copied to userspace only
later.

For a remote unicast frame addressed to a SAN behind the RedBox,
check_local_dest() leaves frame->is_local_dest false (pkt_type is
PACKET_OTHERHOST) and frame->port_rcv->type is SLAVE_A or SLAVE_B, so the
new predicate is false, no copy happens, and the rewrite lower down in
hsr_xmit() still touches the shared data:

	if (port->type == HSR_PT_INTERLINK)
		ether_addr_copy(eth_hdr(skb)->h_source,
				port->hsr->macaddress_redbox);

Would the tap consumer on the ring slave then read the RedBox MAC instead
of the real source MAC?  Reaching this needs an HSR RedBox whose ring
slave advertises NETIF_F_HW_HSR_TAG_RM, so that untagged ring ingress is
forwarded rather than passed up:

net/hsr/hsr_slave.c:hsr_handle_frame() {
	if (!(port->dev->features & NETIF_F_HW_HSR_TAG_RM) &&
	    port->type != HSR_PT_INTERLINK &&
	    hsr->proto_ops->invalid_dan_ingress_frame &&
	    hsr->proto_ops->invalid_dan_ingress_frame(protocol))
		goto finish_pass;
	...
}

At this patch alone the same case also mutates a real on-wire frame,
because hsr_create_tagged_frame() and prp_create_tagged_frame() return a
bare clone of frame->skb_std for NETIF_F_HW_HSR_TAG_INS devices:

	} else if (port->dev->features & NETIF_F_HW_HSR_TAG_INS) {
		return skb_clone(frame->skb_std, GFP_ATOMIC);
	}

That second aliasing consumer is removed later in the series by "net: hsr:
return private clones from the tagged-frame helpers", which switches those
helpers to hsr_clone_private(), so only the tap aliasing remains at the
end of the series.

Since skb_cow(skb, 0) is a no-op for a non-cloned skb (__skb_cow() only
calls pskb_expand_head() when skb_cloned() or extra headroom is needed),
could the call be keyed on the actual sharing state, or simply made
unconditional for HSR_PT_INTERLINK, instead of on the topology predicate?
-- 
pw-bot: cr

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

[Jakub Kicinski]

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?

Re: [PATCH net v3 3/3] selftests: net: hsr: add shared-mutation regression test

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
selftests: net: hsr: add shared-mutation regression test

This adds tools/testing/selftests/net/hsr/hsr_shared_mutation.sh, which
runs two subtests: a PRP subtest that delays one slave with netem and
checks the LAN-ID nibble of captured frames, and an HSR RedBox subtest
that checks the source MAC seen on the master and on the interlink.  The
two subtest results are combined with ksft_status_merge().

A few questions on the new script below.

> diff --git a/tools/testing/selftests/net/hsr/Makefile b/tools/testing/selftests/net/hsr/Makefile
> index 31fb9326cf53..87fe34951b8b 100644
> --- a/tools/testing/selftests/net/hsr/Makefile
> +++ b/tools/testing/selftests/net/hsr/Makefile
> @@ -5,6 +5,7 @@ top_srcdir = ../../../../..
>  TEST_PROGS := \
>  	hsr_ping.sh \
>  	hsr_redbox.sh \
> +	hsr_shared_mutation.sh \
>  	link_faults.sh \
>  	prp_ping.sh \
>  # end of TEST_PROGS
> diff --git a/tools/testing/selftests/net/hsr/hsr_shared_mutation.sh b/tools/testing/selftests/net/hsr/hsr_shared_mutation.sh
> new file mode 100755
> index 000000000000..0b8b8791190d
> --- /dev/null
> +++ b/tools/testing/selftests/net/hsr/hsr_shared_mutation.sh
> @@ -0,0 +1,242 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Verify that per-egress mutations of shared skb data are private:
> +#
> +# F2 (path/LAN ID): on an affected kernel the second slave's LAN-ID write
> +# lands in the first slave's still-queued clone; with a netem delay on
> +# slave A, injected frames leave A carrying B's LAN ID.

[ ... ]

> +	if ! nsx ip link add name prp0 type hsr slave1 vA slave2 vB \
> +		supervision 45 proto 1 2>/dev/null; then
> +		echo "SKIP: HSR/PRP not supported by this kernel"
> +		return $ksft_skip
> +	fi

[Severity: Low]

The header calls this subtest "F2 (path/LAN ID)" and the changelog says
"Add regression coverage for both shared-data corruptions", but is any
path ID actually checked anywhere in the script?

This link is created with proto 1, so only the PRP path is exercised,
and the checks below only decode the PRP RCT LAN-ID nibble.  That covers
prp_create_tagged_frame()'s frame->skb_prp branch.

The sibling branches in hsr_create_tagged_frame() are not touched by
either subtest:

net/hsr/hsr_forward.c:hsr_create_tagged_frame() {
	if (frame->skb_hsr) {
		skb = hsr_clone_private(frame->skb_hsr);
		...
	} else if (port->dev->features & NETIF_F_HW_HSR_TAG_INS) {
		return hsr_clone_private(frame->skb_std);
	...
}

Would it be worth adding an HSR-tagged (proto 0, version 1) case with
captures on the slave peers so those branches are covered too?

> +	nsx python3 /dev/stdin "$DUR" <<'PYF2'
> +import socket, struct, select, sys, time

[ ... ]

> +tx = socket.socket(socket.AF_PACKET, socket.SOCK_RAW); tx.bind(("prp0", 0))
> +sA = socket.socket(socket.AF_PACKET, socket.SOCK_RAW,
> +        socket.ntohs(0x0003))
> +sA.bind(("vAp", 0))

[Severity: Low]

Every other capability this script needs is probed and turned into a
skip: ip/tc/python3 via require(), sch_netem via the tc qdisc probe, and
HSR/PRP plus HSR RedBox via the ip link add probes.  AF_PACKET is the
exception.

tools/testing/selftests/net/hsr/config lists only:

CONFIG_BRIDGE=y
CONFIG_HSR=y
CONFIG_IPV6=y
CONFIG_NET_SCH_NETEM=m
CONFIG_VETH=y
CONFIG_VLAN_8021Q=m

and CONFIG_PACKET in net/packet/Kconfig is a plain tristate with no
default y.  On a kernel built from this fragment, socket(AF_PACKET, ...)
raises OSError(EAFNOSUPPORT), python3 exits 1, run_f2() returns 1 and
the merge reports a hard FAIL rather than a skip.

Should CONFIG_PACKET be added to the hsr config fragment?

[ ... ]

> +m_src = i_src = None
> +end = time.time() + 4
> +while time.time() < end and (m_src is None or i_src is None):
> +    r, _, _ = select.select([sm, si], [], [], 0.3)
> +    for s in r:
> +        pkt = s.recv(65535)
> +        # exact flow: dst, post-strip EtherType, exact payload, min length;
> +        # h_source is the asserted value and must NOT be filtered on
> +        if (len(pkt) < 60 or pkt[:6] != MCAST or pkt[12:14] != b"\x08\x00"
> +                or pkt[14:14 + len(PAY)] != PAY):
> +            continue
> +        if s is sm and m_src is None:
> +            m_src = pkt[6:12]
> +        elif s is si and i_src is None:
> +            i_src = pkt[6:12]

[ ... ]

> +if i_src != RB:
> +    print("FAIL: interlink did not carry the RedBox MAC")
> +    sys.exit(1)
> +if m_src != NODE:
> +    print("FAIL: master received %s instead of the node MAC "
> +          "(shared-mutation corruption)"
> +          % (m_src.hex() if m_src else "nothing"))
> +    sys.exit(1)

[Severity: Low]

The capture loop above also exits when the four second deadline expires,
leaving i_src as None.  Since None != RB is true, does that make a
missed vIp capture report as "interlink did not carry the RedBox MAC"?

The m_src check that follows is the assertion this subtest exists for -
it detects hsr_xmit()'s ether_addr_copy(eth_hdr(skb)->h_source,
hsr->macaddress_redbox) landing in the master clone - and it is skipped
because the i_src check already called sys.exit(1).

run_f2 has a "nothing arrived" guard:

if len(a) < 150 or len(b) < 150:
    print("FAIL: too few injected frames captured (A=%d B=%d, sent 200)"
          % (len(a), len(b)))

Could F1 get an equivalent explicit check for m_src is None / i_src is
None before the value comparisons, so a timeout is distinguishable from
a wrong MAC?

> +print("PASS: master kept node MAC, interlink kept RedBox MAC")
> +sys.exit(0)
> +PYF1
> +}
> +
> +rc=0
> +
> +run_f2
> +ret=$?
> +rc=$(ksft_status_merge "$rc" "$ret")
> +
> +run_f1
> +ret=$?
> +rc=$(ksft_status_merge "$rc" "$ret")
> +
> +if [ "$rc" -eq 0 ]; then
> +	echo "hsr_shared_mutation: per-egress mutation isolation (F1+F2) [ OK ]"

[Severity: Medium]

The changelog says the results are merged "so packet-socket pressure or
a skip cannot hide a failure".  Does the merge hold for statuses that
are not one of the kselftest constants?

Both run_f2() and run_f1() end with nsx python3 /dev/stdin, so ret is
the raw exit status of ip netns exec python3.  A signal-killed
interpreter gives 128+N (137 for a SIGKILL/OOM, 139 for SIGSEGV), and an
ip netns exec failure gives 255.

ksft_status_merge() only ranks four values:

tools/testing/selftests/net/lib.sh:
ksft_status_merge()
{
	local a=$1; shift
	local b=$1; shift

	__ksft_status_merge "$a" "$b" \
		$ksft_pass $ksft_xfail $ksft_skip $ksft_fail
}

__ksft_status_merge()
{
	...
	for i in "$@"; do
		weights[$i]=$((weight++))
	done

	if [[ ${weights[$a]} -ge ${weights[$b]} ]]; then
		echo "$a"

With ret=137, ${weights[137]} expands to the empty string, which bash
evaluates as 0 inside [[ ]], so 0 -ge 0 succeeds and the function echoes
a, i.e. 0.  rc stays 0, the branch above prints "[ OK ]" and exit "$rc"
returns 0 for a subtest that was killed mid-run.

(If instead the empty operand aborts the conditional, the command
substitution produces no output, rc becomes empty, and both
[ "$rc" -eq 0 ] and [ "$rc" -eq "$ksft_skip" ] error out with exit ""
not propagating a failure either.)

Would normalising ret to $ksft_fail for anything outside
0/$ksft_xfail/$ksft_skip/$ksft_fail before merging address this?

> +elif [ "$rc" -eq "$ksft_skip" ]; then
> +	echo "hsr_shared_mutation: subtests skipped (capabilities missing)"
> +else
> +	echo "hsr_shared_mutation: per-egress mutation isolation [ FAIL ]" \
> +		"rc=$rc" 1>&2
> +fi
> +exit "$rc"

Re: [PATCH net v3 1/3] net: hsr: privatize interlink-bound skbs before address mutation

[Jakub Kicinski]

On Sat,  8 Aug 2026 02:45:22 +0200 Xin Xie wrote:
> +	if (port->type == HSR_PT_INTERLINK &&
> +	    (frame->port_rcv->type == HSR_PT_MASTER ||
> +	     (frame->is_local_dest && !frame->is_local_exclusive)) &&
> +	    skb_cow(skb, 0)) {

Would skb_cow_head() be sufficient? It's much cheaper, at least for TCP.

Re: [PATCH net v3 3/3] selftests: net: hsr: add shared-mutation regression test

[Jakub Kicinski]

On Sat,  8 Aug 2026 02:45:24 +0200 Xin Xie wrote:
> +	nsx python3 /dev/stdin <<'PYF1'
> +import socket, select, sys, time

It's a "no" on embedded Python in bash scripts.
Where do people get that from? Did AI generate it or are there projects
in which this is common practice?

Please write the whole test in Python (see .../net/lib/py/ helpers),
or move the python code out to its own scrip under TEST_FILES