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 remote
nit: s/Unlikely/Unlike/
ACK
+ * hosts and therefore openvpn instantiates one only for all its peers
+ */
Thanks a lot!
Regards,
--
Antonio Quartulli
OpenVPN Inc.
