diff mbox series

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

Message ID 20240718122017.b1051af4e7f7.I68eb9c0f02545b364b79a59f2110f2cf5682a8e2@changeid (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [RFC,1/2] net: bonding: correctly annotate RCU in bond_should_notify_peers() | expand

Checks

Context Check Description
netdev/series_format warning Single patches do not need cover letters; Target tree name not specified in the subject
netdev/tree_selection success Guessed tree name to be net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 661 this patch: 661
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers warning 4 maintainers not CCed: andy@greyhouse.net pabeni@redhat.com kuba@kernel.org j.vosburgh@gmail.com
netdev/build_clang success Errors and warnings before: 662 this patch: 662
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 662 this patch: 662
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 15 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 13 this patch: 13
netdev/source_inline success Was 0 now: 0

Commit Message

Johannes Berg July 18, 2024, 7:20 p.m. UTC 
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(-)

Comments

Jiri Pirko July 19, 2024, 9:42 a.m. UTC | #1 
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>
Johannes Berg July 19, 2024, 4:31 p.m. UTC | #2 
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
Jiri Pirko July 23, 2024, 2:22 p.m. UTC | #3 
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
Johannes Berg July 23, 2024, 2:27 p.m. UTC | #4 
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
diff mbox series

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 %