Re: [Openvpn-devel] [PATCH ovpn net v2 4/5] ovpn: avoid in-place updates of peer bind local address

Hi Ralf,

2026-07-29, 09:20:35 +0200, Ralf Lici wrote:
> struct ovpn_bind is published through peer->bind with RCU. Remote
> endpoint changes already replace the whole bind object, but local
> endpoint learning and UDP source fallback still updated bind->local in
> place. UDP TX can read it locklessly while another CPU updates it under
> peer->lock. For IPv6, that can produce torn reads of the address field.
> 
> Fix this by making the local endpoint immutable after publication too:
> build a new bind object with the updated local address and publish it
> through peer->bind.
> 
> When UDP TX discovers that the remembered local source is no longer
> usable, retry route lookup with a wildcard source. If the lookup
> succeeds and the bind used for the lookup is still current, invalidate
> the peer dst cache and best-effort publish a replacement bind with
> wildcard local address. The current packet can still be transmitted with
> the resolved route even if that bind replacement fails; a later cache
> miss will retry the repair.
> 
> Only store the resolved dst when the local address did not need to be
> reset. A local address change invalidates all per-CPU dst cache entries,
> while dst_cache_set_ip4 and dst_cache_set_ip6 only update the current
> CPU slot. Avoid the old reset-then-set pattern and let the next TX
> repopulate the cache from the new bind state.

This feels like 2 or 3 different bugs. At least making the endpoint
changes rebuild the bind object seems like a standalone patch.

> Fixes: f0281c1d3732 ("ovpn: add support for updating local or remote UDP endpoint")
> Fixes: 08857b5ec5d9 ("ovpn: implement basic TX path (UDP)")
> Signed-off-by: Ralf Lici <[email protected]>
> ---
> No changes since v1 https://lore.kernel.org/openvpn-devel/082540583b9145d89e1cdd5a74c485ea3a53d285.1785253480.git.ralf@mandelbit.com/
> 
>  drivers/net/ovpn/peer.c |  36 +++++++++-----
>  drivers/net/ovpn/udp.c  | 108 +++++++++++++++++++++++++++++++---------
>  2 files changed, 107 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/net/ovpn/peer.c b/drivers/net/ovpn/peer.c
> index 4806e942be27..383d712582c9 100644
> --- a/drivers/net/ovpn/peer.c
> +++ b/drivers/net/ovpn/peer.c
[...]
> @@ -267,26 +263,40 @@ void ovpn_peer_endpoints_update(struct ovpn_peer *peer, struct sk_buff *skb)
>  					    netdev_name(peer->ovpn->dev),
>  					    peer->id, &bind->local.ipv6,
>  					    &ipv6_hdr(skb)->daddr);
> -			bind->local.ipv6 = ipv6_hdr(skb)->daddr;
> -			reset_cache = true;
> +			local_ip = &ipv6_hdr(skb)->daddr;
>  		}
>  		break;
>  	default:
>  		goto unlock;
>  	}
>  
> -	if (unlikely(reset_cache))
> -		dst_cache_reset(&peer->dst_cache);
> -
> -	/* if the peer did not float, we can bail out now */
> -	if (likely(!salen))
> +	/* if there was no float and the local address is unchanged, bail out */
> +	if (likely(!salen && !local_ip))
>  		goto unlock;

nit: !salen is redundant? local_ip is also set in the float case.

but this whole check could be skipped by adding a "break" in the
"local endpoint changed" case and a "goto unlock" if nothing changed
in both cases of the skb->protocol switch.

>  
> +	/* if only the local address changed, populate ss with the current
> +	 * remote
> +	 */
> +	if (!salen)
> +		memcpy(&ss, &bind->remote,
> +		       bind->remote.in4.sin_family == AF_INET ?
> +			       sizeof(struct sockaddr_in) :
> +			       sizeof(struct sockaddr_in6));

nit: Maybe move that under the "learning local IPv*" blocks to avoid
the sizeof ugliness here?


-- 
Sabrina


_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel