hsr->seqnr_lock is currently held across entire hsr_forward_skb()
calls: master TX (hsr_dev_xmit()), interlink RX (hsr_handle_frame()),
and both supervision frame builders hold it while frames are built,
classified, duplicated and forwarded on every port. The only state
that actually needs the lock is the sequence counters themselves
(hsr->sequence_nr / hsr->sup_sequence_nr).
Shrink the locking to the individual counter updates:
handle_std_frame() now takes the lock around its sequence number
allocation (replacing the lockdep assertion), the master TX and
interlink RX paths drop their outer lock, and the supervision
builders release the lock right after updating their counter instead
of holding it across frame construction and forwarding.
Sequence numbers remain unique and monotonically allocated per
counter; concurrent inputs may now interleave allocations, which is
fine as the output paths were already concurrent.
This is a prerequisite for unfolding GSO super-packets at the forward
entry: segmentation with its dozens of GFP_ATOMIC allocations must
not run with BH disabled under the global sequence lock.
The locking being narrowed here was introduced by 06afd2c31d33
("hsr: Synchronize sending frames to have always incremented outgoing
seq nr."), briefly removed by b3c9e65eb227 ("net: hsr: remove
seqnr_lock") and reinstated for the interlink RX path by 430d67bdcb04
("net: hsr: Use the seqnr lock for frames received via interlink
port.") after a syzbot lockdep report. All sequence counter updates
remain protected; only the forwarding work moves out of the critical
section.
Cc: [email protected]
Signed-off-by: Xin Xie <[email protected]>
---
net/hsr/hsr_device.c | 15 ++++-----------
net/hsr/hsr_forward.c | 3 ++-
net/hsr/hsr_slave.c | 11 +----------
3 files changed, 7 insertions(+), 22 deletions(-)
diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
index 5555b71ab..3fd1762d8 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,14 +355,10 @@ 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;
}
static void send_prp_supervision_frame(struct hsr_port *master,
@@ -390,6 +385,7 @@ static void send_prp_supervision_frame(struct hsr_port
*master,
spin_lock_bh(&hsr->seqnr_lock);
hsr_stag->sequence_nr = htons(hsr->sup_sequence_nr);
hsr->sup_sequence_nr++;
+ spin_unlock_bh(&hsr->seqnr_lock);
hsr_stag->tlv.HSR_TLV_type = PRP_TLV_LIFE_CHECK_DD;
hsr_stag->tlv.HSR_TLV_length = sizeof(struct hsr_sup_payload);
@@ -397,13 +393,10 @@ static void send_prp_supervision_frame(struct hsr_port
*master,
hsr_sp = skb_put(skb, sizeof(struct hsr_sup_payload));
ether_addr_copy(hsr_sp->macaddress_A, master->dev->dev_addr);
- 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, master);
- spin_unlock_bh(&hsr->seqnr_lock);
}
/* Announce (supervision frame) timer function
diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
index 0774981a6..8e4158a9b 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 cefbbfbd5..c7fd021f0 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;
--
2.43.0
