[net-next] net: bonding: correctly annotate RCU in bond_should_notify_peers()

Commit Message

Johannes Berg July 19, 2024, 4:41 p.m. UTC

From: Johannes Berg <[e-mail]>

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.

Fixes: 4cb4f97b7e36 ("bonding: rebuild the lock use for bond_mii_monitor()")
Reviewed-by: Jiri Pirko <[e-mail]>
Signed-off-by: Johannes Berg <[e-mail]>
---
 drivers/net/bonding/bond_main.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

Comments

Jay Vosburgh July 21, 2024, 5:09 p.m. UTC | #1

Johannes Berg <[e-mail]> wrote:

>From: Johannes Berg <[e-mail]>
>
>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.
>
>Fixes: 4cb4f97b7e36 ("bonding: rebuild the lock use for bond_mii_monitor()")
>Reviewed-by: Jiri Pirko <[e-mail]>
>Signed-off-by: Johannes Berg <[e-mail]>

Acked-by: Jay Vosburgh <[e-mail]>

>---
> 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
>

Paolo Abeni July 23, 2024, 10:25 a.m. UTC | #2

On 7/19/24 18:41, Johannes Berg wrote:
> From: Johannes Berg <[e-mail]>
> 
> 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.
> 
> Fixes: 4cb4f97b7e36 ("bonding: rebuild the lock use for bond_mii_monitor()")
> Reviewed-by: Jiri Pirko <[e-mail]>
> Signed-off-by: Johannes Berg <[e-mail]>

Any special reasons to target net-next? this looks like a legit net fix 
to me. If you want to target net, no need to re-post, otherwise it will 
have to wait the merge window end.

Thanks,

Paolo

Johannes Berg July 23, 2024, 11:30 a.m. UTC | #3

On Tue, 2024-07-23 at 12:25 +0200, Paolo Abeni wrote:
> 
> On 7/19/24 18:41, Johannes Berg wrote:
> > From: Johannes Berg <[e-mail]>
> > 
> > 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.
> > 
> > Fixes: 4cb4f97b7e36 ("bonding: rebuild the lock use for bond_mii_monitor()")
> > Reviewed-by: Jiri Pirko <[e-mail]>
> > Signed-off-by: Johannes Berg <[e-mail]>
> 
> Any special reasons to target net-next? this looks like a legit net fix 
> to me. If you want to target net, no need to re-post, otherwise it will 
> have to wait the merge window end.

Well, I guess it's kind of a fix, but functionally all it really does is
remove the RCU critical section which isn't necessary because either we
hold the lock or there's already one around it. So locally the function
_looks_ wrong (using the pointer outside the section it uses to deref
it), but because of other reasons in how the function is used, it's not
really wrong.

I'd really prefer not to have to resend it though ;-)

johannes

Paolo Abeni July 23, 2024, 1:12 p.m. UTC | #4

On 7/23/24 13:30, Johannes Berg wrote:
> On Tue, 2024-07-23 at 12:25 +0200, Paolo Abeni wrote:
>>
>> On 7/19/24 18:41, Johannes Berg wrote:
>>> From: Johannes Berg <[e-mail]>
>>>
>>> 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.
>>>
>>> Fixes: 4cb4f97b7e36 ("bonding: rebuild the lock use for bond_mii_monitor()")
>>> Reviewed-by: Jiri Pirko <[e-mail]>
>>> Signed-off-by: Johannes Berg <[e-mail]>
>>
>> Any special reasons to target net-next? this looks like a legit net fix
>> to me. If you want to target net, no need to re-post, otherwise it will
>> have to wait the merge window end.
> 
> Well, I guess it's kind of a fix, but functionally all it really does is
> remove the RCU critical section which isn't necessary because either we
> hold the lock or there's already one around it. So locally the function
> _looks_ wrong (using the pointer outside the section it uses to deref
> it), but because of other reasons in how the function is used, it's not
> really wrong.

I think we are better off with this applied on net. No need to resend.

Thanks,

Paolo

[e-mail] July 23, 2024, 1:20 p.m. UTC | #5

Hello:

This patch was applied to netdev/net.git (main)
by Paolo Abeni <[e-mail]>:

On Fri, 19 Jul 2024 09:41:18 -0700 you wrote:
> From: Johannes Berg <[e-mail]>
> 
> 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.
> 
> [...]

Here is the summary with links:
  - [net-next] net: bonding: correctly annotate RCU in bond_should_notify_peers()
    https://git.kernel.org/netdev/net/c/3ba359c0cd6e

You are awesome, thank you!

Patch

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 %