2025-01-06, 00:27:28 +0100, 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?
TCP detach can sleep, and UDP attach as well (setup_udp_tunnel_sock -> udp_tunnel_encap_enable -> udp_encap_enable -> static_branch_inc -> static_key_slow_inc -> cpus_read_lock). UDP detach would also not work under bh_lock_sock if it really disabled encap on the socket (we end up in udp_tunnel_encap_enable but that doesn't do anything since encap is already turned on -- but a "real" detach should disable the encap and do static_branch_dec).
So attach/detach need to be under lock_sock, not 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.
Rough idea:
- ovpn_socket_new is pretty much unchanged (locking still needed to protect against another simultaneous attach attempt, EALREADY case becomes a bit easier)
- ovpn_peer_remove doesn't do anything socket-related
- use ->encap_destroy/ovpn_tcp_close() to clean up sk_user_data
- no more refcounting on ovpn_socket (since the encap can't be removed, the lifetime to ovpn_socket is tied to its socket)
What do you think?
hmm how would that work with UDP? On a server all clients may disconnect, but the UDP socket is expected to still survive and be re-used for new clients (userspace will keep it alive and keep listening for new clients).
Or you're saying that the socket will remain "attached" (i.e. sk_user_data set to the ovpn_priv*) even when no more clients are connected?
Yes. Once attached, it stays attached.
I'm trying to poke holes into this idea now. close() vs attach worries me a bit.
Can that truly happen?
Actually it can't, so this isn't a concern.
If a socket is going through close(), there should be some way to mark it as "non-attachable".
Actually, do we even need to clean up sk_user_data? The socket is being destroyed - why clean that up at all?
If we allocated some memory to store per-socket info, we need to free it when we detach or close. There's no generic mechanism to free sk_user_data since the core can't know where it came from, maybe kfree() isn't appropriate.