ipv6: tie the dst cookie to the lookup that returned the dst
Subsystems (4)
v2 cover ipv6: tie the dst cookie to the lookup that returned the dst
Hi,
This series implements a first part of the model proposed in [1] to address the late cookie sampling race: a dst can be stored together with a cookie read after a FIB change that its lookup did not see, and then passes dst_check until the next change.
The case that made me post it as a series is xfrm. Adding a policy restamps every fib6 node, so a socket whose lookup ran just before the insertion keeps its plain dst and leaks plaintext although the policy matches. I reproduced it for TCP connect and connected UDP, and the same window lets a more specific blackhole route be bypassed for routes using nexthop objects. The ovpn patch [2] hit the same pattern in dst_cache_set_ip6.
The conversion is initially limited to tcp_v6_connect, ip6_datagram_dst_update and ip6_sk_dst_lookup_flow. Other TCP TX cache paths, including established-socket route refills, header rebuilds and accepted-socket setup, still use late cookie sampling. This gives the lookup-cookie API a small set of users and concrete fixes to review. If the approach is accepted, I plan to follow up with a broader conversion across the networking core and drivers.
Compared to [1], the fallback cookie is 0 instead of the pre-lookup generation, so fib6_flush_trees is left alone, and nexthop-object dsts are checked in ip6_dst_check instead of passing the generation down to ip6_rt_pcpu_alloc. The other cache users (dst_cache, sctp, ipvs, flowtable, sk_rx_dst, the xfrm bundle cookies) are not converted and behave as before.
I tested it in a VM with lockdep, PROVE_RCU and DEBUG_ATOMIC_SLEEP, using a debug-only delay after the lookup to hit the window: no warnings, and the plaintext and blackhole bypass are gone in these test cases.
[1] https://lore.kernel.org/netdev/[email protected]/
[2] https://lore.kernel.org/netdev/[email protected]/
Thanks,
Ralf Lici
Mandelbit Srl
---
Changes since v1 https://lore.kernel.org/netdev/[email protected]/
- Fix kdoc return statements in patch 2/3. (NIPA)
Ralf Lici (3):
ipv6: stamp the default route node with a fresh serial on link failure
ipv6: add ip6_dst_lookup_flow_cookie
ipv6: use the lookup cookie when caching the dst in sockets
include/net/ip6_route.h | 18 +++++++++---- include/net/ipv6.h | 13 ++++++++++ net/ipv6/datagram.c | 6 +++-- net/ipv6/ip6_output.c | 57 ++++++++++++++++++++++++++++++++++++++--- net/ipv6/route.c | 24 ++++++++--------- net/ipv6/tcp_ipv6.c | 5 ++-- 6 files changed, 98 insertions(+), 25 deletions(-)
base-commit: c4c470912125ec8a1a427a70f4085d8a731b00e4
Signature · 1 line
2.56.0
PATCH v2 1/3 ipv6: stamp the default route node with a fresh serial on link failure
ip6_link_failure invalidates the cached dsts of an RTF_DEFAULT route by writing -1 into the node's fn_sernum. Unlike every other fn_sernum update, this does not allocate from net->ipv6.fib6_sernum, so the change is invisible to anyone watching the generation, and a cookie sampled from the node after the write simply reads -1 and matches it.
The write is also a no-op when the node already holds -1, so dsts cached after one link failure survive the next one. dsts of nexthop objects are validated against the generation and ignore fn_sernum, so a link failure never invalidated them.
Allocate a fresh serial under tb6_lock instead, as for any other FIB change. A link failure now invalidates the nexthop-object dsts of the namespace too, as a route insertion does.
---
No changes since v1 https://lore.kernel.org/netdev/5b039a5a24808f2678eedd283c29f8dae514ef5e.1793806941.git.ralf@mandelbit.com/
net/ipv6/route.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
net/ipv6/route.c +5 -5
@@ -2867,13 +2867,13 @@ static void ip6_link_failure(struct sk_buff *skb)rt6_remove_exception_rt (rt); }else {struct fib6_info *from; -struct fib6_node *fn; from =rcu_dereference (rt->from); -if (from) { - fn =rcu_dereference (from->fib6_node); -if (fn && (rt->rt6i_flags &RTF_DEFAULT )) -WRITE_ONCE (fn->fn_sernum,-1 ); +if (from && from->fib6_table && + (rt->rt6i_flags &RTF_DEFAULT )) { +spin_lock_bh (&from->fib6_table->tb6_lock); +fib6_update_sernum (rt6_exception_net (rt), from); +spin_unlock_bh (&from->fib6_table->tb6_lock); } }rcu_read_unlock ();
Signature · 1 line
2.56.0
PATCH v2 2/3 ipv6: add ip6_dst_lookup_flow_cookie
In reply to Ralf LiciCallers that keep a dst in a persistent cache read its validation cookie with rt6_get_cookie once the lookup has returned. For a route without its own sernum that is the current fn_sernum of the node, so a FIB change landing in between, or an xfrm policy insertion, which restamps every node, pairs the old dst with the new serial. dst_check then keeps accepting it until the next change.
Add ip6_dst_lookup_flow_cookie, which returns the cookie together with the dst. The cookie is taken between two reads of the IPv6 generation:
if it moved during the lookup the cookie is 0, which is never a node serial, so a route dst can still be used for the packet at hand but is not accepted again. An xfrm bundle ignores the cookie and is validated against its own route and path cookies, which this patch does not change.
dsts of nexthop objects are validated against the global generation and ignore the cookie. For them the helper returns the generation read before the lookup, and ip6_dst_check now requires it to match rt->sernum, so a dst allocated after a FIB change is rejected for a lookup that began before it. Callers that read the cookie from the dst keep passing rt->sernum and are not affected.
---
Changes since v1 https://lore.kernel.org/netdev/e40f48e925a0950eac99a3f084e485e82cabe88f.1793806941.git.ralf@mandelbit.com/
- Fix kdoc return statement in ip6_dst_lookup_flow and
ip6_dst_lookup_flow_cookie. (NIPA)
include/net/ipv6.h | 13 +++++++++++ net/ipv6/ip6_output.c | 51 +++++++++++++++++++++++++++++++++++++++++-- net/ipv6/route.c | 2 +- 3 files changed, 63 insertions(+), 3 deletions(-)
include/net/ipv6.h +13 -0
@@ -1062,6 +1062,11 @@ int ip6_dst_lookup(struct net *net, struct sock *sk, struct dst_entry **dst,#if IS_ENABLED (CONFIG_IPV6 )struct dst_entry *ip6_dst_lookup_flow (struct net *net,const struct sock *sk,struct flowi6 *fl6,const struct in6_addr *final_dst); +struct dst_entry *ip6_dst_lookup_flow_cookie (struct net *net, +const struct sock *sk, +struct flowi6 *fl6, +const struct in6_addr *final_dst, +u32 *cookie);#else static inline struct dst_entry *ip6_dst_lookup_flow(struct net *net, const struct sock *sk, struct flowi6 *fl6,
@@ -1069,6 +1074,14 @@ static inline struct dst_entry *ip6_dst_lookup_flow(struct net *net, const struc {return ERR_PTR (-EAFNOSUPPORT ); } + +static inline struct dst_entry * +ip6_dst_lookup_flow_cookie (struct net *net,const struct sock *sk, +struct flowi6 *fl6,const struct in6_addr *final_dst, +u32 *cookie) +{ +return ERR_PTR (-EAFNOSUPPORT ); +}#endif struct dst_entry *ip6_sk_dst_lookup_flow(struct sock *sk, struct flowi6 *fl6,
net/ipv6/ip6_output.c +49 -2
63 lines not on this page · raw message
net/ipv6/route.c +1 -1
@@ -2811,7 +2811,7 @@ INDIRECT_CALLABLE_SCOPE struct dst_entry *ip6_dst_check(struct dst_entry *dst, rt =dst_rt6_info (dst);if (rt->sernum) -return rt6_is_valid (rt) ? dst :NULL ; +return (rt6_is_valid (rt) && cookie == rt->sernum) ? dst :NULL ;rcu_read_lock ();
Signature · 1 line
2.56.0
PATCH v2 3/3 ipv6: use the lookup cookie when caching the dst in sockets
In reply to Ralf Licitcp_v6_connect, ip6_datagram_dst_update and ip6_sk_dst_lookup_flow store the dst in the socket and read its cookie afterwards. If an xfrm policy matching the flow is added after the route and policy lookup but before the cookie is read, the plain route found without the policy is cached with a post-insertion serial. Later packets pass the validation in __sk_dst_check and skip xfrm, so a socket that should be using IPsec keeps sending plaintext until the FIB changes again. It affects TCP and connected UDP sockets alike.
Store the cookie returned by ip6_dst_lookup_flow_cookie instead. Add ip6_dst_store_cookie for this and keep ip6_dst_store for callers that have no lookup cookie.
---
No changes since v1 https://lore.kernel.org/netdev/832892eaa7d2371f20459c3134c72914e42970b0.1793806941.git.ralf@mandelbit.com/
include/net/ip6_route.h | 18 +++++++++++++----- net/ipv6/datagram.c | 6 ++++-- net/ipv6/ip6_output.c | 6 ++++-- net/ipv6/route.c | 12 ++++++------ net/ipv6/tcp_ipv6.c | 5 +++-- 5 files changed, 30 insertions(+), 17 deletions(-)
include/net/ip6_route.h +13 -5
@@ -252,13 +252,13 @@ static inline const struct rt6_info *skb_rt6_info(const struct sk_buff *skb)/* * Store a destination cache entry in a socket */ -static inline void ip6_dst_store (struct sock *sk,struct dst_entry *dst, -bool daddr_set, -bool saddr_set) +static inline void ip6_dst_store_cookie (struct sock *sk,struct dst_entry *dst, +u32 cookie,bool daddr_set, +bool saddr_set) {struct ipv6_pinfo *np =inet6_sk (sk); - np->dst_cookie =rt6_get_cookie (dst_rt6_info (dst)); + np->dst_cookie = cookie;sk_setup_caps (sk, dst); np->daddr_cache = daddr_set;#ifdef CONFIG_IPV6_SUBTREES
@@ -266,8 +266,16 @@ static inline void ip6_dst_store(struct sock *sk, struct dst_entry *dst,#endif } +static inline void ip6_dst_store (struct sock *sk,struct dst_entry *dst, +bool daddr_set, +bool saddr_set) +{ +ip6_dst_store_cookie (sk, dst,rt6_get_cookie (dst_rt6_info (dst)), + daddr_set, saddr_set); +} +void ip6_sk_dst_store_flow (struct sock *sk,struct dst_entry *dst, -const struct flowi6 *fl6); +const struct flowi6 *fl6,u32 cookie);static inline bool ipv6_unicast_destination (const struct sk_buff *skb) {
net/ipv6/datagram.c +4 -2
@@ -78,6 +78,7 @@ int ip6_datagram_dst_update(struct sock *sk, bool fix_sk_saddr)struct in6_addr *final_p;struct dst_entry *dst;struct flowi6 *fl6; +u32 cookie;int err =0 ; if (inet6_test_bit(SNDFLOW, sk) &&
@@ -94,7 +95,8 @@ int ip6_datagram_dst_update(struct sock *sk, bool fix_sk_saddr) final_p =fl6_update_dst (fl6, opt, &np->final);rcu_read_unlock (); - dst =ip6_dst_lookup_flow (sock_net (sk), sk, fl6, final_p); + dst =ip6_dst_lookup_flow_cookie (sock_net (sk), sk, fl6, final_p, + &cookie);if (IS_ERR (dst)) { err =PTR_ERR (dst); goto out;
@@ -112,7 +114,7 @@ int ip6_datagram_dst_update(struct sock *sk, bool fix_sk_saddr) } } -ip6_sk_dst_store_flow (sk, dst, fl6); +ip6_sk_dst_store_flow (sk, dst, fl6, cookie); out:fl6_sock_release (flowlabel);
net/ipv6/ip6_output.c +4 -2
@@ -1370,14 +1370,16 @@ struct dst_entry *ip6_sk_dst_lookup_flow(struct sock *sk, struct flowi6 *fl6, bool connected) {struct dst_entry *dst =sk_dst_check (sk,inet6_sk (sk)->dst_cookie); +u32 cookie; dst =ip6_sk_dst_check (sk, dst, fl6);if (dst)return dst; - dst =ip6_dst_lookup_flow (sock_net (sk), sk, fl6, final_dst); + dst =ip6_dst_lookup_flow_cookie (sock_net (sk), sk, fl6, final_dst, + &cookie);if (connected && !IS_ERR (dst)) -ip6_sk_dst_store_flow (sk,dst_clone (dst), fl6); +ip6_sk_dst_store_flow (sk,dst_clone (dst), fl6, cookie);return dst; }
net/ipv6/route.c +6 -6
@@ -3042,19 +3042,19 @@ void ip6_sk_update_pmtu(struct sk_buff *skb, struct sock *sk, __be32 mtu) }void ip6_sk_dst_store_flow (struct sock *sk,struct dst_entry *dst, -const struct flowi6 *fl6) +const struct flowi6 *fl6,u32 cookie) {#ifdef CONFIG_IPV6_SUBTREES struct ipv6_pinfo *np =inet6_sk (sk);#endif -ip6_dst_store (sk, dst, -ipv6_addr_equal (&fl6->daddr, &sk->sk_v6_daddr), +ip6_dst_store_cookie (sk, dst, cookie, +ipv6_addr_equal (&fl6->daddr, &sk->sk_v6_daddr), #ifdef CONFIG_IPV6_SUBTREES -ipv6_addr_equal (&fl6->saddr, &np->saddr) ? - true : +ipv6_addr_equal (&fl6->saddr, &np->saddr) ? + true : #endif - false); + false); } static bool ip6_redirect_nh_match(const struct fib6_result *res,
net/ipv6/tcp_ipv6.c +3 -2
@@ -145,6 +145,7 @@ static int tcp_v6_connect(struct sock *sk, struct sockaddr_unsized *uaddr,struct dst_entry *dst;struct flowi6 *fl6;int addr_type; +u32 cookie;int err;if (addr_len <SIN6_LEN_RFC2133 )
@@ -288,7 +289,7 @@ static int tcp_v6_connect(struct sock *sk, struct sockaddr_unsized *uaddr,*/ ip6_ecmp_set_mp_hash (net, fl6, sk->sk_txhash); - dst =ip6_dst_lookup_flow (net, sk, fl6, final_p); + dst =ip6_dst_lookup_flow_cookie (net, sk, fl6, final_p, &cookie);if (IS_ERR (dst)) { err =PTR_ERR (dst); goto failure;
@@ -312,7 +313,7 @@ static int tcp_v6_connect(struct sock *sk, struct sockaddr_unsized *uaddr, inet->inet_rcv_saddr =LOOPBACK4_IPV6 ; sk->sk_gso_type =SKB_GSO_TCPV6 ; -ip6_dst_store (sk, dst, false, false); +ip6_dst_store_cookie (sk, dst, cookie, false, false); icsk->icsk_ext_hdr_len =0 ;if (opt)
Signature · 1 line
2.56.0