Fix RTNL: assertion failed at net/core/rtnetlink.c

The commit 1d3ee88ae0d
(bonding: add netlink attributes to slave link dev) make the bond_set_active_slave() and bond_set_backup_slave() use rtmsg_ifinfo to send slave's states and this functions should be called in RTNL.

But the 902.3ad and ARP monitor did not hold the RTNL when calling thses two functions, so fix them.

v1->v2: Add new micro to indicate that the notification should be send
later, not never.
And add a new patch to fix the same problem for ARP mode.

v2->v3: modify the bond_should_notify to should_notify_rtnl, it is more
reasonable, and use bool for should_notify_rtnl.

Ding Tianhong (2):
bonding: Fix RTNL: assertion failed at net/core/rtnetlink.c for

    802.3ad mode

bonding: Fix RTNL: assertion failed at net/core/rtnetlink.c for ab
arp monitor

 drivers/net/bonding/bond_3ad.c  |  16 +++++-
 drivers/net/bonding/bond_main.c | 115 ++++++++++++++++++++++------------------
 drivers/net/bonding/bonding.h   |  47 ++++++++++++++--
 3 files changed, 120 insertions(+), 58 deletions(-)

1.8.0

The problem was introduced by the commit 1d3ee88ae0d (bonding: add netlink attributes to slave link dev). The bond_set_active_slave() and bond_set_backup_slave() will use rtmsg_ifinfo to send slave's states, so these two functions should be called in RTNL.

In 802.3ad mode, acquiring RTNL for the __enable_port and __disable_port cases is difficult, as those calls generally already hold the state machine lock, and cannot unconditionally call rtnl_lock because either they already hold RTNL (for calls via bond_3ad_unbind_slave) or due to the potential for deadlock with bond_3ad_adapter_speed_changed, bond_3ad_adapter_duplex_changed, bond_3ad_link_change, or bond_3ad_update_lacp_rate. All four of those are called with RTNL held, and acquire the state machine lock second. The calling contexts for __enable_port and __disable_port already hold the state machine lock, and may or may not need RTNL.

According to the Jay's opinion, I don't think it is a problem that the slave don't send notify message synchronously when the status changed, normally the state machine is running every 100 ms, send the notify message at the end of the state machine if the slave's state changed should be better.

I fix the problem through these steps:

1). add a new function bond_set_slave_state() which could change
the slave's state and call rtmsg_ifinfo() according to the input parameters called notify.

2). Add a new slave parameter which called should_notify, if the slave's state
changed and don't notify yet, the parameter will be set to 1, and then if the slave's state changed again, the param will be set to 0, it indicate that the slave's state has been restored, no need to notify any one.

3). the __enable_port and __disable_port should not call rtmsg_ifinfo
in the state machine lock, any change in the state of slave could set a flag in the slave, it will indicated that an rtmsg_ifinfo should be called at the end of the state machine.

---
 drivers/net/bonding/bond_3ad.c  | 22 ++++++++++++++++++++--
 drivers/net/bonding/bond_main.c | 41 ++++++++++++++++++++++++++---------------
 drivers/net/bonding/bonding.h   | 34 +++++++++++++++++++++++++++++-----
 3 files changed, 75 insertions(+), 22 deletions(-)

index 6d20fbd..6826e4f 100644
@@ -181,7 +181,7 @@ static inline int __agg_has_partner(struct aggregator *agg)
  */
 static inline void __disable_port(struct port *port)
 {
-	bond_set_slave_inactive_flags(port->slave);
+	bond_set_slave_inactive_flags(port->slave, BOND_SLAVE_NOTIFY_LATER);
 }
 
 /**

@@ -193,7 +193,7 @@ static inline void __enable_port(struct port *port)
 	struct slave *slave = port->slave;
 
 	if ((slave->link == BOND_LINK_UP) && IS_UP(slave->dev))
-		bond_set_slave_active_flags(slave);
+		bond_set_slave_active_flags(slave, BOND_SLAVE_NOTIFY_LATER);
 }
 
 /**

@@ -2062,6 +2062,7 @@ void bond_3ad_state_machine_handler(struct work_struct *work)
 	struct list_head *iter;
 	struct slave *slave;
 	struct port *port;
+	bool should_notify_rtnl = BOND_SLAVE_NOTIFY_LATER;
 
 	read_lock(&bond->lock);
 	rcu_read_lock();

@@ -2119,8 +2120,25 @@ void bond_3ad_state_machine_handler(struct work_struct *work)
 	}
 
 re_arm:
+	bond_for_each_slave_rcu(bond, slave, iter) {
+		if (slave->should_notify) {
+			should_notify_rtnl = BOND_SLAVE_NOTIFY_NOW;
+			break;
+		}
+	}
 	rcu_read_unlock();
 	read_unlock(&bond->lock);
+
+	if (should_notify_rtnl && rtnl_trylock()) {
+		bond_for_each_slave(bond, slave, iter) {
+			if (slave->should_notify) {
+				rtmsg_ifinfo(RTM_NEWLINK, slave->dev, 0,
+					     GFP_KERNEL);
+				slave->should_notify = 0;
+			}
+		}
+		rtnl_unlock();
+	}
 	queue_delayed_work(bond->wq, &bond->ad_work, ad_delta_in_ticks);
 }
 

index 86ccfb9..9b280ac 100644
@@ -195,7 +195,8 @@ struct slave {
 	s8     new_link;
 	u8     backup:1,   /* indicates backup slave. Value corresponds with
 			      BOND_STATE_ACTIVE and BOND_STATE_BACKUP */
