ovpn: avoid caching stale IPv6 dst after FIB changes

Hi,

This patch was originally part of a wider ovpn series addressing UDP route-cache correctness when mutable socket routing properties or peer endpoint state change while packets are being transmitted [1]. The ovpn-specific fixes were resubmitted separately after dropping this patch, which addresses an independent IPv6 FIB race reproduced in ovpn.

During review, Sabrina suggested submitting the patch directly to netdev rather than through the ovpn pull request because it touches generic dst_cache code and the underlying late-cookie sampling pattern is not specific to ovpn. I first sent an RFC describing the race and a possible kernel-wide solution [2], but it has not received any feedback so far.

After discussing the submission path with Antonio, we decided to post the concrete ovpn fix directly to netdev. It prevents ovpn from caching a route if the IPv6 FIB generation changed during lookup, without attempting the broader network-stack refactoring discussed in the RFC.

[1] https://lore.kernel.org/openvpn-devel/[email protected]/
[2] https://lore.kernel.org/netdev/[email protected]/

Compared with the version previously posted as part of the ovpn series, this patch has been rebased on current net/main and made independent of the pending ovpn route-cache and endpoint changes.

Thanks,

Ralf Lici
Mandelbit Srl

---
Ralf Lici (1):
ovpn: avoid caching stale IPv6 dst after FIB changes

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

base-commit: c9151088f1674fd29ff26a20f5fc687acf53a2f0

2.55.0

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.

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

