Re: [PATCH net-next v11 17/23] ovpn: add support for peer floating

[Antonio Quartulli <antonio@openvpn.net>]

On 12/11/2024 11:56, Sabrina Dubroca wrote:
> 2024-10-29, 11:47:30 +0100, Antonio Quartulli wrote:
>> diff --git a/drivers/net/ovpn/io.c b/drivers/net/ovpn/io.c
>> index 63c140138bf98e5d1df79a2565b666d86513323d..0e8a6f2c76bc7b2ccc287ad1187cf50f033bf261 100644
>> --- a/drivers/net/ovpn/io.c
>> +++ b/drivers/net/ovpn/io.c
>> @@ -135,6 +135,15 @@ void ovpn_decrypt_post(void *data, int ret)
>>   	/* keep track of last received authenticated packet for keepalive */
>>   	peer->last_recv = ktime_get_real_seconds();
>>   
>> +	if (peer->sock->sock->sk->sk_protocol == IPPROTO_UDP) {
> 
> What prevents peer->sock from being replaced and released
> concurrently?

Technically nothing.
Userspace currently does not even support updating a peer socket at 
runtime, but I wanted ovpn to be flexible enough from the beginning.

One approach might be to go back to peer->sock being unmutable and 
forget about this.

OTOH, if we want to keep this flexibility (which I think is nice), I 
think I should make peer->sock an RCU pointer and access it accordingly.
Does it make sense?

> 
> Or possibly reading the error value that ovpn_socket_new can return
> before peer->sock is reset to NULL, just noticed this in
> ovpn_nl_peer_modify:
> 
> 	if (attrs[OVPN_A_PEER_SOCKET]) {
> 		// ...
> 		peer->sock = ovpn_socket_new(sock, peer);
> 		if (IS_ERR(peer->sock)) {
> 			// ...
> 			peer->sock = NULL;
> 
> 
> (ovpn_encrypt_post has a similar check on
> peer->sock->sock->sk->sk_protocol that I don't think is safe either)

Yap, agreed.

> 
> 
>> +		/* check if this peer changed it's IP address and update
>> +		 * state
>> +		 */
>> +		ovpn_peer_float(peer, skb);
>> +		/* update source endpoint for this peer */
>> +		ovpn_peer_update_local_endpoint(peer, skb);
> 
> Why not do both in the same function? They're not called anywhere else
> (at least in this version of the series). They both modify peer->bind
> depending on skb_protocol_to_family(skb), and operate under
> peer->lock.

I never considered to do so as I just always assumed the two to be two 
separate features/routines.

I think it's a good idea and I would get rid of a few common 
instructions (along with acquiring the lock twice). Thanks!

> 
> 
>> +void ovpn_peer_float(struct ovpn_peer *peer, struct sk_buff *skb)
>> +{
>> +	struct hlist_nulls_head *nhead;
>> +	struct sockaddr_storage ss;
>> +	const u8 *local_ip = NULL;
>> +	struct sockaddr_in6 *sa6;
>> +	struct sockaddr_in *sa;
>> +	struct ovpn_bind *bind;
>> +	sa_family_t family;
>> +	size_t salen;
>> +
>> +	rcu_read_lock();
>> +	bind = rcu_dereference(peer->bind);
>> +	if (unlikely(!bind)) {
>> +		rcu_read_unlock();
>> +		return;
>> +	}
>> +
>> +	spin_lock_bh(&peer->lock);
> 
> You could take the lock from the start, instead of using rcu_read_lock
> to get peer->bind. It would guarantee that the bind we got isn't
> already being replaced just as we wait to update it. And same in
> ovpn_peer_update_local_endpoint, it would make sure we're updating the
> local IP for the active bind.
> 
> (sorry I didn't think about that last time we discussed this)

no worries :) and I like the idea. will do that, thanks.

> 
>> +	if (likely(ovpn_bind_skb_src_match(bind, skb)))
>> +		goto unlock;
>> +
>> +	family = skb_protocol_to_family(skb);
>> +
> 

-- 
Antonio Quartulli
OpenVPN Inc.