Message-ID: <cd9df084-8633-49f0-a851-ed2b1c9946d3@openvpn.net>
Date: Wed, 5 Mar 2025 14:14:36 +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 05/03/2025 12:20, Sabrina Dubroca wrote:
> 2025-03-05, 00:19:32 +0100, Antonio Quartulli wrote:
>> 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).
> 
> Not just the source address, the routing entry too. I'm more concerned
> about that: trying to reuse a a cached routing entry that was good for
> the previous remote address, but not for the new one.
> 
> 
> [adding your next email]
>> I spent some more time thinking about this.
>> It makes sense to reset the dst cache when the local address changes, but
>> not in case of float (remote address changed).
>>
>> That's because we always want to first attempt sending packets using the
>> address where the remote peer sent the traffic to.
>> Should that not work (quite rare), then we have code in ovpn_udpX_output
>> that will reset the cache and attempt a different address.
> 
> I don't think the code in ovpn_udpX_output will reset the cache unless
> it was made invalid by a system-wide routing table update (see
> dst_cache_per_cpu_get).
> 
> 	rt = dst_cache_get_ip4(cache, &fl.saddr);
> 	if (rt)
> 		goto transmit;
> ...
> transmit:
> 	udp_tunnel_xmit_skb(rt, sk, skb, fl.saddr, fl.daddr, 0,
> 			    ip4_dst_hoplimit(&rt->dst), 0, fl.fl4_sport,
> 			    fl.fl4_dport, false, sk->sk_no_check_tx);
> 
> 
> So it seems that as long as dst_cache_get_ip4 gets us a dst (which
> AFAIU will happen, unless we did a dst_cache_reset or something else
> made the cached dst invalid -- and ovpn's floating/endpoint update
> doesn't do that), we'll just use it.

Mh yeah, you're right.
Then I'll reset the cache also when a float is detected.

> 
> 
>>>> +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.
> 
> Only if the bind was of the correct family? Checking an IPv4 local
> address (in the bind) against an IPv6 source address in the packet (or
> the other way around) isn't going to work well.

Ah I understand what you mean.

The purpose of "local_ip" is to provide a working local endpoint to be 
used with the new remote address.
However, if the float is switching family we can't re-use the same old 
local endpoint (hence the check).
In this case we'll learn the "new" local address later.

Does it make sense?

Cheers,

> 
> 
>> 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;
> 

-- 
Antonio Quartulli
OpenVPN Inc.

Powered by blists - more mailing lists