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.
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?
I'm trying to poke holes into this idea now. close() vs attach worries me a bit.
Can that truly happen? 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 (sock->sock->sk->sk_protocol == IPPROTO_UDP) ovpn_udp_socket_detach(sock->sock);
- bh_unlock_sock(sock->sock->sk);
- sockfd_put(sock->sock);
- kfree_rcu(sock, rcu);
+}
[...]
+struct ovpn_socket *ovpn_socket_new(struct socket *sock, struct ovpn_peer *peer) +{
- struct ovpn_socket *ovpn_sock;
- int ret;
- lock_sock(sock->sk);
- ret = ovpn_socket_attach(sock, peer);
- if (ret < 0 && ret != -EALREADY)
goto err_release;- /* if this socket is already owned by this interface, just increase the
* refcounter and use it as expected.** Since UDP sockets can be used to talk to multiple remote endpoints,* openvpn normally instantiates only one socket and shares it among all* its peers. For this reason, when we find out that a socket is already* used for some other peer in *this* instance, we can happily increase* its refcounter and use it normally.*/- if (ret == -EALREADY) {
/* caller is expected to increase the sock refcounter before* passing it to this function. For this reason we drop it if* not needed, like when this socket is already owned.*/ovpn_sock = ovpn_socket_get(sock);release_sock(sock->sk);sockfd_put(sock);return ovpn_sock;- }
- ovpn_sock = kzalloc(sizeof(*ovpn_sock), GFP_KERNEL);
- if (!ovpn_sock) {
ret = -ENOMEM;goto err_detach;- }
- ovpn_sock->ovpn = peer->ovpn;
- ovpn_sock->sock = sock;
- kref_init(&ovpn_sock->refcount);
- rcu_assign_sk_user_data(sock->sk, ovpn_sock);
- release_sock(sock->sk);
- return ovpn_sock;
+err_detach:
- if (sock->sk->sk_protocol == IPPROTO_UDP)
ovpn_udp_socket_detach(sock);This would leave the TCP socket half-attached, and if userspace tries to attach the same socket again (I don't think the ovpn module would prevent that since sk_user_data is still unset), both ->sk_data_ready and tcp.sk_cb.sk_data_ready will be set to ovpn's (same for sk_write_space with ovpn_tcp_write_space which will recurse into itself when called).
I think it'd be easier to do the alloc first, then attach. Handling a failure to attach would be a simple kfree, while handling a failure to alloc is a detach (or part of a detach) which is not as easy.
Yap, makes sense!
+int ovpn_udp_socket_attach(struct socket *sock, struct ovpn_priv *ovpn) +{
- struct ovpn_socket *old_data;
- int ret = 0;
- /* make sure no pre-existing encapsulation handler exists */
- rcu_read_lock();
- old_data = rcu_dereference_sk_user_data(sock->sk);
- if (!old_data) {
/* socket is currently unused - we can take it */rcu_read_unlock();return 0;- }
- /* socket is in use. We need to understand if it's owned by this ovpn
* instance or by something else.* In the former case, we can increase the refcounter and happily* use it, because the same UDP socket is expected to be shared among* different peers.** Unlikely TCP, a single UDP socket can be used to talk to many remotenit: s/Unlikely/Unlike/
ACK
* hosts and therefore openvpn instantiates one only for all its peers*/
Thanks a lot!
Regards,