Hi Sabrina,

On 03/01/2025 18:00, Sabrina Dubroca wrote:

Hello Antonio,

2024-12-19, 02:42:01 +0100, Antonio Quartulli wrote:

quoted

+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]

quoted

	/* 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?

quoted

	 */
	if (sock->sock->sk->sk_protocol == IPPROTO_UDP)
		ovpn_udp_socket_detach(sock->sock);

quoted

+	bh_unlock_sock(sock->sock->sk);
+	sockfd_put(sock->sock);
+	kfree_rcu(sock, rcu);
+}

[...]

quoted

+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!

quoted

+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

quoted

+	 * hosts and therefore openvpn instantiates one only for all its peers
+	 */

Thanks a lot!

Regards,

-- 
Antonio Quartulli
OpenVPN Inc.