-	       inactive:1; /* indicates inactive slave */
+	       inactive:1, /* indicates inactive slave */
+	       should_notify:1; /* indicateds whether the state changed */
 	u8     duplex;
 	u32    original_mtu;
 	u32    link_failure_count;

@@ -303,6 +304,24 @@ static inline void bond_set_backup_slave(struct slave *slave)
 	}
 }
 
+static inline void bond_set_slave_state(struct slave *slave,
+					int slave_state, bool notify)
+{
+	if (slave->backup == slave_state)
+		return;
+
+	slave->backup = slave_state;
+	if (notify) {
+		rtmsg_ifinfo(RTM_NEWLINK, slave->dev, 0, GFP_KERNEL);
+		slave->should_notify = 0;
+	} else {
+		if (slave->should_notify)
+			slave->should_notify = 0;
+		else
+			slave->should_notify = 1;
+	}
+}
+
 static inline void bond_slave_state_change(struct bonding *bond)
 {
 	struct list_head *iter;

@@ -343,6 +362,9 @@ static inline bool bond_is_active_slave(struct slave *slave)
 #define BOND_ARP_VALIDATE_ALL		(BOND_ARP_VALIDATE_ACTIVE | \
 					 BOND_ARP_VALIDATE_BACKUP)
 
+#define BOND_SLAVE_NOTIFY_NOW		true
+#define BOND_SLAVE_NOTIFY_LATER		false
+
 static inline int slave_do_arp_validate(struct bonding *bond,
 					struct slave *slave)
 {

@@ -394,17 +416,19 @@ static inline void bond_netpoll_send_skb(const struct slave *slave,
 }
 #endif
 
-static inline void bond_set_slave_inactive_flags(struct slave *slave)
+static inline void bond_set_slave_inactive_flags(struct slave *slave,
+						 bool notify)
 {
 	if (!bond_is_lb(slave->bond))
-		bond_set_backup_slave(slave);
+		bond_set_slave_state(slave, BOND_STATE_BACKUP, notify);
 	if (!slave->bond->params.all_slaves_active)
 		slave->inactive = 1;
 }
 
-static inline void bond_set_slave_active_flags(struct slave *slave)
+static inline void bond_set_slave_active_flags(struct slave *slave,
+					       bool notify)
 {
-	bond_set_active_slave(slave);
+	bond_set_slave_state(slave, BOND_STATE_ACTIVE, notify);
 	slave->inactive = 0;
 }
 

1.8.0

Veaceslav has reported and fix this problem by commit f2ebd477f141bc0 (bonding: restructure locking of bond_ab_arp_probe()). According Jay's opinion, the current solution is not very well, because the notification is to indicate that the interface has actually changed state in a meaningful way, but these calls in the ab ARP monitor are internal settings of the flags to allow the ARP monitor to search for a slave to become active when there are no active slaves. The flag setting to active or backup is to permit the ARP monitor's response logic to do the right thing when deciding if the test slave (current_arp_slave) is up or not.

So the best way to fix the problem is that we should not send a notification when the slave is in testing state, and check the state at the end of the monitor, if the slave's state recover, avoid to send pointless notification twice. And RTNL is really a big lock, hold it regardless the slave's state changed or not when the current_active_slave is null will loss performance (every 100ms), so we should hold it only when the slave's state changed and need to notify.

I revert the old commit and add new modifications.

---
 drivers/net/bonding/bond_3ad.c  |  8 +----
 drivers/net/bonding/bond_main.c | 80 +++++++++++++++++++++--------------------
 drivers/net/bonding/bonding.h   | 13 +++++++
 3 files changed, 55 insertions(+), 46 deletions(-)

index 6826e4f..dcde560 100644
@@ -2130,13 +2130,7 @@ re_arm:
 	read_unlock(&bond->lock);
 
 	if (should_notify_rtnl && rtnl_trylock()) {
-		bond_for_each_slave(bond, slave, iter) {
-			if (slave->should_notify) {
-				rtmsg_ifinfo(RTM_NEWLINK, slave->dev, 0,
-					     GFP_KERNEL);
-				slave->should_notify = 0;
-			}
-		}
+		bond_slave_state_notify(bond);
 		rtnl_unlock();
 	}
 	queue_delayed_work(bond->wq, &bond->ad_work, ad_delta_in_ticks);

index 9b280ac..2b0fdec 100644
@@ -335,6 +335,19 @@ static inline void bond_slave_state_change(struct bonding *bond)
 	}
 }
 
+static inline void bond_slave_state_notify(struct bonding *bond)
+{
+	struct list_head *iter;
+	struct slave *tmp;
+
+	bond_for_each_slave(bond, tmp, iter) {
+		if (tmp->should_notify) {
+			rtmsg_ifinfo(RTM_NEWLINK, tmp->dev, 0, GFP_KERNEL);
+			tmp->should_notify = 0;
+		}
+	}
+}
+
 static inline int bond_slave_state(struct slave *slave)
 {
 	return slave->backup;

1.8.0

Hello Ding,
the patch did not apply for me cleanly against Linus tip, so I manually merged it. The resulting patch is here:

http://pbot.rmdir.de/wHFa0-bLDuVEIT-5lnbIrQ

I tested it:

(node-62) [~] dmesg | pbot
http://pbot.rmdir.de/QeWgt5TNhISgRpENt6pCIg
(node-62) [~] pbot /proc/net/bonding/bond0
http://pbot.rmdir.de/zNSSqmjSI0o1Qvt6DrSEQw
(node-62) [~] pbot /proc/net/bonding/bond1
http://pbot.rmdir.de/CoI00Rguie2P-kOQytOM9w

Cheers,
Thomas

From: Ding Tianhong <[email protected]>
Date: Wed, 26 Feb 2014 11:05:21 +0800

Series applied, thank you.