kernel: netfilter: fix dst entries in flowtable offload

Signed-off-by: Felix Fietkau <nbd@nbd.name>
openwrt · Mar 15, 2018 · c89e338fe68fd5af61b80ef37c55a657721c6542 · c89e338
1 parent db108cd
commit c89e338fe68fd5af61b80ef37c55a657721c6542
Unified Split

Showing with 111 additions and 11 deletions.

+89 −0 target/linux/generic/backport-4.14/366-netfilter-nf_flow_table-clean-up-and-fix-dst-handlin.patch

+22 −11 target/linux/generic/hack-4.14/650-netfilter-add-xt_OFFLOAD-target.patch
diff --git a/target/linux/generic/backport-4.14/366-netfilter-nf_flow_table-clean-up-and-fix-dst-handlin.patch b/target/linux/generic/backport-4.14/366-netfilter-nf_flow_table-clean-up-and-fix-dst-handlin.patch
@@ -0,0 +1,89 @@
+  
+From: Felix Fietkau <nbd@nbd.name>
+  
+Date: Thu, 15 Mar 2018 18:21:43 +0100
+  
+Subject: [PATCH] netfilter: nf_flow_table: clean up and fix dst handling
+  
+
+  
+dst handling in the code is inconsistent and possibly wrong. In my test,
+  
+skb_dst(skb) holds the dst entry after routing but before NAT, so the
+  
+code could possibly return the same dst entry for both directions of a
+  
+connection.
+  
+Additionally, there was some confusion over the dst entry vs the address
+  
+passed as parameter to rt_nexthop/rt6_nexthop.
+  
+
+  
+Do an explicit dst lookup for both ends of the connection and always use
+  
+the source address for it. When running the IP hook, use the dst entry
+  
+for the opposite direction for determining the route.
+  
+
+  
+Signed-off-by: Felix Fietkau <nbd@nbd.name>
+  
+---
+  
+
+  
+--- a/net/netfilter/nf_flow_table_ip.c
+  
+++ b/net/netfilter/nf_flow_table_ip.c
+  
+@@ -238,7 +238,7 @@ nf_flow_offload_ip_hook(void *priv, stru
+  
+
+  
+ 	dir = tuplehash->tuple.dir;
+  
+ 	flow = container_of(tuplehash, struct flow_offload, tuplehash[dir]);
+  
+-	rt = (const struct rtable *)flow->tuplehash[dir].tuple.dst_cache;
+  
+	rt = (const struct rtable *)flow->tuplehash[!dir].tuple.dst_cache;
+  
+
+  
+ 	if (unlikely(nf_flow_exceeds_mtu(skb, flow->tuplehash[dir].tuple.mtu)) &&
+  
+ 	    (ip_hdr(skb)->frag_off & htons(IP_DF)) != 0)
+  
+@@ -455,7 +455,7 @@ nf_flow_offload_ipv6_hook(void *priv, st
+  
+
+  
+ 	dir = tuplehash->tuple.dir;
+  
+ 	flow = container_of(tuplehash, struct flow_offload, tuplehash[dir]);
+  
+-	rt = (struct rt6_info *)flow->tuplehash[dir].tuple.dst_cache;
+  
+	rt = (struct rt6_info *)flow->tuplehash[!dir].tuple.dst_cache;
+  
+
+  
+ 	if (unlikely(nf_flow_exceeds_mtu(skb, flow->tuplehash[dir].tuple.mtu)))
+  
+ 		return NF_ACCEPT;
+  
+--- a/net/netfilter/nft_flow_offload.c
+  
+++ b/net/netfilter/nft_flow_offload.c
+  
+@@ -17,27 +17,38 @@ struct nft_flow_offload {
+  
+ 	struct nft_flowtable	*flowtable;
+  
+ };
+  
+
+  
+-static int nft_flow_route(const struct nft_pktinfo *pkt,
+  
+-			  const struct nf_conn *ct,
+  
+-			  struct nf_flow_route *route,
+  
+-			  enum ip_conntrack_dir dir)
+  
+static struct dst_entry *
+  
+nft_flow_dst(const struct nf_conn *ct, enum ip_conntrack_dir dir,
+  
+	     const struct nft_pktinfo *pkt)
+  
+ {
+  
+-	struct dst_entry *this_dst = skb_dst(pkt->skb);
+  
+-	struct dst_entry *other_dst = NULL;
+  
+	struct dst_entry *dst;
+  
+ 	struct flowi fl;
+  
+
+  
+ 	memset(&fl, 0, sizeof(fl));
+  
+ 	switch (nft_pf(pkt)) {
+  
+ 	case NFPROTO_IPV4:
+  
+-		fl.u.ip4.daddr = ct->tuplehash[!dir].tuple.dst.u3.ip;
+  
+		fl.u.ip4.daddr = ct->tuplehash[dir].tuple.src.u3.ip;
+  
+ 		break;
+  
+ 	case NFPROTO_IPV6:
+  
+-		fl.u.ip6.daddr = ct->tuplehash[!dir].tuple.dst.u3.in6;
+  
+		fl.u.ip6.daddr = ct->tuplehash[dir].tuple.src.u3.in6;
+  
+ 		break;
+  
+ 	}
+  
+
+  
+-	nf_route(nft_net(pkt), &other_dst, &fl, false, nft_pf(pkt));
+  
+-	if (!other_dst)
+  
+	nf_route(nft_net(pkt), &dst, &fl, false, nft_pf(pkt));
+  
+
+  
+	return dst;
+  
+}
+  
+
+  
+static int nft_flow_route(const struct nft_pktinfo *pkt,
+  
+			  const struct nf_conn *ct,
+  
+			  struct nf_flow_route *route,
+  
+			  enum ip_conntrack_dir dir)
+  
+{
+  
+	struct dst_entry *this_dst, *other_dst;
+  
+
+  
+	this_dst = nft_flow_dst(ct, dir, pkt);
+  
+	other_dst = nft_flow_dst(ct, !dir, pkt);
+  
+	if (!this_dst || !other_dst)
+  
+ 		return -ENOENT;
+  
+
+  
+ 	route->tuple[dir].dst		= this_dst;
diff --git a/target/linux/generic/hack-4.14/650-netfilter-add-xt_OFFLOAD-target.patch b/target/linux/generic/hack-4.14/650-netfilter-add-xt_OFFLOAD-target.patch
@@ -98,7 +98,7 @@ Signed-off-by: Felix Fietkau <nbd@nbd.name>
 obj-$(CONFIG_NETFILTER_XT_TARGET_LED) += xt_LED.o
 --- /dev/null
 +++ b/net/netfilter/xt_FLOWOFFLOAD.c
-  
-@@ -0,0 +1,340 @@
+  
+@@ -0,0 +1,351 @@
 +/*
 + * Copyright (C) 2018 Felix Fietkau <nbd@nbd.name>
 + *
@@ -290,27 +290,38 @@ Signed-off-by: Felix Fietkau <nbd@nbd.name>
 +	return false;
 +}
 +
-  
-+static int
-  
-+xt_flowoffload_route(struct sk_buff *skb, const struct nf_conn *ct,
-  
-+		   const struct xt_action_param *par,
-  
-+		   struct nf_flow_route *route, enum ip_conntrack_dir dir)
+  
+static struct dst_entry *
+  
+xt_flowoffload_dst(const struct nf_conn *ct, enum ip_conntrack_dir dir,
+  
+		   const struct xt_action_param *par)
 +{
-  
-+	struct dst_entry *this_dst = skb_dst(skb);
-  
-+	struct dst_entry *other_dst = NULL;
+  
+	struct dst_entry *dst;
 +	struct flowi fl;
 +
 +	memset(&fl, 0, sizeof(fl));
 +	switch (xt_family(par)) {
 +	case NFPROTO_IPV4:
-  
-+		fl.u.ip4.daddr = ct->tuplehash[!dir].tuple.dst.u3.ip;
+  
+		fl.u.ip4.daddr = ct->tuplehash[dir].tuple.src.u3.ip;
 +		break;
 +	case NFPROTO_IPV6:
-  
-+		fl.u.ip6.daddr = ct->tuplehash[!dir].tuple.dst.u3.in6;
+  
+		fl.u.ip6.daddr = ct->tuplehash[dir].tuple.src.u3.in6;
 +		break;
 +	}
 +
-  
-+	nf_route(xt_net(par), &other_dst, &fl, false, xt_family(par));
-  
-+	if (!other_dst)
+  
+	nf_route(xt_net(par), &dst, &fl, false, xt_family(par));
+  
+
+  
+	return dst;
+  
+}
+  
+
+  
+static int
+  
+xt_flowoffload_route(struct sk_buff *skb, const struct nf_conn *ct,
+  
+		   const struct xt_action_param *par,
+  
+		   struct nf_flow_route *route, enum ip_conntrack_dir dir)
+  
+{
+  
+	struct dst_entry *this_dst, *other_dst;
+  
+
+  
+	this_dst = xt_flowoffload_dst(ct, dir, par);
+  
+	other_dst = xt_flowoffload_dst(ct, !dir, par);
+  
+	if (!this_dst || !other_dst)
 +		return -ENOENT;
 +
 +	route->tuple[dir].dst		= this_dst;