Message-ID: <9c919407-fb91-48d7-bf2d-8437c2f3f4da@openvpn.net>
Date: Wed, 5 Mar 2025 00:19:32 +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>
Subject: Re: [PATCH v21 18/24] ovpn: add support for peer floating

On 04/03/2025 19:37, Sabrina Dubroca wrote:
> 2025-03-04, 01:33:48 +0100, Antonio Quartulli wrote:
>> A peer connected via UDP may change its IP address without reconnecting
>> (float).
> 
> Should that trigger a reset of the peer->dst_cache? And same when
> userspace updates the remote address? Otherwise it seems we could be
> stuck with a cached dst that cannot reach the peer.

Yeah, that make sense, otherwise ovpn_udpX_output would just try over 
and over to re-use the cached source address (unless it becomes 
unavailable).

> 
> 
>> +void ovpn_peer_endpoints_update(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;
>> +	size_t salen = 0;
>> +
>> +	spin_lock_bh(&peer->lock);
>> +	bind = rcu_dereference_protected(peer->bind,
>> +					 lockdep_is_held(&peer->lock));
>> +	if (unlikely(!bind))
>> +		goto unlock;
>> +
>> +	switch (skb->protocol) {
>> +	case htons(ETH_P_IP):
>> +		/* float check */
>> +		if (unlikely(!ovpn_bind_skb_src_match(bind, skb))) {
>> +			if (bind->remote.in4.sin_family == AF_INET)
>> +				local_ip = (u8 *)&bind->local;
> 
> If I'm reading this correctly, we always reuse the existing local
> address when we have to re-create the bind, even if it doesn't match
> the skb? The "local endpoint update" chunk below is doing that, but
> only if we're keeping the same remote? It'll get updated the next time
> we receive a packet and call ovpn_peer_endpoints_update.
> 
> That might irritate the RPF check on the other side, if we still use
> our "old" source to talk to the new dest?
> 
>> +			sa = (struct sockaddr_in *)&ss;
>> +			sa->sin_family = AF_INET;
>> +			sa->sin_addr.s_addr = ip_hdr(skb)->saddr;
>> +			sa->sin_port = udp_hdr(skb)->source;
>> +			salen = sizeof(*sa);
>> +			break;

I think the issue is simply this 'break' above - by removing it, 
everything should work as expected.

I thin this is a leftover from when float check and endpoint update were 
two different functions/switch blocks.

>> +		}
>> +
>> +		/* local endpoint update */
>> +		if (unlikely(bind->local.ipv4.s_addr != ip_hdr(skb)->daddr)) {
>> +			net_dbg_ratelimited("%s: learning local IPv4 for peer %d (%pI4 -> %pI4)\n",
>> +					    netdev_name(peer->ovpn->dev),
>> +					    peer->id, &bind->local.ipv4.s_addr,
>> +					    &ip_hdr(skb)->daddr);
>> +			bind->local.ipv4.s_addr = ip_hdr(skb)->daddr;
>> +		}
>> +		break;
> 
> [...]
>> +	if (peer->ovpn->mode == OVPN_MODE_MP) {
>> +		spin_lock_bh(&peer->ovpn->lock);
>> +		spin_lock_bh(&peer->lock);
>> +		bind = rcu_dereference_protected(peer->bind,
>> +						 lockdep_is_held(&peer->lock));
>> +		if (unlikely(!bind)) {
>> +			spin_unlock_bh(&peer->lock);
>> +			spin_unlock_bh(&peer->ovpn->lock);
>> +			return;
>> +		}
>> +
>> +		/* his function may be invoked concurrently, therefore another
> 
> typo:
>                     ^ This

ACK

> 
> 
> [...]
>> -/* variable name __tbl2 needs to be different from __tbl1
>> - * in the macro below to avoid confusing clang
>> - */
>> -#define ovpn_get_hash_slot(_tbl, _key, _key_len) ({	\
>> -	typeof(_tbl) *__tbl2 = &(_tbl);			\
>> -	jhash(_key, _key_len, 0) % HASH_SIZE(*__tbl2);	\
>> -})
>> -
>> -#define ovpn_get_hash_head(_tbl, _key, _key_len) ({		\
>> -	typeof(_tbl) *__tbl1 = &(_tbl);				\
>> -	&(*__tbl1)[ovpn_get_hash_slot(*__tbl1, _key, _key_len)];\
>> -})
>> -
>>   /**
>>    * ovpn_peer_get_by_vpn_addr4 - retrieve peer by its VPN IPv4 address
>>    * @ovpn: the openvpn instance to search
>> @@ -522,51 +694,6 @@ static void ovpn_peer_remove(struct ovpn_peer *peer,
>>   	llist_add(&peer->release_entry, release_list);
>>   }
>>   
>> -/**
>> - * ovpn_peer_update_local_endpoint - update local endpoint for peer
>> - * @peer: peer to update the endpoint for
>> - * @skb: incoming packet to retrieve the destination address (local) from
>> - */
>> -void ovpn_peer_update_local_endpoint(struct ovpn_peer *peer,
>> -				     struct sk_buff *skb)
>> -{
>> -	struct ovpn_bind *bind;
>> -
>> -	rcu_read_lock();
>> -	bind = rcu_dereference(peer->bind);
>> -	if (unlikely(!bind))
>> -		goto unlock;
>> -
>> -	spin_lock_bh(&peer->lock);
>> -	switch (skb->protocol) {
>> -	case htons(ETH_P_IP):
>> -		if (unlikely(bind->local.ipv4.s_addr != ip_hdr(skb)->daddr)) {
>> -			net_dbg_ratelimited("%s: learning local IPv4 for peer %d (%pI4 -> %pI4)\n",
>> -					    netdev_name(peer->ovpn->dev),
>> -					    peer->id, &bind->local.ipv4.s_addr,
>> -					    &ip_hdr(skb)->daddr);
>> -			bind->local.ipv4.s_addr = ip_hdr(skb)->daddr;
>> -		}
>> -		break;
>> -	case htons(ETH_P_IPV6):
>> -		if (unlikely(!ipv6_addr_equal(&bind->local.ipv6,
>> -					      &ipv6_hdr(skb)->daddr))) {
>> -			net_dbg_ratelimited("%s: learning local IPv6 for peer %d (%pI6c -> %pI6c\n",
>> -					    netdev_name(peer->ovpn->dev),
>> -					    peer->id, &bind->local.ipv6,
>> -					    &ipv6_hdr(skb)->daddr);
>> -			bind->local.ipv6 = ipv6_hdr(skb)->daddr;
>> -		}
>> -		break;
>> -	default:
>> -		break;
>> -	}
>> -	spin_unlock_bh(&peer->lock);
>> -
>> -unlock:
>> -	rcu_read_unlock();
>> -}
> 
> I guess you could squash this and the previous patch into one to
> reduce the churn (and get a little bit closer to the 15-patch limit :))

Hehe I can see you're getting familiar with the code :-)
Will do!

Thanks!

> 

-- 
Antonio Quartulli
OpenVPN Inc.

Powered by blists - more mailing lists