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,