[RFC PATCH 1/2] net: bonding: correctly annotate RCU in bond_should_notify_peers()

[RFC PATCH 1/2] net: bonding: correctly annotate RCU in bond_should_notify_peers()

[Johannes Berg]

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

RCU use in bond_should_notify_peers() looks wrong, since it does
rcu_dereference(), leaves the critical section, and uses the
pointer after that.

Luckily, it's called either inside a nested RCU critical section
or with the RTNL held.

Annotate it with rcu_dereference_rtnl() instead, and remove the
inner RCU critical section.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 drivers/net/bonding/bond_main.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index d19aabf5d4fb..2ed0da068490 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1121,13 +1121,10 @@ static struct slave *bond_find_best_slave(struct bonding *bond)
 	return bestslave;
 }
 
+/* must be called in RCU critical section or with RTNL held */
 static bool bond_should_notify_peers(struct bonding *bond)
 {
-	struct slave *slave;
-
-	rcu_read_lock();
-	slave = rcu_dereference(bond->curr_active_slave);
-	rcu_read_unlock();
+	struct slave *slave = rcu_dereference_rtnl(bond->curr_active_slave);
 
 	if (!slave || !bond->send_peer_notif ||
 	    bond->send_peer_notif %
-- 
2.45.2

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

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.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@syzkaller.appspotmail.com
Signed-off-by: Johannes Berg <johannes.berg@intel.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

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

[Jay Vosburgh]

Johannes Berg <johannes@sipsolutions.net> wrote:

>From: Johannes Berg <johannes.berg@intel.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.

	We can't do this, as it will hit RTNL every monitor interval,
which can be many times per second.  The logic is structured to
specifically avoid acquiring RTNL during the inspection pass.

	The issue that szybot is seeing only happens if bonding's
use_carrier option is set to 0, which is not the normal case.
use_carrier is a backwards compatibility option from years ago for
drivers that do not implement netif_carrier_on/off (and thus calling
netif_carrier_ok() would be unreliable).

	This also came up in [0], and looking now I see there's a patch
that syzbot tested, although I haven't reviewed it.

	Another option is to for the Powers That Be to declare that it's
safe to assume that network drivers implement netif_carrier_on/off to
advertise their link state, in which case the use_carrier logic in
bonding can be removed.

	Or we can somehow isolate the "must acquire RTNL instead of RCU"
to the problematic use_carrier=0 path, but that's a nontrivial change.

	-J

[0] https://lore.kernel.org/lkml/000000000000eb54bf061cfd666a@google.com/

>Reported-by: syzbot+2120b9a8f96b3fa90bad@syzkaller.appspotmail.com
>Signed-off-by: Johannes Berg <johannes.berg@intel.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
>
>

---
	-Jay Vosburgh, jv@jvosburgh.net

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

[Johannes Berg]

On Thu, 2024-07-18 at 14:12 -0700, Jay Vosburgh wrote:
> 
> 	We can't do this, as it will hit RTNL every monitor interval,
> which can be many times per second.

Fair.

>   The logic is structured to
> specifically avoid acquiring RTNL during the inspection pass.

We also cannot do _that_, however, it's just broken with devices that
want to sleep there. Arguably the common ethtool op that syzbot
complains about is just a distraction, because while that does sleep
(now), it's also equivalent to use_carrier==1, so we could just say "oh
if it's the common ethtool op then use the carrier directly", but while
that'd prevent syzbot from reporting the issue again, it'd not actually
fix the problem with all the USB drivers etc.

> 	The issue that szybot is seeing only happens if bonding's
> use_carrier option is set to 0, which is not the normal case.

Sure, but like I said above, syzbot doesn't really matter. Don't get too
hung up on it.

> use_carrier is a backwards compatibility option from years ago for
> drivers that do not implement netif_carrier_on/off (and thus calling
> netif_carrier_ok() would be unreliable).

Sure, OK.

> 	This also came up in [0], and looking now I see there's a patch
> that syzbot tested, although I haven't reviewed it.

+Hillf, I believe that patch is broken because it completely defeats the
purpose of my original patch there, and also addresses only the "syzbot
complains about common ethtool op" issue, not the more general problem.

> 	Another option is to for the Powers That Be to declare that it's
> safe to assume that network drivers implement netif_carrier_on/off to
> advertise their link state, in which case the use_carrier logic in
> bonding can be removed.

No objection to that, if you don't have proper carrier reporting then a
lot of other things will likely be broken anyway?

> 	Or we can somehow isolate the "must acquire RTNL instead of RCU"
> to the problematic use_carrier=0 path, but that's a nontrivial change.
> 

I guess.

johannes

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

[Jiri Pirko]

Thu, Jul 18, 2024 at 09:20:17PM CEST, johannes@sipsolutions.net wrote:
>From: Johannes Berg <johannes.berg@intel.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.

Is there a good reason why to directly query device here using whatever?
I mean, why netif_oper_up() would not return the correct bool here?
Introduction of periodic rtnl locking for no good reason is not probably
something we should do :/

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

[Johannes Berg]

On Fri, 2024-07-19 at 11:50 +0200, Jiri Pirko wrote:
> Thu, Jul 18, 2024 at 09:20:17PM CEST, johannes@sipsolutions.net wrote:
> > From: Johannes Berg <johannes.berg@intel.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.
> 
> Is there a good reason why to directly query device here using whatever?

See Jay's email for that, I think?

> I mean, why netif_oper_up() would not return the correct bool here?
> Introduction of periodic rtnl locking for no good reason is not probably
> something we should do :/
> 

Yeah, fair.

johannes

Re: [RFC PATCH 1/2] net: bonding: correctly annotate RCU in bond_should_notify_peers()

[Jiri Pirko]

Thu, Jul 18, 2024 at 09:20:16PM CEST, johannes@sipsolutions.net wrote:
>From: Johannes Berg <johannes.berg@intel.com>
>
>RCU use in bond_should_notify_peers() looks wrong, since it does
>rcu_dereference(), leaves the critical section, and uses the
>pointer after that.
>
>Luckily, it's called either inside a nested RCU critical section
>or with the RTNL held.
>
>Annotate it with rcu_dereference_rtnl() instead, and remove the
>inner RCU critical section.
>
>Signed-off-by: Johannes Berg <johannes.berg@intel.com>

Fixes 4cb4f97b7e361745281e843499ba58691112d2f8 perhaps?

Patch looks okay.

Reviewed-by: Jiri Pirko <jiri@nvidia.com>

Re: [RFC PATCH 1/2] net: bonding: correctly annotate RCU in bond_should_notify_peers()

[Johannes Berg]

On Fri, 2024-07-19 at 11:42 +0200, Jiri Pirko wrote:
> Thu, Jul 18, 2024 at 09:20:16PM CEST, johannes@sipsolutions.net wrote:
> > From: Johannes Berg <johannes.berg@intel.com>
> > 
> > RCU use in bond_should_notify_peers() looks wrong, since it does
> > rcu_dereference(), leaves the critical section, and uses the
> > pointer after that.
> > 
> > Luckily, it's called either inside a nested RCU critical section
> > or with the RTNL held.
> > 
> > Annotate it with rcu_dereference_rtnl() instead, and remove the
> > inner RCU critical section.
> > 
> > Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> 
> Fixes 4cb4f97b7e361745281e843499ba58691112d2f8 perhaps?
> 

I don't really want to get into that discussion again :)

Thanks for looking!

johannes

Re: [RFC PATCH 1/2] net: bonding: correctly annotate RCU in bond_should_notify_peers()

[Jiri Pirko]

Fri, Jul 19, 2024 at 06:31:18PM CEST, johannes@sipsolutions.net wrote:
>On Fri, 2024-07-19 at 11:42 +0200, Jiri Pirko wrote:
>> Thu, Jul 18, 2024 at 09:20:16PM CEST, johannes@sipsolutions.net wrote:
>> > From: Johannes Berg <johannes.berg@intel.com>
>> > 
>> > RCU use in bond_should_notify_peers() looks wrong, since it does
>> > rcu_dereference(), leaves the critical section, and uses the
>> > pointer after that.
>> > 
>> > Luckily, it's called either inside a nested RCU critical section
>> > or with the RTNL held.
>> > 
>> > Annotate it with rcu_dereference_rtnl() instead, and remove the
>> > inner RCU critical section.
>> > 
>> > Signed-off-by: Johannes Berg <johannes.berg@intel.com>
>> 
>> Fixes 4cb4f97b7e361745281e843499ba58691112d2f8 perhaps?
>> 
>
>I don't really want to get into that discussion again :)

Which one? I have to be missing something...


>
>Thanks for looking!
>
>johannes

Re: [RFC PATCH 1/2] net: bonding: correctly annotate RCU in bond_should_notify_peers()

[Johannes Berg]

On Tue, 2024-07-23 at 16:22 +0200, Jiri Pirko wrote:
> Fri, Jul 19, 2024 at 06:31:18PM CEST, johannes@sipsolutions.net wrote:
> > On Fri, 2024-07-19 at 11:42 +0200, Jiri Pirko wrote:
> > > Thu, Jul 18, 2024 at 09:20:16PM CEST, johannes@sipsolutions.net wrote:
> > > > From: Johannes Berg <johannes.berg@intel.com>
> > > > 
> > > > RCU use in bond_should_notify_peers() looks wrong, since it does
> > > > rcu_dereference(), leaves the critical section, and uses the
> > > > pointer after that.
> > > > 
> > > > Luckily, it's called either inside a nested RCU critical section
> > > > or with the RTNL held.
> > > > 
> > > > Annotate it with rcu_dereference_rtnl() instead, and remove the
> > > > inner RCU critical section.
> > > > 
> > > > Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> > > 
> > > Fixes 4cb4f97b7e361745281e843499ba58691112d2f8 perhaps?
> > > 
> > 
> > I don't really want to get into that discussion again :)
> 
> Which one? I have to be missing something...
> 

The one that we like to repeat all the time about whether a Fixes tag
should be included or not, like in 

https://lore.kernel.org/netdev/20240705134221.2f4de205caa1.I28496dc0f2ced580282d1fb892048017c4491e21@changeid/

johannes