net: hsr: fix super-packet forwarding

HSR and PRP tag, number and de-duplicate each frame on the wire. GRO and GSO can hand them one skb holding several frames, which the lower device then drops or which goes out with a single tag or RCT and sequence number.

Patch 1 keeps software GRO off on HSR/PRP ports. GRO_HW that the driver keeps on is left alone.

Patch 2 refuses bridge, bond, team and OVS masters as ports, a macvlan or MACsec device on top of a bridge, bond, team or HSR/PRP master, and devices that already have a master or cannot take an RX handler. This fixes the syzbot recursive seqnr_lock report, as Sebastian suggested [1].

A port check cannot catch every path back into an HSR master, such as a tc/BPF redirect or a tunnel; those still nest the lock and can deadlock on the same device. Fixing them without dropping traffic needs seqnr_lock released before the port transmit, which can reorder frames for receivers without aae9d6b616b5 ("hsr: Implement more robust duplicate discard for HSR"). That is left to net-next, where Hangbin's patch [2] fits. He dropped it in favor of this series [3]; as this series no longer narrows seqnr_lock, it can go ahead there.

Patch 3 segments GSO skbs before per-frame processing, outside seqnr_lock. GSO reaches an HSR/PRP port even with GRO off, typically from a container attached to a PRP RedBox interlink through veth:

  container/netns                  host PRP RedBox
  prp0-peer ---- veth ---- prp0-int (interlink RX)
                                  |
                           split GSO into frames
                                  |
                           sequence number + RCT
                              /          \
                           LAN A        LAN B

Some old Linux PRP senders put one RCT on a whole GSO skb; patch 3 drops such aggregates when it can detect them. With GRO_HW off on virtio-net, the host may segment them first and the RCT arrives as payload; upgrade the sender or disable GSO and TSO on its HSR/PRP master.

Patch 4 adds selftests.

Tested on a lockdep kernel: the new selftests (also with Python 3.9) and the existing hsr selftests pass, and the syzbot reproducer no longer reports recursive locking. No new W=1 warnings.

No stable backport is requested. The remaining HSR dev->stats races are left to a separate series, as Paolo suggested [4].

Based on net 30c71476d842; also applies to net-next. Patch 3 conflicts with Sebastian's pending net-next series [5]; its notes give the resolution.

Changes since v8:
- Patch 1: the message and comments say which ports the rule covers.
- Patch 2: macvlan and MACsec are refused only on top of a bridge, bond, team or HSR/PRP master; all ports are checked before any is changed.
- Patch 3: copy a head shared with a clone before segmenting.
- Patch 4: no fixed sleep, no need for iproute2 to print the HSR version, new refusal cases.

[1] https://lore.kernel.org/netdev/[email protected]/
[2] https://lore.kernel.org/netdev/[email protected]/
[3] https://lore.kernel.org/netdev/anXnxn-PBv8qH6VB@fedora/
[4] https://lore.kernel.org/netdev/[email protected]/
[5] https://lore.kernel.org/netdev/[email protected]/

v8: https://lore.kernel.org/netdev/[email protected]/
v7: https://lore.kernel.org/netdev/[email protected]/
v6: https://lore.kernel.org/netdev/[email protected]/
v5: https://lore.kernel.org/netdev/[email protected]/
v4: https://lore.kernel.org/netdev/[email protected]/
v3: https://lore.kernel.org/netdev/[email protected]/
v2: https://lore.kernel.org/netdev/[email protected]/
v1: https://lore.kernel.org/netdev/[email protected]/

