Re: [PATCH net-next v16 07/26] ovpn: introduce the ovpn_socket object

[Antonio Quartulli <antonio@openvpn.net>]

On 06/01/2025 00:27, Antonio Quartulli wrote:
> Hi Sabrina,
> 
> On 03/01/2025 18:00, Sabrina Dubroca wrote:
>> Hello Antonio,
>>
>> 2024-12-19, 02:42:01 +0100, Antonio Quartulli wrote:
>>> +static void ovpn_socket_release_kref(struct kref *kref)
>>> +    __releases(sock->sock->sk)
>>> +{
>>> +    struct ovpn_socket *sock = container_of(kref, struct ovpn_socket,
>>> +                        refcount);
>>> +
>>
>> [extend with bits of patch 9]
>>>     /* UDP sockets are detached in this kref callback because
>>>      * we now know for sure that all concurrent users have
>>>      * finally gone (refcounter dropped to 0).
>>>      *
>>>      * Moreover, detachment is performed under lock to prevent
>>>      * a concurrent ovpn_socket_new() call with the same socket
>>>      * to find the socket still attached but with refcounter 0.
>>
>> I'm not convinced this really works, because ovpn_socket_new() doesn't
>> use the same lock. lock_sock and bh_lock_sock both "lock the socket"
>> in some sense, but they're not mutually exclusive (we talked about
>> that around the TCP patch).
> 
> You're right - but what prevents us from always using bh_lock_sock?
> 
>>
>> Are you fundamentally opposed to making attach permanent? ie, once
>> a UDP or TCP socket is assigned to an ovpn instance, it can't be
>> detached and reused. I think it would be safer, simpler, and likely
>> sufficient (I don't know openvpn much, but I don't see a use case for
>> moving a socket from one ovpn instance to another, or using it without
>> encap).
> 
> I hardly believe a socket will ever be moved to a different instance.
> There is no use case (and no userspace support) for that at the moment.

Actually, there might be situations where userspace may want to close 
and re-open the device, while keeping the original socket alive.

This means the socket should be able to be attached to a new device 
without being destroyed and re-opened again.

The refcount_dec with bh_lock_sock still seems to be the approach 
covering all cases, no?

Regards,


-- 
Antonio Quartulli
OpenVPN Inc.