Re: [PATCH net-next v15 06/22] ovpn: introduce the ovpn_socket object

[Antonio Quartulli <antonio@openvpn.net>]

On 16/12/2024 12:09, Sabrina Dubroca wrote:
[...]
>> Maybe we should call cancel_sync_work(&ovpn_sock->work) inside
>> ovpn_socket_get()?
>> So the latter will return NULL only when it is sure that the socket has been
>> detached.
>>
>> At that point we can skip the following return and continue along the "new
>> socket" path.
>>
>> What do you think?
> 
> The work may not have been scheduled yet? (small window between the
> last kref_put and schedule_work)
> 
> Maybe a completion [Documentation/scheduler/completion.rst] would
> solve it (but it makes things even more complex, unfortunately):
> 
>   - at the end of ovpn_socket_detach: complete(&ovpn_sock->detached);
>   - in ovpn_socket_new when handling EALREADY: wait_for_completion(&ovpn_sock->detached);
>   - in ovpn_socket_new for the new socket: init_completion(&ovpn_sock->detached);
> 
> but ovpn_sock could be gone immediately after complete(). Maybe
> something with completion_done() before the kfree_rcu in
> ovpn_socket_detach? I'm not that familiar with the completion API.
> 

It seems the solution we are aiming for is more complex than the concept 
of ovpn_socket per se :-D

I'll think a bit more about this..maybe we can avoid entering this 
situation at all..

> 
>> However, this makes we wonder: what happens if we have two racing PEER_NEW
>> with the same non-yet-attached UDP socket?
> 
> mhmm, I remember noticing that, but it seems I never mentioned it in
> my reviews. Sorry.
> 
>> Maybe we should lock the socket in ovpn_udp_socket_attach() when checking
>> its user-data and setting it (in order to make the test-and-set atomic)?
> 
> I'd use the lock to protect all of ovpn_socket_new.
> ovpn_tcp_socket_attach locks the socket but after doing the initial
> checks, so 2 callers could both see sock->sk->sk_user_data == NULL and
> do the full attach. And I don't think unlocking before
> rcu_assign_sk_user_data is safe for either UDP or TCP.

I tend to agree here. Guarding the whole ovpn_socket_new with 
lock_sock() seems the right thing to do.

> 
>> I am specifically talking about this in udp.c:
>>
>> 345         /* make sure no pre-existing encapsulation handler exists */
>> 346         rcu_read_lock();
>> 347         old_data = rcu_dereference_sk_user_data(sock->sk);
>> 348         if (!old_data) {
>> 349                 /* socket is currently unused - we can take it */
>> 350                 rcu_read_unlock();
>> 351                 setup_udp_tunnel_sock(sock_net(sock->sk), sock, &cfg);
>> 352                 return 0;
>> 353         }
>>
>> We will end up returning 0 in both contexts and thus allocate two
>> ovpn_sockets instead of re-using the first one we allocated.
>>
>> Does it make sense?
> 
> Yes.
> 
> [...]
>>> [I have some more nits/typos here and there but I worry the
>>> maintainers will get "slightly" annoyed if I make you repost 22
>>> patches once again :) -- if that's all I find in the next few days,
>>> everyone might be happier if I stash them and we get them fixed after
>>> merging?]
>>
>> If we have to rework this socket attaching part, it may be worth throwing in
>> those typ0 fixes too :)
> 
> ACK, I'll send them out.

Thanks.

Regards,


-- 
Antonio Quartulli
OpenVPN Inc.