Xin Xie (4):
net: hsr: keep GRO disabled on HSR/PRP ports
net: hsr: refuse forwarding devices as HSR/PRP ports
net: hsr: segment GSO before per-frame forwarding
selftests: net: hsr: verify GRO policy and ordered forwarding

 .../networking/net_cachelines/net_device.rst  |    1 +
 include/linux/netdevice.h                     |    8 +
 net/core/dev.c                                |   15 +
 net/hsr/hsr_device.c                          |   13 +-
 net/hsr/hsr_forward.c                         |  158 ++
 net/hsr/hsr_forward.h                         |    1 +
 net/hsr/hsr_slave.c                           |  145 +-
 net/hsr/hsr_slave.h                           |    2 +
 tools/testing/selftests/net/hsr/Makefile      |    7 +-
 tools/testing/selftests/net/hsr/config        |   10 +
 .../selftests/net/hsr/hsr_gro_superpacket.py  | 1456 +++++++++++++++++
 .../selftests/net/hsr/hsr_gro_superpacket.sh  |    9 +
 .../net/hsr/hsr_ordered_forwarding.sh         |    9 +
 13 files changed, 1819 insertions(+), 15 deletions(-)
 create mode 100755 tools/testing/selftests/net/hsr/hsr_gro_superpacket.py
 create mode 100755 tools/testing/selftests/net/hsr/hsr_gro_superpacket.sh
 create mode 100755 tools/testing/selftests/net/hsr/hsr_ordered_forwarding.sh

base-commit: 30c71476d842b094651bd7712f6c8ee8716147d0

2.53.0

HSR and PRP tag and de-duplicate each wire frame separately. GRO on a member device merges several frames into one skb that cannot get valid per-frame metadata, and userspace can turn GRO back on at any time.

Mark direct members (slave A/B, interlink) with a kernel role bit. While it is set, __netdev_update_features() drops software GRO and configurable GRO_HW from wanted_features as well as from the computed features, before ndo_fix_features(). GRO_HW that the driver keeps on is left alone. The request has to go too: veth adds and deletes its NAPI instances according to wanted_features, so with GRO still requested, closing an attached veth spins in napi_disable(), and detaching a veth that was attached while down and then brought up hits the BUG_ON() in napi_enable(). As for LRO disabled by an upper device, GRO is not restored when the port leaves; ethtool can turn it back on. The rule covers every direct member, PRP ports and ports with HSR offload included: one simple rule is preferred to per-port exceptions, at some cost in receive throughput.

The attach fails if software GRO stays active. A failed attach does not restore the GRO request either. The next patch checks all ports before any of them is changed, so only a failure at run time, for example a driver that cannot turn GRO off or the upper device nesting limit (-EMLINK), leaves a port with its GRO request dropped. GSO skbs can still arrive, for example from a stacked member whose lower device runs GRO; a later patch segments them.

---
 .../networking/net_cachelines/net_device.rst  |  1 +
 include/linux/netdevice.h                     |  8 +++
 net/core/dev.c                                | 15 ++++++
 net/hsr/hsr_slave.c                           | 54 +++++++++++++++++++
 4 files changed, 78 insertions(+)

index 8eceaa80b686..0d87e4d92a58 100644
@@ -169,6 +169,7 @@ unsigned_long:1                     see_all_hwtstamp_requests
 unsigned_long:1                     change_proto_down
 unsigned_long:1                     netns_immutable
 unsigned_long:1                     fcoe_mtu
+unsigned_long:1                     hsr_port
 struct list_head                    net_notifier_list
 struct macsec_ops*                  macsec_ops
 struct udp_tunnel_nic_info*         udp_tunnel_nic_info

