netdev - [RFC PATCH 2/2] net: bonding: don't call ethtool methods under RCU

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240718122017.d2e33aaac43a.I10ab9c9ded97163aef4e4de10985cd8f7de60d28@changeid>
Date: Thu, 18 Jul 2024 12:20:17 -0700
From: Johannes Berg <johannes@...solutions.net>
To: netdev@...r.kernel.org
Cc: Eric Dumazet <edumazet@...gle.com>,
	Johannes Berg <johannes.berg@...el.com>,
	syzbot+2120b9a8f96b3fa90bad@...kaller.appspotmail.com
Subject: [RFC PATCH 2/2] net: bonding: don't call ethtool methods under RCU

From: Johannes Berg <johannes.berg@...el.com>

Currently, bond_miimon_inspect() is called under RCU, but it
calls ethtool ops. Since my earlier commit facd15dfd691
("net: core: synchronize link-watch when carrier is queried")
this is no longer permitted in the general ethtool case, but
it was already not permitted for many drivers such as USB in
which it can sleep to do MDIO register accesses etc.

Therefore, it's better to simply not do this. Change bonding
to acquire the RTNL for the MII monitor work directly to call
the bond_miimon_inspect() function and thus ethtool ops.

Reported-by: syzbot+2120b9a8f96b3fa90bad@...kaller.appspotmail.com
Signed-off-by: Johannes Berg <johannes.berg@...el.com>
---
 drivers/net/bonding/bond_main.c | 49 +++++++++++----------------------
 1 file changed, 16 insertions(+), 33 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 2ed0da068490..6a635c23b00e 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2576,7 +2576,6 @@ static int bond_slave_info_query(struct net_device *bond_dev, struct ifslave *in
 
 /*-------------------------------- Monitoring -------------------------------*/
 
-/* called with rcu_read_lock() */
 static int bond_miimon_inspect(struct bonding *bond)
 {
 	bool ignore_updelay = false;
@@ -2585,17 +2584,17 @@ static int bond_miimon_inspect(struct bonding *bond)
 	struct slave *slave;
 
 	if (BOND_MODE(bond) == BOND_MODE_ACTIVEBACKUP) {
-		ignore_updelay = !rcu_dereference(bond->curr_active_slave);
+		ignore_updelay = !rcu_access_pointer(bond->curr_active_slave);
 	} else {
 		struct bond_up_slave *usable_slaves;
 
-		usable_slaves = rcu_dereference(bond->usable_slaves);
+		usable_slaves = rtnl_dereference(bond->usable_slaves);
 
 		if (usable_slaves && usable_slaves->count == 0)
 			ignore_updelay = true;
 	}
 
-	bond_for_each_slave_rcu(bond, slave, iter) {
+	bond_for_each_slave(bond, slave, iter) {
 		bond_propose_link_state(slave, BOND_LINK_NOCHANGE);
 
 		link_state = bond_check_dev_link(bond, slave->dev, 0);
@@ -2807,8 +2806,7 @@ static void bond_mii_monitor(struct work_struct *work)
 {
 	struct bonding *bond = container_of(work, struct bonding,
 					    mii_work.work);
-	bool should_notify_peers = false;
-	bool commit;
+	bool should_notify_peers;
 	unsigned long delay;
 	struct slave *slave;
 	struct list_head *iter;
@@ -2818,45 +2816,30 @@ static void bond_mii_monitor(struct work_struct *work)
 	if (!bond_has_slaves(bond))
 		goto re_arm;
 
-	rcu_read_lock();
-	should_notify_peers = bond_should_notify_peers(bond);
-	commit = !!bond_miimon_inspect(bond);
-	if (bond->send_peer_notif) {
-		rcu_read_unlock();
-		if (rtnl_trylock()) {
-			bond->send_peer_notif--;
-			rtnl_unlock();
-		}
-	} else {
-		rcu_read_unlock();
+	/* deadlock avoidance with bond_close cancel of workqueue */
+	if (!rtnl_trylock()) {
+		delay = 1;
+		goto re_arm;
 	}
 
-	if (commit) {
-		/* Race avoidance with bond_close cancel of workqueue */
-		if (!rtnl_trylock()) {
-			delay = 1;
-			should_notify_peers = false;
-			goto re_arm;
-		}
+	should_notify_peers = bond_should_notify_peers(bond);
 
+	if (bond_miimon_inspect(bond)) {
 		bond_for_each_slave(bond, slave, iter) {
 			bond_commit_link_state(slave, BOND_SLAVE_NOTIFY_LATER);
 		}
 		bond_miimon_commit(bond);
-
-		rtnl_unlock();	/* might sleep, hold no other locks */
 	}
 
+	if (bond->send_peer_notif)
+		bond->send_peer_notif--;
+	if (should_notify_peers)
+		call_netdevice_notifiers(NETDEV_NOTIFY_PEERS, bond->dev);
+	rtnl_unlock();	/* might sleep, hold no other locks */
+
 re_arm:
 	if (bond->params.miimon)
 		queue_delayed_work(bond->wq, &bond->mii_work, delay);
-
-	if (should_notify_peers) {
-		if (!rtnl_trylock())
-			return;
-		call_netdevice_notifiers(NETDEV_NOTIFY_PEERS, bond->dev);
-		rtnl_unlock();
-	}
 }
 
 static int bond_upper_dev_walk(struct net_device *upper,
-- 
2.45.2