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, bond and team masters (also without
ports, since ports can be added later), and macvlan and MACsec devices
as slave or interlink. The rule is by device type, not by the presence
of lower devices, because a DSA user port is an upper of its conduit
and can offload HSR. VLAN devices are already refused. A tunnel device
whose route leads back through the HSR master, or a software device
without an identifying flag such as batman-adv, is not covered.
Also refuse, before any state is changed, a device whose RX handler is
in use or not supported (IFF_NO_RX_HANDLER: ipvlan, VRF), with an
extack message and the same error codes. Some of them, 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.
A bridge can no longer be used directly as the interlink; connect it
through a veth pair.
Reported-by: <redacted>
Closes: https://syzkaller.appspot.com/bug?extid=fbf74291c3b7e753b481
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.")
Signed-off-by: Xin Xie <redacted>
---
net/hsr/hsr_slave.c | 30 ++++++++++++++++++++++++++++--
1 file changed, 28 insertions(+), 2 deletions(-)
diff --git a/net/hsr/hsr_slave.c b/net/hsr/hsr_slave.c
index bc340ae0b52e..26f16d98e479 100644
--- a/net/hsr/hsr_slave.c
+++ b/net/hsr/hsr_slave.c
@@ -120,6 +120,22 @@ static int hsr_check_dev_ok(struct net_device *dev,
return -EINVAL;
}
+ /* The RX handler is taken by devices such as bridge/bond/team
+ * ports, macvlan and ipvlan lowers and HSR ports, or unsupported
+ * (IFF_NO_RX_HANDLER). Refuse before any state change, so a
+ * failed attach leaves the device as it was.
+ */
+ if (dev->priv_flags & IFF_NO_RX_HANDLER) {
+ NL_SET_ERR_MSG_MOD(extack,
+ "This device does not support an RX handler.");
+ return -EINVAL;
+ }
+ if (netdev_is_rx_handler_busy(dev)) {
+ NL_SET_ERR_MSG_MOD(extack,
+ "RX handler busy: device is already a lower of another device.");
+ return -EBUSY;
+ }
+
if (is_vlan_dev(dev)) {
NL_SET_ERR_MSG_MOD(extack, "HSR on top of VLAN is not yet supported in this driver.");
return -EINVAL;@@ -131,9 +147,19 @@ static int hsr_check_dev_ok(struct net_device *dev,
return -EOPNOTSUPP;
}
- /* HSR over bonded devices has not been tested, but I'm not sure it
- * won't work...
+ /* Forwarding devices feed HSR frames into another device's
+ * transmit path. A bridge, bond, team or OVS master or a macvlan
+ * or MACsec device makes no sense as a ring or LAN port and can
+ * nest seqnr_lock through a second HSR device. Refuse them by
+ * type: devices with lower devices in general stay valid, DSA
+ * user ports are uppers of their conduit.
*/
+ if (netif_is_any_bridge_master(dev) || netif_is_lag_master(dev) ||
+ netif_is_macvlan(dev) || netif_is_macsec(dev)) {
+ NL_SET_ERR_MSG_MOD(extack,
+ "Cannot use a bridge, bond, team, OVS, macvlan or MACsec device as HSR port.");
+ return -EINVAL;
+ }
return 0;
}--
2.53.0