netdev - Re: [PATCH net-next] net: bonding: correctly annotate RCU in bond_should_notify

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]

Message-ID: <4286c2af-2262-4894-b745-71e18e56b498@redhat.com>
Date: Tue, 23 Jul 2024 15:12:29 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Johannes Berg <johannes@...solutions.net>, netdev@...r.kernel.org
Cc: Jay Vosburgh <j.vosburgh@...il.com>, Andy Gospodarek
 <andy@...yhouse.net>, Jiri Pirko <jiri@...dia.com>
Subject: Re: [PATCH net-next] net: bonding: correctly annotate RCU in
 bond_should_notify_peers()

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 <johannes.berg@...el.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.
>>>
>>> Fixes: 4cb4f97b7e36 ("bonding: rebuild the lock use for bond_mii_monitor()")
>>> Reviewed-by: Jiri Pirko <jiri@...dia.com>
>>> Signed-off-by: Johannes Berg <johannes.berg@...el.com>
>>
>> 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