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

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] [day] [month] [year] [list]

Message-ID: <3a1349ac-acc2-4db5-a8e7-749cd471d3bc@openvpn.net>
Date: Wed, 8 Jan 2025 11:55:21 +0100
From: Antonio Quartulli <antonio@...nvpn.net>
To: Sabrina Dubroca <sd@...asysnail.net>
Cc: netdev@...r.kernel.org, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Donald Hunter <donald.hunter@...il.com>, Shuah Khan <shuah@...nel.org>,
 ryazanov.s.a@...il.com, Andrew Lunn <andrew+netdev@...n.ch>,
 Simon Horman <horms@...nel.org>, linux-kernel@...r.kernel.org,
 linux-kselftest@...r.kernel.org, Xiao Liang <shaw.leon@...il.com>,
 willemdebruijn.kernel@...il.com
Subject: Re: [PATCH net-next v16 07/26] ovpn: introduce the ovpn_socket object

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.