diff mbox series

[Openvpn-devel,net,v2,5/5] ovpn: avoid caching stale IPv6 dst after FIB changes

Message ID 25d830e082b03afb4615aaeed6e0dc1d1370ecbc.1785308184.git.ralf@mandelbit.com
State Changes Requested
Headers
Series ovpn: fix UDP route cache and endpoint handling |

Commit Message

Ralf Lici July 29, 2026, 7:20 a.m. UTC 
  ovpn stores the IPv6 route used for UDP transmission in a per-peer dst
cache. IPv6 dst validation uses a cookie derived from the route itself,
or, for routes without their own sernum, from the associated fib6 node.

If the IPv6 FIB changes after ip6_dst_lookup_flow returns but before
dst_cache_set_ip6 reads the cookie, ovpn can store an old dst with a new
cookie. Later dst_cache_get_ip6 can then consider that stale dst valid
because the stored cookie matches the updated fib6 node sernum.

Sample the IPv6 FIB generation before and after route lookup, and only
populate ovpn's peer dst cache if the generation did not change while
the lookup was in flight. Also add a dst_cache helper that stores a
caller-provided IPv6 cookie, so the cached dst carries the cookie
sampled from the lookup result instead of one read after a concurrent
FIB update.

The current packet may still be transmitted with the route returned by
the lookup if the FIB changes before TX completion. This patch only
prevents that potentially stale route from being preserved in ovpn's
peer dst cache and reused for later packets.

Fixes: 08857b5ec5d9 ("ovpn: implement basic TX path (UDP)")
Signed-off-by: Ralf Lici <ralf@mandelbit.com>
---
Changes since v1 https://lore.kernel.org/openvpn-devel/d6c941fe19455b940dd24019e32b121f332fdc95.1785253480.git.ralf@mandelbit.com/
- Add smp_rmb barriers after the initial generation read and before the
  final generation read to avoid reordering around the lookup on weakly
  ordered architectures (Sashiko).

 drivers/net/ovpn/udp.c  | 50 ++++++++++++++++++++++++++++-------------
 include/net/dst_cache.h | 13 +++++++++++
 net/core/dst_cache.c    | 16 +++++++++----
 3 files changed, 60 insertions(+), 19 deletions(-)

Comments

Sabrina Dubroca Aug. 13, 2026, 5:26 p.m. UTC | #1 
2026-07-29, 09:20:36 +0200, Ralf Lici wrote:
> ovpn stores the IPv6 route used for UDP transmission in a per-peer dst
> cache. IPv6 dst validation uses a cookie derived from the route itself,
> or, for routes without their own sernum, from the associated fib6 node.
> 
> If the IPv6 FIB changes after ip6_dst_lookup_flow returns but before
> dst_cache_set_ip6 reads the cookie, ovpn can store an old dst with a new
> cookie. Later dst_cache_get_ip6 can then consider that stale dst valid
> because the stored cookie matches the updated fib6 node sernum.
> 
> Sample the IPv6 FIB generation before and after route lookup, and only
> populate ovpn's peer dst cache if the generation did not change while
> the lookup was in flight. Also add a dst_cache helper that stores a
> caller-provided IPv6 cookie, so the cached dst carries the cookie
> sampled from the lookup result instead of one read after a concurrent
> FIB update.
> 
> The current packet may still be transmitted with the route returned by
> the lookup if the FIB changes before TX completion. This patch only
> prevents that potentially stale route from being preserved in ovpn's
> peer dst cache and reused for later packets.
> 
> Fixes: 08857b5ec5d9 ("ovpn: implement basic TX path (UDP)")
> Signed-off-by: Ralf Lici <ralf@mandelbit.com>
> ---
> Changes since v1 https://lore.kernel.org/openvpn-devel/d6c941fe19455b940dd24019e32b121f332fdc95.1785253480.git.ralf@mandelbit.com/
> - Add smp_rmb barriers after the initial generation read and before the
>   final generation read to avoid reordering around the lookup on weakly
>   ordered architectures (Sashiko).
> 
>  drivers/net/ovpn/udp.c  | 50 ++++++++++++++++++++++++++++-------------
>  include/net/dst_cache.h | 13 +++++++++++
>  net/core/dst_cache.c    | 16 +++++++++----
>  3 files changed, 60 insertions(+), 19 deletions(-)

I think you should submit this one to netdev for review (as a patch,
not as part of the next ovpn pull request), since it touches code
outside of ovpn.

