[RFC net-next] net/ip_tunnels: ip_tunnel_dst_cache_usable() should consider ECMP hashing

[RFC net-next] net/ip_tunnels: ip_tunnel_dst_cache_usable() should consider ECMP hashing

[Damien Claisse]

From: Damien Claisse <d.claisse@criteo.com>

I have recently observed an issue with the following setup:
* A server with 2 NICs for redundancy purposes
* fib_multipath_hash_policy sysctl set to 1 for both IPv4 and IPv6.
* A VXLAN overlay is in use, the server acting as a software VTEP. Both
  NICs can be used to reach other end of the tunnel.
* Servers go through internet using a set of stateful boxes (doing NAT
  and firewalling). Each one announces default route to the fabric, and
  ECMP is enabled so all boxes are active/active. The VXLAN overlay is
  used to reach the "Internet" VRF.

With this setup, a curl on the multi-homed server to a public website
would frequently fail, because I would receive an RST packet in the
middle of the flow.

Investigations showed that packets for a given inner flow could go out
from multiple NICs, so the packet will end on multiple stateful boxes,
so when a box would receive a packet for an established flow it doesn't
know about, it would send an RST.

Drilling down the kernel code, I determined that dst_cache and
fib_multipath_hash_policy don't play well with each other:

* dst_cache avoids calling fib_select_path() for every packet. When it
  was introduced, this gave reported gains of 3% for IPv4 and 70% for
  IPv6 VXLAN tunneling, as explained in commit 0c1d70af924b ("net: use
  dst_cache for vxlan device").
* ip_tunnel_dst_cache_usable() determines whether dst_cache can be used
  for most tunnel protocols.
* dst_cache stores a routing decision per CPU, keyed on the tunnel
  destination IP address alone. This breaks multihomed tunnel setups
  (VXLAN, GRE, IPIP, SIT, Geneve, ...) whenever multipath hashing is
  configured to look at fields beyond the outer flow's L3 headers, i.e.
  sysctl fib_multipath_hash_policy != 0.

Example failure scenario, using fib_multipath_hash_policy=1:

  1. CPU 0 processes flow A toward tunnel remote R. fib_select_path()
     hashes on 5-tuple and picks nexthop NH1 via eth0. The result is
     stored in dst_cache under key R on CPU 0.

  2. CPU 1 processes flow B toward the same remote R. Its L4 hash
     lands on NH2 via eth1. The result is stored in dst_cache under
     key R on CPU 1.

  3. Once all CPUs have warmed their per-CPU cache, every subsequent
     packet to R bypasses the route lookup entirely and uses whatever
     nexthop happens to be cached on the CPU that processes it. In our
     example, a flow toward tunnel remote R would go through NH1/eth0
     when handled by CPU 0, and NH2/eth1 when handled by CPU 1.

  4. Flows that span multiple CPUs no longer follow a consistent path.
     Packets from the same TCP connection can exit on different NICs,
     ending up through different stateful NAT or firewall boxes and
     breaking the connection, or causing out-of-order delivery.

Analyzing commit history, it seems the chain of events that led to this
situation is the following:
* dst_cache was added in commit 911362c70df5 ("net: add dst_cache
  support"), using only the destination IP as cache key, since
  multipath L4 hashing did not exist yet.
* The IPv4 fib_multipath_hash_policy sysctl was introduced in
  commit bf4e0a3db97e ("net: ipv4: add support for ECMP hash
  policy choice"), but did not account for dst_cache using only the
  destination IP as cache key.
* ip_tunnel_dst_cache_usable() was introduced in commit db3c6139e6ea
  ("bpf, vxlan, geneve, gre: fix usage of dst_cache on xmit") as a way
  to disable the cache under some conditions (e.g. when ToS is set on a
  packet).
* The IPv6 fib_multipath_hash_policy sysctl was introduced in
  commit b4bac172e90c ("net/ipv6: Add support for path selection using
  hash of 5-tuple"), with the same behavior as IPv4.

As a result, multihomed tunnels running with
fib_multipath_hash_policy != 0 hit the issue described above.

As for the fix, I'm not sure about the best option:

* Making dst_cache L4-aware: not sure about the benefit, as I expect
  heavy fragmentation and therefore a very low hit rate, meaning even
  worse performance than without the cache.

* Not using dst_cache when fib_multipath_hash_policy != 0: this is
  the naive approach I'm suggesting here in the attached patch, at
  the cost of losing the cache. I've tested this patch on IPv4 and
  observed only a small CPU consumption increase on a 25Gbps server
  (less than 2%), but I haven't been able to test it on IPv6. I'm
  curious how fib_select_path() performance for IPv6 has evolved
  since 2016 -- is it still that slow? In any case, even if we went
  this way, the patch here does not cover all the issues, as some
  implementations (e.g. wireguard, ovpn or sit) don't use
  ip_tunnel_dst_cache_usable() and will need their own fixes.

What do you think about these? Any preferred option, or any better one
that could be explored?

Signed-off-by: Damien Claisse <d.claisse@criteo.com>
---
 drivers/net/bareudp.c          |  6 +++---
 drivers/net/geneve.c           |  8 ++++----
 drivers/net/vxlan/vxlan_core.c |  2 +-
 include/net/ip_tunnels.h       | 26 +++++++++++++++++++-------
 net/ipv4/ip_tunnel.c           |  4 ++--
 5 files changed, 29 insertions(+), 17 deletions(-)

diff --git a/drivers/net/bareudp.c b/drivers/net/bareudp.c
index c3b5ed52d877..269bf5da9e13 100644
--- a/drivers/net/bareudp.c
+++ b/drivers/net/bareudp.c
@@ -305,7 +305,7 @@ static int bareudp_xmit_skb(struct sk_buff *skb, struct net_device *dev,
 {
 	bool udp_sum = test_bit(IP_TUNNEL_CSUM_BIT, info->key.tun_flags);
 	bool xnet = !net_eq(bareudp->net, dev_net(bareudp->dev));
-	bool use_cache = ip_tunnel_dst_cache_usable(skb, info);
+	bool use_cache = ip_tunnel_dst_cache_usable(skb, info, bareudp->net);
 	struct sock *sk = rcu_dereference(bareudp->sk);
 	const struct ip_tunnel_key *key = &info->key;
 	struct rtable *rt;
@@ -375,7 +375,7 @@ static int bareudp6_xmit_skb(struct sk_buff *skb, struct net_device *dev,
 {
 	bool udp_sum = test_bit(IP_TUNNEL_CSUM_BIT, info->key.tun_flags);
 	bool xnet = !net_eq(bareudp->net, dev_net(bareudp->dev));
-	bool use_cache = ip_tunnel_dst_cache_usable(skb, info);
+	bool use_cache = ip_tunnel_dst_cache_usable(skb, info, bareudp->net);
 	struct sock *sk = rcu_dereference(bareudp->sk);
 	const struct ip_tunnel_key *key = &info->key;
 	struct dst_entry *dst = NULL;
@@ -504,7 +504,7 @@ static int bareudp_fill_metadata_dst(struct net_device *dev,
 	bool use_cache;
 	__be16 sport;
 
-	use_cache = ip_tunnel_dst_cache_usable(skb, info);
+	use_cache = ip_tunnel_dst_cache_usable(skb, info, bareudp->net);
 	sport = udp_flow_src_port(bareudp->net, skb,
 				  bareudp->sport_min, USHRT_MAX,
 				  true);
diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index 0ab729f055ea..3df895dbf301 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -1400,7 +1400,7 @@ static int geneve_xmit_skb(struct sk_buff *skb, struct net_device *dev,
 	if (!gs4)
 		return -EIO;
 
-	use_cache = ip_tunnel_dst_cache_usable(skb, info);
+	use_cache = ip_tunnel_dst_cache_usable(skb, info, geneve->net);
 	tos = geneve_get_dsfield(skb, dev, cfg, info, &use_cache);
 	sport = udp_flow_src_port(geneve->net, skb,
 				  cfg->port_min,
@@ -1518,7 +1518,7 @@ static int geneve6_xmit_skb(struct sk_buff *skb, struct net_device *dev,
 	if (!gs6)
 		return -EIO;
 
-	use_cache = ip_tunnel_dst_cache_usable(skb, info);
+	use_cache = ip_tunnel_dst_cache_usable(skb, info, geneve->net);
 	prio = geneve_get_dsfield(skb, dev, cfg, info, &use_cache);
 	sport = udp_flow_src_port(geneve->net, skb,
 				  cfg->port_min,
@@ -1673,7 +1673,7 @@ static int geneve_fill_metadata_dst(struct net_device *dev, struct sk_buff *skb)
 		if (!gs4)
 			return -EIO;
 
-		use_cache = ip_tunnel_dst_cache_usable(skb, info);
+		use_cache = ip_tunnel_dst_cache_usable(skb, info, geneve->net);
 		tos = geneve_get_dsfield(skb, dev, cfg, info, &use_cache);
 		sport = udp_flow_src_port(geneve->net, skb,
 					  cfg->port_min,
@@ -1700,7 +1700,7 @@ static int geneve_fill_metadata_dst(struct net_device *dev, struct sk_buff *skb)
 		if (!gs6)
 			return -EIO;
 
-		use_cache = ip_tunnel_dst_cache_usable(skb, info);
+		use_cache = ip_tunnel_dst_cache_usable(skb, info, geneve->net);
 		prio = geneve_get_dsfield(skb, dev, cfg, info, &use_cache);
 		sport = udp_flow_src_port(geneve->net, skb,
 					  cfg->port_min,
diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
index 459f19f7071e..f9e3d40eaf13 100644
--- a/drivers/net/vxlan/vxlan_core.c
+++ b/drivers/net/vxlan/vxlan_core.c
@@ -2388,7 +2388,7 @@ void vxlan_xmit_one(struct sk_buff *skb, struct net_device *dev,
 	old_iph = ip_hdr(skb);
 
 	info = skb_tunnel_info(skb);
-	use_cache = ip_tunnel_dst_cache_usable(skb, info);
+	use_cache = ip_tunnel_dst_cache_usable(skb, info, vxlan->net);
 
 	if (rdst) {
 		memset(&key, 0, sizeof(key));
diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
index 7c9aadfe8fe3..92cc5f9738d9 100644
--- a/include/net/ip_tunnels.h
+++ b/include/net/ip_tunnels.h
@@ -317,20 +317,32 @@ static inline void ip_tunnel_key_init(struct ip_tunnel_key *key,
 		       0, sizeof(*key) - IP_TUNNEL_KEY_SIZE);
 }
 
+static inline unsigned short ip_tunnel_info_af(const struct ip_tunnel_info
+					       *tun_info)
+{
+	return tun_info->mode & IP_TUNNEL_INFO_IPV6 ? AF_INET6 : AF_INET;
+}
+
 static inline bool
 ip_tunnel_dst_cache_usable(const struct sk_buff *skb,
-			   const struct ip_tunnel_info *info)
+			   const struct ip_tunnel_info *info,
+			   const struct net *net)
 {
 	if (skb->mark)
 		return false;
 
-	return !info || !test_bit(IP_TUNNEL_NOCACHE_BIT, info->key.tun_flags);
-}
+#if IS_ENABLED(CONFIG_IP_ROUTE_MULTIPATH)
+	if ((!info || ip_tunnel_info_af(info) == AF_INET) &&
+	    READ_ONCE(net->ipv4.sysctl_fib_multipath_hash_policy))
+		return false;
+#endif /* IS_ENABLED(CONFIG_IP_ROUTE_MULTIPATH) */
+#if IS_ENABLED(CONFIG_IPV6)
+	if (info && ip_tunnel_info_af(info) == AF_INET6 &&
+	    READ_ONCE(net->ipv6.sysctl.multipath_hash_policy))
+		return false;
+#endif /* IS_ENABLED(CONFIG_IPV6) */
 
-static inline unsigned short ip_tunnel_info_af(const struct ip_tunnel_info
-					       *tun_info)
-{
-	return tun_info->mode & IP_TUNNEL_INFO_IPV6 ? AF_INET6 : AF_INET;
+	return !info || !test_bit(IP_TUNNEL_NOCACHE_BIT, info->key.tun_flags);
 }
 
 static inline __be64 key32_to_tunnel_id(__be32 key)
diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
index e6bcf01411d0..b8226aa07382 100644
--- a/net/ipv4/ip_tunnel.c
+++ b/net/ipv4/ip_tunnel.c
@@ -607,7 +607,7 @@ void ip_md_tunnel_xmit(struct sk_buff *skb, struct net_device *dev,
 	if (ip_tunnel_encap(skb, &tun_info->encap, &proto, &fl4) < 0)
 		goto tx_error;
 
-	use_cache = ip_tunnel_dst_cache_usable(skb, tun_info);
+	use_cache = ip_tunnel_dst_cache_usable(skb, tun_info, tunnel->net);
 	if (use_cache)
 		rt = dst_cache_get_ip4(&tun_info->dst_cache, &fl4.saddr);
 	if (!rt) {
@@ -769,7 +769,7 @@ void ip_tunnel_xmit(struct sk_buff *skb, struct net_device *dev,
 		goto tx_error;
 
 	if (connected && md) {
-		use_cache = ip_tunnel_dst_cache_usable(skb, tun_info);
+		use_cache = ip_tunnel_dst_cache_usable(skb, tun_info, tunnel->net);
 		if (use_cache)
 			rt = dst_cache_get_ip4(&tun_info->dst_cache,
 					       &fl4.saddr);
-- 
2.53.0