[lkml]   [2022]   [Aug]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH] net: Fix suspicious RCU usage in bpf_sk_reuseport_detach()
On Tue, Aug 16, 2022 at 04:44:35PM -0700, Jakub Kicinski wrote:
> On Tue, 16 Aug 2022 22:16:46 +0100 David Howells wrote:
> > So either __rcu_dereference_sk_user_data_with_flags_check() has to be a macro,
> > or we need to go with something like the first version of my patch where I
> > don't pass the condition through. Do you have a preference?
> I like your version because it documents what the lock protecting this
> field is.
> In fact should we also add && sock_owned_by_user(). Martin, WDYT? Would
> that work for reuseport? Jakub S is fixing l2tp to hold the socket lock
> while setting this field, yet most places take the callback lock...
It needs to take a closer look at where the lock_sock() has already
been acquired and also need to consider the lock ordering with reuseport_lock.
It probably should work but may need a separate patch to discuss those
considerations ?

> One the naming - maybe just drop the _with_flags() ? There's no version
> of locked helper which does not take the flags. And not underscores?
I am also good with a shorter name.

Could a comment be added to bpf_sk_reuseport_detach() mentioning
sk_user_data access is protected by the sk_callback_lock alone (or the lock
sock in the future) while reusing __locked_read_sk_user_data() with
a rcu_dereference(). It will be easier to understand if there is
actually any rcu reader in the future code reading.

 \ /
  Last update: 2022-08-17 02:45    [W:0.107 / U:2.092 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site