I also feel like there should be a better way to do this. Modules
doing their own cookie comparison seems wrong.
Ralf Lici Aug. 14, 2026, 3:12 p.m. UTC | #2 
On Thu, 13 Aug 2026 19:26:29 +0200, Sabrina Dubroca <sd@queasysnail.net> wrote:
> 2026-07-29, 09:20:36 +0200, Ralf Lici wrote:
> > ovpn stores the IPv6 route used for UDP transmission in a per-peer dst
> > cache. IPv6 dst validation uses a cookie derived from the route itself,
> > or, for routes without their own sernum, from the associated fib6 node.
> > 
> > If the IPv6 FIB changes after ip6_dst_lookup_flow returns but before
> > dst_cache_set_ip6 reads the cookie, ovpn can store an old dst with a new
> > cookie. Later dst_cache_get_ip6 can then consider that stale dst valid
> > because the stored cookie matches the updated fib6 node sernum.
> > 
> > Sample the IPv6 FIB generation before and after route lookup, and only
> > populate ovpn's peer dst cache if the generation did not change while
> > the lookup was in flight. Also add a dst_cache helper that stores a
> > caller-provided IPv6 cookie, so the cached dst carries the cookie
> > sampled from the lookup result instead of one read after a concurrent
> > FIB update.
> > 
> > The current packet may still be transmitted with the route returned by
> > the lookup if the FIB changes before TX completion. This patch only
> > prevents that potentially stale route from being preserved in ovpn's
> > peer dst cache and reused for later packets.
> > 
> > Fixes: 08857b5ec5d9 ("ovpn: implement basic TX path (UDP)")
> > Signed-off-by: Ralf Lici <ralf@mandelbit.com>
> > ---
> > Changes since v1 https://lore.kernel.org/openvpn-devel/d6c941fe19455b940dd24019e32b121f332fdc95.1785253480.git.ralf@mandelbit.com/
> > - Add smp_rmb barriers after the initial generation read and before the
> >   final generation read to avoid reordering around the lookup on weakly
> >   ordered architectures (Sashiko).
> > 
> >  drivers/net/ovpn/udp.c  | 50 ++++++++++++++++++++++++++++-------------
> >  include/net/dst_cache.h | 13 +++++++++++
> >  net/core/dst_cache.c    | 16 +++++++++----
> >  3 files changed, 60 insertions(+), 19 deletions(-)
>
> I think you should submit this one to netdev for review (as a patch,
> not as part of the next ovpn pull request), since it touches code
> outside of ovpn.
>

Ok, I'll drop this from the ovpn series and send it directly to netdev
as RFC, rebased on the current net tree and including the ovpn user, so
it does not depend on the other pending ovpn changes.

> I also feel like there should be a better way to do this. Modules
> doing their own cookie comparison seems wrong.
>

I agree, and in fact I was quite surprised when I first realized this
race. Initially kept the fix local to ovpn because a broader fix does
not seem straightforward without changing the existing IPv6 FIB
synchronization model.

