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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z8gzbz6YjdeGPqgu@hog>
Date: Wed, 5 Mar 2025 12:20:15 +0100
From: Sabrina Dubroca <sd@...asysnail.net>
To: Antonio Quartulli <antonio@...nvpn.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

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.


> > > +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.


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

-- 
Sabrina