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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Z8dIXjwZ3QmiEcd-@hog>
Date: Tue, 4 Mar 2025 19:37:18 +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-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.


> +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;
> +		}
> +
> +		/* 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


[...]
> -/* 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 :))

-- 
Sabrina