Specifically, commit bbd63f06d114 ("ipv6: update fn_sernum after route
is inserted to tree") establishes the writer order as route-tree update,
barrier, then fn_sernum update, paired with a barrier after the reader
samples fn_sernum. Thus, observing a new serial orders subsequent
route-tree reads, but cannot associate that serial with a lookup which
has already completed.

After looking through the tree, I found at least 11 other call sites
which perform a lookup and later let dst_cache_set_ip6 sample the
cookie. There are also several persistent caches which store
rt6_get_cookie separately after obtaining the dst (for example
ip6_dst_store, including the tcp_v6_connect path). I haven't really
verified every user but structurally they appear to have the same
window.

The difficult part of a generic fix is that the node whose serial should
be sampled is only identified by the lookup, whereas the synchronization
contract wants that serial to be read before the authoritative
route-tree observation. I'll describe this broader issue in the RFC and
use ovpn as the concrete user, so that the IPv6 maintainers can advise
whether this should instead be addressed in the routing core.

Thanks!
diff mbox series

Patch

diff --git a/drivers/net/ovpn/udp.c b/drivers/net/ovpn/udp.c
index ced4f9ff4a08..e429bfa694fc 100644
--- a/drivers/net/ovpn/udp.c
+++ b/drivers/net/ovpn/udp.c
@@ -316,9 +316,11 @@  static int ovpn_udp6_output(struct ovpn_peer *peer, struct ovpn_bind *bind,
 {
 	struct in6_addr local = in6addr_any;
 	struct sockaddr_storage remote;
+	struct net *net = sock_net(sk);
 	bool reset_local = false;
 	struct dst_entry *dst;
-	int ret;
+	int gen0, gen1, ret;
+	u32 cookie;
 
 	struct flowi6 fl = {
 		.saddr = bind->local.ipv6,
@@ -344,7 +346,11 @@  static int ovpn_udp6_output(struct ovpn_peer *peer, struct ovpn_bind *bind,
 		reset_local = true;
 	}
 
-	dst = ip6_dst_lookup_flow(sock_net(sk), sk, &fl, NULL);
+	gen0 = rt_genid_ipv6(net);
+	/* keep the unordered initial generation read before the FIB lookup */
+	smp_rmb();
+
+	dst = ip6_dst_lookup_flow(net, sk, &fl, NULL);
 	if (IS_ERR(dst)) {
 		ret = PTR_ERR(dst);
 		net_dbg_ratelimited("%s: no route to host %pISpc: %d\n",
@@ -353,27 +359,41 @@  static int ovpn_udp6_output(struct ovpn_peer *peer, struct ovpn_bind *bind,
 		goto err;
 	}
 
+	cookie = rt6_get_cookie(dst_rt6_info(dst));
+
+	/* keep the FIB and cookie reads before the final generation read */
+	smp_rmb();
+	gen1 = rt_genid_ipv6(net);
+
 	/* avoid storing a stale cache or local address */
 	spin_lock_bh(&peer->lock);
 	if (likely(ovpn_dst_cache_current(peer, bind, key))) {
-		if (!reset_local) {
-			dst_cache_set_ip6(cache, dst, &fl.saddr);
+		/* cache the dst with the original cookie only if the learned
+		 * local source was not reset and the FIB did not change
+		 */
+		if (!reset_local && likely(gen0 == gen1)) {
+			dst_cache_set_ip6_cookie(cache, dst, &fl.saddr, cookie);
 			spin_unlock_bh(&peer->lock);
 			goto transmit;
 		}
 
-		/* invalidate per-CPU dst entries that may still carry
-		 * the stale source
-		 */
-		dst_cache_reset(cache);
+		if (reset_local) {
+			/* invalidate per-CPU dst entries that may still carry
+			 * the stale source
+			 */
+			dst_cache_reset(cache);
+
+			/* preserve the current remote */
+			memcpy(&remote, &bind->remote,
+			       sizeof(struct sockaddr_in6));
+			/* The current packet already has a valid
+			 * wildcard-source route. If replacing the bind fails,
+			 * leave the stale local in place; a later cache miss
+			 * will retry the repair.
+			 */
+			ovpn_peer_reset_sockaddr(peer, &remote, &local);
+		}
 
-		/* preserve the current remote */
-		memcpy(&remote, &bind->remote, sizeof(struct sockaddr_in6));
-		/* The current packet already has a valid wildcard-source route.
-		 * If replacing the bind fails, leave the stale local in place;
-		 * a later cache miss will retry the repair.
-		 */
-		ovpn_peer_reset_sockaddr(peer, &remote, &local);
 	}
 	spin_unlock_bh(&peer->lock);
 
diff --git a/include/net/dst_cache.h b/include/net/dst_cache.h
index 1961699598e2..5f9cc4fe926c 100644
--- a/include/net/dst_cache.h
+++ b/include/net/dst_cache.h
@@ -45,6 +45,19 @@  void dst_cache_set_ip4(struct dst_cache *dst_cache, struct dst_entry *dst,
 
 #if IS_ENABLED(CONFIG_IPV6)
 
+/**
+ *	dst_cache_set_ip6_cookie - store ipv6 dst with caller-provided cookie
+ *	@dst_cache: the cache
+ *	@dst: the entry to be cached
+ *	@saddr: the source address to be stored inside the cache
+ *	@cookie: the route validation cookie to store with @dst
+ *
+ *	local BH must be disabled.
+ */
+void dst_cache_set_ip6_cookie(struct dst_cache *dst_cache,
+			      struct dst_entry *dst,
+			      const struct in6_addr *saddr, u32 cookie);
+
 /**
  *	dst_cache_set_ip6 - store the ipv6 dst into the cache
  *	@dst_cache: the cache
diff --git a/net/core/dst_cache.c b/net/core/dst_cache.c
index 9ab4902324e1..1b5e825818ab 100644
--- a/net/core/dst_cache.c
+++ b/net/core/dst_cache.c
@@ -117,8 +117,9 @@  void dst_cache_set_ip4(struct dst_cache *dst_cache, struct dst_entry *dst,
 EXPORT_SYMBOL_GPL(dst_cache_set_ip4);
 
 #if IS_ENABLED(CONFIG_IPV6)
-void dst_cache_set_ip6(struct dst_cache *dst_cache, struct dst_entry *dst,
-		       const struct in6_addr *saddr)
+void dst_cache_set_ip6_cookie(struct dst_cache *dst_cache,
+			      struct dst_entry *dst,
+			      const struct in6_addr *saddr, u32 cookie)
 {
 	struct dst_cache_pcpu *idst;
 
@@ -128,11 +129,18 @@  void dst_cache_set_ip6(struct dst_cache *dst_cache, struct dst_entry *dst,
 	local_lock_nested_bh(&dst_cache->cache->bh_lock);
 
 	idst = this_cpu_ptr(dst_cache->cache);
-	dst_cache_per_cpu_dst_set(idst, dst,
-				  rt6_get_cookie(dst_rt6_info(dst)));
+	dst_cache_per_cpu_dst_set(idst, dst, cookie);
 	idst->in6_saddr = *saddr;
 	local_unlock_nested_bh(&dst_cache->cache->bh_lock);
 }
+EXPORT_SYMBOL_GPL(dst_cache_set_ip6_cookie);
+
+void dst_cache_set_ip6(struct dst_cache *dst_cache, struct dst_entry *dst,
+		       const struct in6_addr *saddr)
+{
+	dst_cache_set_ip6_cookie(dst_cache, dst, saddr,
+				 rt6_get_cookie(dst_rt6_info(dst)));
+}
 EXPORT_SYMBOL_GPL(dst_cache_set_ip6);
 
 struct dst_entry *dst_cache_get_ip6(struct dst_cache *dst_cache,