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.
