Re: [PATCH net-next v16 07/26] ovpn: introduce the ovpn_socket object

From: Antonio Quartulli
Date: Sun Jan 05 2025 - 18:27:00 EST

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.