index 7f69e8890b5b..a28974cc36e8 100644
@@ -220,8 +220,10 @@ static int ovpn_udp6_output(struct ovpn_peer *peer, struct ovpn_bind *bind,
 			    struct dst_cache *cache, struct sock *sk,
 			    struct sk_buff *skb)
 {
+	struct net *net = sock_net(sk);
 	struct dst_entry *dst;
-	int ret;
+	int gen0, gen1, ret;
+	u32 cookie;
 
 	struct flowi6 fl = {
 		.saddr = bind->local.ipv6,

@@ -250,7 +252,11 @@ static int ovpn_udp6_output(struct ovpn_peer *peer, struct ovpn_bind *bind,
 		dst_cache_reset(cache);
 	}
 
-	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",

@@ -258,7 +264,15 @@ static int ovpn_udp6_output(struct ovpn_peer *peer, struct ovpn_bind *bind,
 				    &bind->remote.in6, ret);
 		goto err;
 	}
-	dst_cache_set_ip6(cache, dst, &fl.saddr);
+
+	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);
+
+	if (likely(gen0 == gen1))
+		dst_cache_set_ip6_cookie(cache, dst, &fl.saddr, cookie);
 
 transmit:
 	/* user IPv6 packets may be larger than the transport interface

index 1961699598e2..5f9cc4fe926c 100644
@@ -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

index 9ab4902324e1..76c9fc1b0acf 100644
@@ -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,21 @@ 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)
+{
+	if (!dst_cache->cache)
+		return;
+
+	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,

2.55.0

I agree. Keeping this detail inside the IPv6 routing code is exactly why I sent the RFC linked from the cover letter, and I would appreciate your thoughts on the proposed approach:

https://lore.kernel.org/netdev/[email protected]/

The pattern of obtaining a dst and only later sampling its validation cookie when publishing it into a persistent cache is widespread in both drivers and the networking core. It is racy for IPv6 routes with rt6_info::sernum == 0, because rt6_get_cookie then reads the current fib6_node::fn_sernum, not necessarily the value corresponding to the lookup which returned that dst.

Ralf Lici
Mandelbit Srl

the patch is focused on detecting a genid change at the time the dst is cached. Why? It can just as easily change immediately after the double genid check, so you are not really solving the problem.

Staleness is irrelevant until it is used again (ie, genid can change many times between dst_cache_set and next use). That is the proper time to recheck the genid and refresh the dst as needed.

The double generation check is not intended to guarantee that the dst cannot become stale after the final read. Its purpose is to ensure that a pre-change dst is not cached with a post-change cookie. And that is enough because dst_cache_get_ip6, on the next run, misses the cache if it's no longer valid.

For example, suppose the lookup returns D0 under cookie C0. If a relevant FIB change to C1 happens after the final generation read (even if it happens before dst_cache_set_ip6_cookie) the cache stores (D0, C0). On the next use, dst_cache_get_ip6 invokes the ipv6 dst check, which compares the cached C0 with the node's current C1 and rejects the entry.

The problematic sequence is instead:

lookup returns D0 under C0
FIB changes to C1
dst_cache_set_ip6 samples C1 from D0's fib6 node
cache stores (D0, C1)

The reuse-time check then compares the cached C1 with the current C1 and accepts D0, potentially until another relevant FIB change. The patch ensures that the saved cookie is either coherent with the lookup or conservatively old, so the existing check at reuse remains meaningful.

Ralf Lici
Mandelbit Srl

Hi David,

Just following up on this. Does my previous example clarify why storing an old dst with a new cookie defeats reuse-time validation?

If so, would you prefer that I pursue the kernel-wide lookup-provenance approach described in the RFC, or keep the fix self-contained in ovpn? The same late-cookie-sampling pattern is used by several drivers and callers in the networking core, so I think fixing it generically is the better architectural choice.

If changing the generic interfaces is not acceptable, I can rework the patch as an ovpn-local fix, although that would duplicate IPv6 routing details in the driver and leave the equivalent pattern elsewhere unchanged.

Thanks,

Ralf Lici
Mandelbit Srl

dst's are cached in lots of places. Why does ovpn need internal routing details that the other places do not?

Because, as explained in this thread and in the RFC, there is enough evidence that the current IPv6 dst-caching pattern is racy. If other kernel users are expected to live with that race, I cannot force a generic fix, but I do not think that is a good reason to require ovpn to do the same.

If you believe the sequence I described is safe, please explain which invariant makes it so.

Ralf Lici
Mandelbit Srl

Hi David,

ovpn maintainer here: I wanted to back this patch.

You're right that a cached dst can go stale later and that reuse-time validation handles that. This case is different, because it breaks reuse-time validation.

dst_cache_set_ip6() samples the cookie itself:

        dst_cache_per_cpu_dst_set(idst, dst,
                                  rt6_get_cookie(dst_rt6_info(dst)));

rt6_get_cookie() only returns a route-tied value when rt->sernum is non-zero, which happens in ip6_rt_pcpu_alloc() and only for nexthop routes. Everything else reads the current node generation:

        *cookie = READ_ONCE(fn->fn_sernum);

So if the FIB changes between ip6_dst_lookup_flow() returning D0 and dst_cache_set_ip6() reading the cookie, we cache D0 with cookie C1. At reuse dst_check() compares C1 against the node's current C1, they match, and the stale route is used until the next FIB change.

Which invariant stops that cookie read from seeing a generation newer than the one the lookup used? If there is one, we'll drop both the patch and the RFC.

Regarding the layering, I agree. rt_genid_ipv6() does not belong in a driver.
That's exactly what the RFC [1] was for, but it's had no feedback so far, hence I suggested Ralf to pursue this other path. The late sampling lives in dst_cache_set_ip6(), not in ovpn.

Happy to confine the IPv6 bits to a helper, or to revive the RFC if you'd like review it.

Please also note that the workaround we're looking at is already implemented by other drivers, which they all could drop if we'd be able to provide an IPv6-generic solution.

Please let us know!

[1] https://lore.kernel.org/netdev/[email protected]/

Thanks,
Antonio

Antonio Quartulli
OpenVPN Inc.

Have you tried setting sernum for all dst_entry when they are created?

This deferred reading of fn_sernum seems to be the problem. fib entries have sernum set. dst_entries are created from the fib entries and should carry the sernum from the fib entry at that moment.

Hi David,

We have considered that, but the existing field cannot simply be populated for every dst. A nonzero rt6_info->sernum currently selects global validation: ip6_dst_check and rt6_get_pcpu_route compare it with rt_genid_ipv6. It is populated only for external nexthop objects, while ordinary routes leave it zero and validate through their fib6_node->fn_sernum, as commit 8f34e53b60b3 ("ipv6: Use global sernum for dst validation with nexthop objects") explains.

If by setting it for every dst you mean assigning the global rt_genid_ipv6 value, that would fix the race, but it would turn ordinary IPv6 dst validation from per-node invalidation into namespace-wide invalidation, similarly to IPv4.

Yes, that is precisely the core issue. The candidate node's fn_sernum must be sampled as part of route selection and preserved together with the selected dst, rather than recovered from the dst's fib6_node later.

So there seems to be two possible solutions:

1. add a separate lookup-time cookie to rt6_info, while retaining the existing sernum as the external nexthop / global validation marker;
2. have cache-producing lookup APIs return the explicit (dst, cookie) pair proposed in the RFC.

The former keeps existing cache users unchanged, but the stored cookie would have to remain immutable, and reusable per-CPU or exception dsts would need to be replaced when their node serial changes. The latter keeps provenance per lookup but requires propagating the pair through cache-producing callers. What would you prefer?

Thanks!

Ralf Lici
Mandelbit Srl