[PATCH net] hsr: Avoid holding seqnr_lock while transmitting packets

From: Hangbin Liu <[email protected]>

Commit 06afd2c31d33 ("hsr: Synchronize sending frames to have always
incremented outgoing seq nr.") added spin lock around the whole
hsr_forward_skb() path to synchronize outgoing sequence number handling.
Commit 430d67bdcb04 ("net: hsr: Use the seqnr lock for frames received
via interlink port.") also use the same approach.

However, holding seqnr_lock while transmitting packets can cause lock
dependency issues when HSR devices are stacked with other net devices.
For example, when an HSR device is enslaved to a bridge and the bridge is
enslaved to another HSR device, hsr_dev_xmit() can be called recursively
through the networking stack. Although the two seqnr_lock instances are
different in this case, the same lock class can trigger a lockdep warning,
and more complex stacking topologies could lead to a real deadlock.

Since commit aae9d6b616b5 ("hsr: Implement more robust duplicate discard
for HSR"), sequence numbers are stored in an array and no longer require
linear comparison. Protecting only the sequence number update is
sufficient. There is no need to hold seqnr_lock during the whole
forwarding operation.

Revert commit 06afd2c31d33 ("hsr: Synchronize sending frames to have always
incremented outgoing seq nr.") and commit 430d67bdcb04 ("net: hsr: Use the
seqnr lock for frames received via interlink port.") to avoid holding
seqnr_lock while transmitting packets.

Fixes: 06afd2c31d33 ("hsr: Synchronize sending frames to have always incremented outgoing seq nr.")
Fixes: 430d67bdcb04 ("net: hsr: Use the seqnr lock for frames received via interlink port.")
Reported-by: [email protected]
Closes: https://syzkaller.appspot.com/bug?extid=fbf74291c3b7e753b481
Signed-off-by: Hangbin Liu <[email protected]>
---
I'm not sure if I should add these 2 fixes as this patch is depend on
Felix's aae9d6b616b5 ("hsr: Implement more robust duplicate discard for
HSR"). Please correct me if I need a change.
---
 net/hsr/hsr_device.c  | 12 +++++-------
 net/hsr/hsr_forward.c |  3 ++-
 net/hsr/hsr_slave.c   | 11 +----------
 3 files changed, 8 insertions(+), 18 deletions(-)

diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
index 5555b71ab19b..14e8d0676229 100644
--- a/net/hsr/hsr_device.c
+++ b/net/hsr/hsr_device.c
@@ -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);
 	} else {
 		dev_core_stats_tx_dropped_inc(dev);
 		dev_kfree_skb_any(skb);
@@ -335,6 +333,7 @@ static void send_hsr_supervision_frame(struct hsr_port *port,
 		hsr_stag->sequence_nr = htons(hsr->sequence_nr);
 		hsr->sequence_nr++;
 	}
+	spin_unlock_bh(&hsr->seqnr_lock);
 
 	hsr_stag->tlv.HSR_TLV_type = type;
 	/* HSRv0 has 6 unused bytes after the MAC */
@@ -356,13 +355,11 @@ static void send_hsr_supervision_frame(struct hsr_port *port,
 		ether_addr_copy(hsr_sp->macaddress_A, hsr->macaddress_redbox);
 	}
 
-	if (skb_put_padto(skb, ETH_ZLEN)) {
-		spin_unlock_bh(&hsr->seqnr_lock);
+	if (skb_put_padto(skb, ETH_ZLEN))
 		return;
-	}
 
 	hsr_forward_skb(skb, port);
-	spin_unlock_bh(&hsr->seqnr_lock);
+
 	return;
 }
 
@@ -402,8 +399,9 @@ static void send_prp_supervision_frame(struct hsr_port *master,
 		return;
 	}
 
-	hsr_forward_skb(skb, master);
 	spin_unlock_bh(&hsr->seqnr_lock);
+
+	hsr_forward_skb(skb, master);
 }
 
 /* Announce (supervision frame) timer function
diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
index 0774981a65c1..8e4158a9b57c 100644
--- a/net/hsr/hsr_forward.c
+++ b/net/hsr/hsr_forward.c
@@ -621,9 +621,10 @@ static void handle_std_frame(struct sk_buff *skb,
 	if (port->type == HSR_PT_MASTER ||
 	    port->type == HSR_PT_INTERLINK) {
 		/* Sequence nr for the master/interlink node */
-		lockdep_assert_held(&hsr->seqnr_lock);
+		spin_lock_bh(&hsr->seqnr_lock);
 		frame->sequence_nr = hsr->sequence_nr;
 		hsr->sequence_nr++;
+		spin_unlock_bh(&hsr->seqnr_lock);
 	}
 }
 
diff --git a/net/hsr/hsr_slave.c b/net/hsr/hsr_slave.c
index 01c73b4b50dd..267ffbd3c838 100644
--- a/net/hsr/hsr_slave.c
+++ b/net/hsr/hsr_slave.c
@@ -73,16 +73,7 @@ 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.
-	 */
-	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_skb(skb, port);
 
 finish_consume:
 	return RX_HANDLER_CONSUMED;

---
base-commit: 2a33516f9ef59ad11844d4fc152f889449b5daf3
change-id: 20260804-hsr_deadlock-40fd40b36154

Best regards,
-- 
Hangbin Liu <[email protected]>