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.