index 435be7880d7e..0d13c7ecdd1f 100644
@@ -2110,6 +2110,7 @@ enum netdev_reg_state {
  *	@change_proto_down: device supports setting carrier via IFLA_PROTO_DOWN
  *	@netns_immutable: interface can't change network namespaces
  *	@fcoe_mtu:	device supports maximum FCoE MTU, 2158 bytes
+ *	@hsr_port:	direct HSR/PRP member (slave A/B or interlink)
  *
  *	@net_notifier_list:	List of per-net netdev notifier block
  *				that follow this device when it is moved

@@ -2531,6 +2532,13 @@ struct net_device {
 	unsigned long		change_proto_down:1;
 	unsigned long		netns_immutable:1;
 	unsigned long		fcoe_mtu:1;
+	/* Direct HSR/PRP member: slave A/B or interlink. While set,
+	 * software GRO and configurable GRO_HW are dropped from the
+	 * request and before ndo_fix_features(); GRO_HW that the driver
+	 * keeps on is left alone. Kernel role state only; no user ABI or
+	 * offload capability.
+	 */
+	unsigned long		hsr_port:1;
 
 	struct list_head	net_notifier_list;
 

index df1a840b0334..c8c2de212556 100644
@@ -11269,6 +11269,21 @@ int __netdev_update_features(struct net_device *dev)
 
 	features = netdev_get_wanted_features(dev);
 
+	/* A direct HSR/PRP port needs per-frame metadata. Drop software
+	 * GRO and configurable GRO_HW from the request as well as from the
+	 * features passed to ndo_fix_features(), as the core does for
+	 * upper-disabled features: drivers such as veth derive state from
+	 * wanted_features and need the requested and the active bit to
+	 * agree. Fixed-on or driver-required GRO_HW is left to the driver.
+	 */
+	if (dev->hsr_port) {
+		netdev_features_t gro = NETIF_F_GRO |
+					(dev->hw_features & NETIF_F_GRO_HW);
+
+		dev->wanted_features &= ~gro;
+		features &= ~gro;
+	}
+
 	if (dev->netdev_ops->ndo_fix_features)
 		features = dev->netdev_ops->ndo_fix_features(dev, features);
 

2.53.0

syzbot reported recursive locking with an HSR device whose slave is a bridge, and that bridge has the master of a second HSR device as a port. The first device transmits its supervision frames with its seqnr_lock held; the bridge floods them to the second HSR master, whose hsr_dev_xmit() takes the seqnr_lock of that device:

WARNING: possible recursive locking detected
...

   hsr_dev_xmit+0x18a/0x210 net/hsr/hsr_device.c:234
   ...
   br_dev_xmit+0x1202/0x1b40
   ...
   hsr_forward_skb+0x171c/0x2ac0 net/hsr/hsr_forward.c:728
   send_hsr_supervision_frame+0x63b/0xcc0 net/hsr/hsr_device.c:351
   hsr_announce+0x1f8/0x3a0 net/hsr/hsr_device.c:408

Interlink RX forwards to the slaves with the same lock held. A chain of two devices does not deadlock, but such a port makes no sense: an HSR ring and the two PRP LANs are attached to the node directly.

Refuse bridge and OVS masters and bond and team masters (also without ports, since ports can be added later) as slave or interlink, and a macvlan or MACsec device when an HSR/PRP, bridge, bond or team master is below it; on an ordinary NIC or veth such a device stays valid. This also refuses one on a DSA user port whose conduit is a bond or team, since it can gain ports later too. A macvlan takes its GRO setting from its lower device, so it is accepted only if GRO is off there when it is attached. The lower devices of other ports are not checked, because a DSA user port is an upper of its conduit and can offload HSR. VLAN devices are already refused. The check runs at attach; later changes below a stacked device are not rechecked. Not covered are a tunnel whose route leads back through the HSR master, a tc or BPF redirect to it, other stacked devices, and a software device without an identifying flag such as batman-adv. Narrowing seqnr_lock, planned for net-next, addresses that class as a whole.

Also refuse a device whose RX handler is in use or not supported (IFF_NO_RX_HANDLER: ipvlan, VRF), or that already has a master, such as a VRF port, with an extack message and the same error codes. All ports are checked before any of them is changed. Some of these devices, such as a macvlan lower, ipvlan or VRF, used to fail only in netdev_rx_handler_register(), after the previous patch had dropped their GRO request; a refused device now keeps its state, and so do the ports given with it.

A bridge can no longer be used directly as the interlink; connect it through a veth pair.

---
 net/hsr/hsr_device.c |  7 ++++
 net/hsr/hsr_slave.c  | 79 ++++++++++++++++++++++++++++++++++++++++++--
 net/hsr/hsr_slave.h  |  2 ++
 3 files changed, 86 insertions(+), 2 deletions(-)

index fc512204600c..90ee55eb3c19 100644
@@ -758,6 +758,13 @@ int hsr_dev_finalize(struct net_device *hsr_dev, struct net_device *slave[2],
 	INIT_LIST_HEAD(&hsr->proxy_node_db);
 	spin_lock_init(&hsr->list_lock);
 
+	/* Refuse a bad port before any port is changed. The same checks
+	 * in hsr_add_port() remain as a safety net.
+	 */
+	res = hsr_check_ports(slave, interlink, extack);
+	if (res)
+		return res;
+
 	eth_hw_addr_set(hsr_dev, slave[0]->dev_addr);
 
 	/* initialize protocol specific functions */

index edc4612bb009..e19c779b500d 100644
@@ -14,6 +14,8 @@
 #include <linux/rtnetlink.h>
 #include "hsr_main.h"
 
+int hsr_check_ports(struct net_device *slave[2], struct net_device *interlink,
+		    struct netlink_ext_ack *extack);
 int hsr_add_port(struct hsr_priv *hsr, struct net_device *dev,
 		 enum hsr_port_type pt, struct netlink_ext_ack *extack);
 void hsr_del_port(struct hsr_port *port);

2.53.0

HSR and PRP tag and de-duplicate each wire frame separately. A GSO super-packet reaching the forwarding path hides several wire frames, which cannot be tagged or numbered per frame. Such skbs reach a member even with GRO off, for example from a container or VM attached through veth, tap or virtio to a software RedBox, or from a member whose GRO_HW cannot be turned off.

Add one forwarding entry, hsr_forward_input(), for master TX and for frames received on any port. It segments GSO input outside seqnr_lock; each segment is then numbered and submitted under the lock, one frame at a time as before, and follows the normal forwarding and local delivery paths. Master TX and interlink RX now take the lock there instead of in their callers.

Classify by the effective inner protocol with the core's bounded VLAN decoder (802.1Q, 802.1AD, nested or accelerated). Drop aggregates with inner HSR/PRP, unreadable fragments or undecodable headers, and IPv4 or IPv6 aggregates whose nonzero IP length ends before the skb does, since the trailing bytes would become segment payload. GSO_PARTIAL and zero IP lengths are left to the segmenter. Drops are counted once on the entry device in the core per-CPU drop counters. Segmentation writes the control block, the headers and the L4 checksum, so a still-shared input is unshared first, and a head still shared with a clone, for example from a packet socket, is copied.

Older HSR and PRP senders can pass a whole GSO skb with one tag or one RCT, for example over tap or virtio. Such aggregates, including locally destined ones, are now dropped instead of being delivered.

Remove the NETIF_F_GSO_MASK types from the master's hw_features so the core segments local traffic before ndo_start_xmit(). This also holds over ports with HSR tag insertion offload, at some cost in performance.

---
Integration with net-next: Sebastian's pending series "hsr: Allow to send packets on specific port and pass the timestamp" (v10) changes hsr_forward_skb() to take the requested TX port and a has_hsr_header flag, and keeps seqnr_lock in hsr_dev_xmit() and in the interlink path of hsr_handle_frame(). With both series:
- keep v10's PTP header parsing in hsr_dev_xmit(), then call hsr_forward_input(skb, master, req_tx_port, has_hsr_header) without taking seqnr_lock there;
- call hsr_forward_input(skb, port, HSR_PT_NONE, false) from hsr_handle_frame(), without the interlink lock;
- add both arguments to hsr_forward_input() and hsr_forward_frame() and pass them to hsr_forward_skb() for the skb and for each segment;
- keep both include sets in hsr_forward.c.
A frame with the PTP header is not GSO and takes the single-frame path under the lock, as in v10. A GSO input with the header is classified after the pull: with an HSR header it is dropped like any inner HSR/PRP aggregate, otherwise each segment keeps req_tx_port.
Checked: on net-next fa5196392691 with v10 applied, this series with the resolution builds without warnings, and on a lockdep kernel hsr_ptp.sh, the two tests of patch 4 and the existing hsr selftests pass.

 net/hsr/hsr_device.c  |   6 +-
 net/hsr/hsr_forward.c | 158 ++++++++++++++++++++++++++++++++++++++++++
 net/hsr/hsr_forward.h |   1 +
 net/hsr/hsr_slave.c   |  12 ++--
 4 files changed, 165 insertions(+), 12 deletions(-)

index 90ee55eb3c19..5db71b49d08e 100644
@@ -232,9 +232,7 @@ static netdev_tx_t hsr_dev_xmit(struct sk_buff *skb, struct net_device *dev)
 		skb->dev = master->dev;
 		skb_reset_mac_header(skb);
 		skb_reset_mac_len(skb);
-		spin_lock_bh(&hsr->seqnr_lock);
-		hsr_forward_skb(skb, master);
-		spin_unlock_bh(&hsr->seqnr_lock);
+		hsr_forward_input(skb, master);
 	} else {
 		dev_core_stats_tx_dropped_inc(dev);
 		dev_kfree_skb_any(skb);

@@ -686,7 +684,7 @@ void hsr_dev_setup(struct net_device *dev)
 	dev->needs_free_netdev = true;
 
 	dev->hw_features = NETIF_F_SG | NETIF_F_FRAGLIST | NETIF_F_HIGHDMA |
-			   NETIF_F_GSO_MASK | NETIF_F_HW_CSUM |
+			   NETIF_F_HW_CSUM |
 			   NETIF_F_HW_VLAN_CTAG_TX |
 			   NETIF_F_HW_VLAN_CTAG_FILTER;
 

index 206636750b30..ec08c4330259 100644
@@ -14,6 +14,7 @@
 #include "hsr_main.h"
 
 void hsr_forward_skb(struct sk_buff *skb, struct hsr_port *port);
+void hsr_forward_input(struct sk_buff *skb, struct hsr_port *port);
 struct sk_buff *prp_create_tagged_frame(struct hsr_frame_info *frame,
 					struct hsr_port *port);
 struct sk_buff *hsr_create_tagged_frame(struct hsr_frame_info *frame,

index 666220b24dee..7fe7fa68af9e 100644
@@ -76,15 +76,11 @@ static rx_handler_result_t hsr_handle_frame(struct sk_buff **pskb)
 	skb_reset_mac_len(skb);
 
 	/* Only the frames received over the interlink port will assign a
-	 * sequence number and require synchronisation vs other sender.
+	 * sequence number and require synchronisation vs other sender;
+	 * hsr_forward_input() takes seqnr_lock for them, one frame at a
+	 * time.
 	 */
-	if (port->type == HSR_PT_INTERLINK) {
-		spin_lock_bh(&hsr->seqnr_lock);
-		hsr_forward_skb(skb, port);
-		spin_unlock_bh(&hsr->seqnr_lock);
-	} else {
-		hsr_forward_skb(skb, port);
-	}
+	hsr_forward_input(skb, port);
 
 finish_consume:
 	return RX_HANDLER_CONSUMED;

2.53.0

Add two tests, run by shell entries that exec one installed Python helper.

hsr_gro_superpacket.sh checks the member GRO policy: GRO is off, requested and active, while a veth is a member and after it leaves, and can then be turned on again; a device refused at attach keeps its request. It covers close and reopen of an attached veth, a veth attached while down, the master's GSO type mask, a refused bridge port that leaves the port given before it unchanged, a busy macvlan lower, the refusal of bridge, bond and ipvlan devices, of a macvlan on a bridge or on a bond and of a VRF port without leaving an upper link or a promiscuity reference on any port, a macvlan whose lower device runs GRO that is refused as second port with the first port unchanged, and a macvlan on a veth without GRO that is accepted. Natural TCP from the interlink is compared byte for byte, retransmissions included, with both LAN captures. TCP from a SAN on one PRP LAN, in plain and VLAN forms, is compared the same way with the ingress LAN and the local delivery, and its reply must appear once on each LAN. Tags/RCT, sequence numbers and VLAN stacks are checked.

hsr_ordered_forwarding.sh checks local data around interlink data and UDP GSO aggregates, local and proxy supervision, two concurrent producers and an observed sequence-number wrap.

Work is bounded to 120 seconds and cleanup to 20; INT/TERM remove the test's namespaces. Packets are judged only on complete captures without socket loss or truncation. A missing prerequisite SKIPs the affected case; query errors and missing packet evidence FAIL, and the cases after a failure are reported as not ok.

---
 tools/testing/selftests/net/hsr/Makefile      |    7 +-
 tools/testing/selftests/net/hsr/config        |   10 +
 .../selftests/net/hsr/hsr_gro_superpacket.py  | 1456 +++++++++++++++++
 .../selftests/net/hsr/hsr_gro_superpacket.sh  |    9 +
 .../net/hsr/hsr_ordered_forwarding.sh         |    9 +
 5 files changed, 1490 insertions(+), 1 deletion(-)
 create mode 100755 tools/testing/selftests/net/hsr/hsr_gro_superpacket.py
 create mode 100755 tools/testing/selftests/net/hsr/hsr_gro_superpacket.sh
 create mode 100755 tools/testing/selftests/net/hsr/hsr_ordered_forwarding.sh

index 2150e487ac7d..70f8a804b44a 100644
@@ -3,6 +3,8 @@
 top_srcdir = ../../../../..
 
 TEST_PROGS := \
+	hsr_gro_superpacket.sh \
+	hsr_ordered_forwarding.sh \
 	hsr_ping.sh \
 	hsr_prp_redbox.sh \
 	hsr_redbox.sh \

@@ -10,6 +12,9 @@ TEST_PROGS := \
 	prp_ping.sh \
 # end of TEST_PROGS
 
-TEST_FILES += hsr_common.sh
+TEST_FILES += \
+	hsr_common.sh \
+	hsr_gro_superpacket.py \
+# end of TEST_FILES
 
 include ../../lib.mk

index 205cc4d3d64b..d9463ae0025a 100644
@@ -1,6 +1,16 @@
+CONFIG_BONDING=y
 CONFIG_BRIDGE=y
 CONFIG_HSR=y
+CONFIG_INET=y
+CONFIG_IP_ADVANCED_ROUTER=y
+CONFIG_IP_MULTIPLE_TABLES=y
 CONFIG_IPV6=y
+CONFIG_IPV6_MULTIPLE_TABLES=y
+CONFIG_IPVLAN=y
+CONFIG_MACVLAN=y
+CONFIG_NET_L3_MASTER_DEV=y
 CONFIG_NET_SCH_NETEM=m
+CONFIG_NET_VRF=y
+CONFIG_PACKET=y
 CONFIG_VETH=y
 CONFIG_VLAN_8021Q=m

new file mode 100755
index 000000000000..9da711ad2ab2
@@ -0,0 +1,9 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+if ! command -v python3 > /dev/null; then
+	echo "SKIP: missing python3"
+	exit 4
+fi
+
+exec python3 -B "$(dirname "$0")/hsr_gro_superpacket.py" gro "$@"

new file mode 100755
index 000000000000..b43218709455
@@ -0,0 +1,9 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+if ! command -v python3 > /dev/null; then
+	echo "SKIP: missing python3"
+	exit 4
+fi
+
+exec python3 -B "$(dirname "$0")/hsr_gro_superpacket.py" ordered "$@"

2.53.0

I did not forget you. Thanks for the repost. I try to handle it before Monday.

Sebastian