netdev - Re: [PATCHv3 net-next] bonding: 3ad: send ifinfo notify when mux state changed

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: Windows password security audit tool. GUI, reports in PDF.

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

Date: Thu, 27 Jun 2024 16:26:11 +0800
From: Hangbin Liu <liuhangbin@...il.com>
To: Jay Vosburgh <jay.vosburgh@...onical.com>
Cc: Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
	Andy Gospodarek <andy@...yhouse.net>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
	Ido Schimmel <idosch@...dia.com>, Jiri Pirko <jiri@...nulli.us>,
	Amit Cohen <amcohen@...dia.com>,
	Nikolay Aleksandrov <razor@...ckwall.org>
Subject: Re: [PATCHv3 net-next] bonding: 3ad: send ifinfo notify when mux
 state changed

On Wed, Jun 26, 2024 at 05:06:00PM -0700, Jay Vosburgh wrote:
> >Hits:
> >
> >RTNL: assertion failed at net/core/rtnetlink.c (1823)

Thanks for this hits...

> >
> >On two selftests. Please run the selftests on a debug kernel..

OK, I will try run my tests on debug kernel in future.

> 
> 	Oh, I forgot about needing RTNL.
> 
> 	We cannot simply acquire RTNL in ad_mux_machine(), as the
> bond->mode_lock is already held, and the lock ordering must be RTNL
> first, then mode_lock, lest we deadlock.
> 
> 	Hangbin, I'd suggest you look at how bond_netdev_notify_work()
> complies with the lock ordering (basically, doing the actual work out of
> line in a workqueue event), or how the "should_notify" flag is used in
> bond_3ad_state_machine_handler().  The first is more complicated, but
> won't skip events; the second may miss intermediate state transitions if
> it cannot acquire RTNL and has to delay the notification.

I think the administer doesn't want to loose the state change info. So how
about something like:

diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index c6807e473ab7..046f11c5c47a 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -1185,6 +1185,8 @@ static void ad_mux_machine(struct port *port, bool *update_slave_arr)
 		default:
 			break;
 		}
+
+		port->slave->should_notify_lacp = 1;
 	}
 }
 
@@ -2527,6 +2529,9 @@ void bond_3ad_state_machine_handler(struct work_struct *work)
 		bond_slave_state_notify(bond);
 		rtnl_unlock();
 	}
+
+	/* Notify the mux state changes */
+	bond_slave_link_notify(bond);
 	queue_delayed_work(bond->wq, &bond->ad_work, ad_delta_in_ticks);
 }
 
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 3c3fcce4acd4..db8f2fb613df 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1748,11 +1748,19 @@ static void bond_netdev_notify_work(struct work_struct *_work)
 					   notify_work.work);
 
 	if (rtnl_trylock()) {
-		struct netdev_bonding_info binfo;
+		if (slave->should_notify_link) {
+			struct netdev_bonding_info binfo;
+			bond_fill_ifslave(slave, &binfo.slave);
+			bond_fill_ifbond(slave->bond, &binfo.master);
+			netdev_bonding_info_change(slave->dev, &binfo);
+			slave->should_notify_link = 0;
+		}
+
+		if (slave->should_notify_lacp) {
+			rtmsg_ifinfo(RTM_NEWLINK, slave->dev, 0, GFP_KERNEL, 0, NULL);
+			slave->should_notify_lacp = 0;
+		}
 
-		bond_fill_ifslave(slave, &binfo.slave);
-		bond_fill_ifbond(slave->bond, &binfo.master);
-		netdev_bonding_info_change(slave->dev, &binfo);
 		rtnl_unlock();
 	} else {
 		queue_delayed_work(slave->bond->wq, &slave->notify_work, 1);
diff --git a/include/net/bonding.h b/include/net/bonding.h
index b61fb1aa3a56..38d37ea2382c 100644
--- a/include/net/bonding.h
+++ b/include/net/bonding.h
@@ -170,7 +170,8 @@ struct slave {
 	       inactive:1, /* indicates inactive slave */
 	       rx_disabled:1, /* indicates whether slave's Rx is disabled */
 	       should_notify:1, /* indicates whether the state changed */
-	       should_notify_link:1; /* indicates whether the link changed */
+	       should_notify_link:1, /* indicates whether the link changed */
+	       should_notify_lacp:1; /* indicates whether the lacp state changed */
 	u8     duplex;
 	u32    original_mtu;
 	u32    link_failure_count;
@@ -641,11 +642,10 @@ static inline void bond_slave_link_notify(struct bonding *bond)
 	struct slave *tmp;
 
 	bond_for_each_slave(bond, tmp, iter) {
-		if (tmp->should_notify_link) {
+		if (tmp->should_notify_link || tmp->should_notify_lacp)
 			bond_queue_slave_event(tmp);
+		if (tmp->should_notify_link)
 			bond_lower_state_changed(tmp);
-			tmp->should_notify_link = 0;
-		}
 	}
 }
 
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index eabfc8290f5e..4507bb8d5264 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -4116,6 +4116,7 @@ void rtmsg_ifinfo(int type, struct net_device *dev, unsigned int change,
 	rtmsg_ifinfo_event(type, dev, change, rtnl_get_event(0), flags,
 			   NULL, 0, portid, nlh);
 }
+EXPORT_SYMBOL(rtmsg_ifinfo);
 
 void rtmsg_ifinfo_newnet(int type, struct net_device *dev, unsigned int change,
 			 gfp_t flags, int *new_nsid, int new_ifindex)
-- 
2.45.0

Thanks
Hangbin