Message-ID: <9634a1e1-6cc4-45ef-89d8-30d0e50ba319@openvpn.net>
Date: Mon, 6 Jan 2025 00:27:28 +0100
From: Antonio Quartulli <antonio@...nvpn.net>
To: Sabrina Dubroca <sd@...asysnail.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

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.

Powered by blists - more mailing lists