Re: [PATCH net-next v15 06/22] ovpn: introduce the ovpn_socket object

[Antonio Quartulli <antonio@openvpn.net>]

On 12/12/2024 17:19, Sabrina Dubroca wrote:
> 2024-12-11, 22:15:10 +0100, Antonio Quartulli wrote:
>> +static struct ovpn_socket *ovpn_socket_get(struct socket *sock)
>> +{
>> +	struct ovpn_socket *ovpn_sock;
>> +
>> +	rcu_read_lock();
>> +	ovpn_sock = rcu_dereference_sk_user_data(sock->sk);
>> +	if (WARN_ON(!ovpn_socket_hold(ovpn_sock)))
> 
> Could we hit this situation when we're removing the last peer (so
> detaching its socket) just as we're adding a new one? ovpn_socket_new
> finds the socket already attached and goes through the EALREADY path,
> but the refcount has already dropped to 0?
> 

hm good point.

> Then we'd also return NULL from ovpn_socket_new [1], which I don't
> think is handled well by the caller (at least the netdev_dbg call at
> the end of ovpn_nl_peer_modify, maybe other spots too).
> 
> (I guess it's not an issue you would see with the existing userspace
> if it's single-threaded)

The TCP patch 11/22 will convert the socket release routine to a 
scheduled worker.

This means we can have the following flow:
1) userspace deletes a peer -> peer drops its reference to the ovpn_socket
2) ovpn_socket refcnt may hit 0 -> cleanup/detach work is scheduled, but 
not yet executed
3) userspace adds a new peer -> attach returns -EALREADY but refcnt is 0

So not so impossible, even with a single-threaded userspace software.

> 
> [...]
>> +struct ovpn_socket *ovpn_socket_new(struct socket *sock, struct ovpn_peer *peer)
>> +{
>> +	struct ovpn_socket *ovpn_sock;
>> +	int ret;
>> +
>> +	ret = ovpn_socket_attach(sock, peer);
>> +	if (ret < 0 && ret != -EALREADY)
>> +		return ERR_PTR(ret);
>> +
>> +	/* 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);
>> +		sockfd_put(sock);
> 
> [1] so we would need to add
> 
>      if (!ovpn_sock)
>          return -EAGAIN;

I am not sure returning -EAGAIN is the right move at this point.
We don't know when the scheduled worker will execute, so we don't know 
when to try again.

Maybe we should call cancel_sync_work(&ovpn_sock->work) inside 
ovpn_socket_get()?
So the latter will return NULL only when it is sure that the socket has 
been detached.

At that point we can skip the following return and continue along the 
"new socket" path.

What do you think?

However, this makes we wonder: what happens if we have two racing 
PEER_NEW with the same non-yet-attached UDP socket?

Maybe we should lock the socket in ovpn_udp_socket_attach() when 
checking its user-data and setting it (in order to make the test-and-set 
atomic)?

I am specifically talking about this in udp.c:

345         /* make sure no pre-existing encapsulation handler exists */
346         rcu_read_lock();
347         old_data = rcu_dereference_sk_user_data(sock->sk);
348         if (!old_data) {
349                 /* socket is currently unused - we can take it */
350                 rcu_read_unlock();
351                 setup_udp_tunnel_sock(sock_net(sock->sk), sock, &cfg);
352                 return 0;
353         }

We will end up returning 0 in both contexts and thus allocate two 
ovpn_sockets instead of re-using the first one we allocated.

Does it make sense?

> 
>> +		return ovpn_sock;
>> +	}
>> +
> 
> [...]
>> +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
> 
> (since I'm commenting on this patch:)
> 
> s/Unlikely/Unlike/

ACK

> 
> [I have some more nits/typos here and there but I worry the
> maintainers will get "slightly" annoyed if I make you repost 22
> patches once again :) -- if that's all I find in the next few days,
> everyone might be happier if I stash them and we get them fixed after
> merging?]

If we have to rework this socket attaching part, it may be worth 
throwing in those typ0 fixes too :)

Thanks a lot.

Regards,


-- 
Antonio Quartulli
OpenVPN Inc.