Message-ID: <Z3gXs65jjYc-g2iw@hog>
Date: Fri, 3 Jan 2025 18:00:35 +0100
From: Sabrina Dubroca <sd@...asysnail.net>
To: Antonio Quartulli <antonio@...nvpn.net>
Cc: netdev@...r.kernel.org, Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Donald Hunter <donald.hunter@...il.com>,
	Shuah Khan <shuah@...nel.org>, ryazanov.s.a@...il.com,
	Andrew Lunn <andrew+netdev@...n.ch>,
	Simon Horman <horms@...nel.org>, linux-kernel@...r.kernel.org,
	linux-kselftest@...r.kernel.org, Xiao Liang <shaw.leon@...il.com>,
	willemdebruijn.kernel@...il.com
Subject: Re: [PATCH net-next v16 07/26] ovpn: introduce the ovpn_socket object

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).

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).

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?

I'm trying to poke holes into this idea now. close() vs attach worries
me a bit.


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



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

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

-- 
Sabrina

Powered by blists - more mailing lists