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.r...@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
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
