Re: [PATCH v21 18/24] ovpn: add support for peer floating

[Antonio Quartulli]

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.