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

On Thu, 13 Aug 2026 18:55:33 +0200, Sabrina Dubroca <[email protected]> wrote:
> Hi Ralf,
>

Hi Sabrina,

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

Agreed. My plan is to split this in 3 patches:

1. Validate bind identity and route key before publishing a looked-up
   dst;
2. Make ovpn_peer_endpoints_update replace the bind when learning a new
   local endpoint;
3. Make the UDP output fallback replace the bind when clearing a stale
   local source.

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

Good catch.

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

Right, that's cleaner. I'll break from the switch when the local
endpoint changed and go directly to unlock when nothing changed.

> >  
> > +	/* 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?
>

Yes, agreed. I'll move the copy into each local-endpoint learning block.

-- 
Ralf Lici
Mandelbit Srl