[PATCH net-next 0/2] Fixes for tc act_ct software offload of established flows (diff v4->v6)

[PATCH net-next 0/2] Fixes for tc act_ct software offload of established flows (diff v4->v6)

[Paul Blakey]

Hi,

v4 of the original patchset was accidentally merged while we moved ahead
with v6 review. This two patches are the diff between v4 that was merged and
v6 that was the final revision, which was acked by the community.

Paul Blakey (2):
  net/sched: act_ct: Fix ipv6 lookup of offloaded connections
  net/sched: act_ct: Use pskb_network_may_pull()

 net/sched/act_ct.c | 64 ++++++++++++++++++++++++------------------------------
 1 file changed, 28 insertions(+), 36 deletions(-)

-- 
1.8.3.1

[PATCH net-next 1/2] net/sched: act_ct: Fix ipv6 lookup of offloaded connections

[Paul Blakey]

When checking the protocol number tcf_ct_flow_table_lookup() handles
the flow as if it's always ipv4, while it can be ipv6.

Instead, refactor the code to fetch the tcp header, if available,
in the relevant family (ipv4/ipv6) filler function, and do the
check on the returned tcp header.

Fixes: 46475bb20f4b ("net/sched: act_ct: Software offload of established flows")
Signed-off-by: Paul Blakey <paulb@mellanox.com>
---
 net/sched/act_ct.c | 60 +++++++++++++++++++++++-------------------------------
 1 file changed, 26 insertions(+), 34 deletions(-)

diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
index a2d5582..f434db7 100644
--- a/net/sched/act_ct.c
+++ b/net/sched/act_ct.c
@@ -188,7 +188,8 @@ static void tcf_ct_flow_table_process_conn(struct tcf_ct_flow_table *ct_ft,
 
 static bool
 tcf_ct_flow_table_fill_tuple_ipv4(struct sk_buff *skb,
-				  struct flow_offload_tuple *tuple)
+				  struct flow_offload_tuple *tuple,
+				  struct tcphdr **tcph)
 {
 	struct flow_ports *ports;
 	unsigned int thoff;
@@ -211,11 +212,16 @@ static void tcf_ct_flow_table_process_conn(struct tcf_ct_flow_table *ct_ft,
 	if (iph->ttl <= 1)
 		return false;
 
-	if (!pskb_may_pull(skb, thoff + sizeof(*ports)))
+	if (!pskb_may_pull(skb, iph->protocol == IPPROTO_TCP ?
+			   thoff + sizeof(struct tcphdr) :
+			   thoff + sizeof(*ports)))
 		return false;
 
-	ports = (struct flow_ports *)(skb_network_header(skb) + thoff);
+	iph = ip_hdr(skb);
+	if (iph->protocol == IPPROTO_TCP)
+		*tcph = (void *)(skb_network_header(skb) + thoff);
 
+	ports = (struct flow_ports *)(skb_network_header(skb) + thoff);
 	tuple->src_v4.s_addr = iph->saddr;
 	tuple->dst_v4.s_addr = iph->daddr;
 	tuple->src_port = ports->source;
@@ -228,7 +234,8 @@ static void tcf_ct_flow_table_process_conn(struct tcf_ct_flow_table *ct_ft,
 
 static bool
 tcf_ct_flow_table_fill_tuple_ipv6(struct sk_buff *skb,
-				  struct flow_offload_tuple *tuple)
+				  struct flow_offload_tuple *tuple,
+				  struct tcphdr **tcph)
 {
 	struct flow_ports *ports;
 	struct ipv6hdr *ip6h;
@@ -247,11 +254,16 @@ static void tcf_ct_flow_table_process_conn(struct tcf_ct_flow_table *ct_ft,
 		return false;
 
 	thoff = sizeof(*ip6h);
-	if (!pskb_may_pull(skb, thoff + sizeof(*ports)))
+	if (!pskb_may_pull(skb, ip6h->nexthdr == IPPROTO_TCP ?
+			   thoff + sizeof(struct tcphdr) :
+			   thoff + sizeof(*ports)))
 		return false;
 
-	ports = (struct flow_ports *)(skb_network_header(skb) + thoff);
+	ip6h = ipv6_hdr(skb);
+	if (ip6h->nexthdr == IPPROTO_TCP)
+		*tcph = (void *)(skb_network_header(skb) + thoff);
 
+	ports = (struct flow_ports *)(skb_network_header(skb) + thoff);
 	tuple->src_v6 = ip6h->saddr;
 	tuple->dst_v6 = ip6h->daddr;
 	tuple->src_port = ports->source;
@@ -262,24 +274,6 @@ static void tcf_ct_flow_table_process_conn(struct tcf_ct_flow_table *ct_ft,
 	return true;
 }
 
-static bool tcf_ct_flow_table_check_tcp(struct flow_offload *flow,
-					struct sk_buff *skb,
-					unsigned int thoff)
-{
-	struct tcphdr *tcph;
-
-	if (!pskb_may_pull(skb, thoff + sizeof(*tcph)))
-		return false;
-
-	tcph = (void *)(skb_network_header(skb) + thoff);
-	if (unlikely(tcph->fin || tcph->rst)) {
-		flow_offload_teardown(flow);
-		return false;
-	}
-
-	return true;
-}
-
 static bool tcf_ct_flow_table_lookup(struct tcf_ct_params *p,
 				     struct sk_buff *skb,
 				     u8 family)
@@ -288,10 +282,9 @@ static bool tcf_ct_flow_table_lookup(struct tcf_ct_params *p,
 	struct flow_offload_tuple_rhash *tuplehash;
 	struct flow_offload_tuple tuple = {};
 	enum ip_conntrack_info ctinfo;
+	struct tcphdr *tcph = NULL;
 	struct flow_offload *flow;
 	struct nf_conn *ct;
-	unsigned int thoff;
-	int ip_proto;
 	u8 dir;
 
 	/* Previously seen or loopback */
@@ -301,11 +294,11 @@ static bool tcf_ct_flow_table_lookup(struct tcf_ct_params *p,
 
 	switch (family) {
 	case NFPROTO_IPV4:
-		if (!tcf_ct_flow_table_fill_tuple_ipv4(skb, &tuple))
+		if (!tcf_ct_flow_table_fill_tuple_ipv4(skb, &tuple, &tcph))
 			return false;
 		break;
 	case NFPROTO_IPV6:
-		if (!tcf_ct_flow_table_fill_tuple_ipv6(skb, &tuple))
+		if (!tcf_ct_flow_table_fill_tuple_ipv6(skb, &tuple, &tcph))
 			return false;
 		break;
 	default:
@@ -320,15 +313,14 @@ static bool tcf_ct_flow_table_lookup(struct tcf_ct_params *p,
 	flow = container_of(tuplehash, struct flow_offload, tuplehash[dir]);
 	ct = flow->ct;
 
+	if (tcph && (unlikely(tcph->fin || tcph->rst))) {
+		flow_offload_teardown(flow);
+		return false;
+	}
+
 	ctinfo = dir == FLOW_OFFLOAD_DIR_ORIGINAL ? IP_CT_ESTABLISHED :
 						    IP_CT_ESTABLISHED_REPLY;
 
-	thoff = ip_hdr(skb)->ihl * 4;
-	ip_proto = ip_hdr(skb)->protocol;
-	if (ip_proto == IPPROTO_TCP &&
-	    !tcf_ct_flow_table_check_tcp(flow, skb, thoff))
-		return false;
-
 	nf_conntrack_get(&ct->ct_general);
 	nf_ct_set(skb, ct, ctinfo);
 
-- 
1.8.3.1

[PATCH net-next 2/2] net/sched: act_ct: Use pskb_network_may_pull()

[Paul Blakey]

To make the filler functions more generic, use network
relative skb pulling.

Signed-off-by: Paul Blakey <paulb@mellanox.com>
---
 net/sched/act_ct.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
index f434db7..23eba61 100644
--- a/net/sched/act_ct.c
+++ b/net/sched/act_ct.c
@@ -195,7 +195,7 @@ static void tcf_ct_flow_table_process_conn(struct tcf_ct_flow_table *ct_ft,
 	unsigned int thoff;
 	struct iphdr *iph;
 
-	if (!pskb_may_pull(skb, sizeof(*iph)))
+	if (!pskb_network_may_pull(skb, sizeof(*iph)))
 		return false;
 
 	iph = ip_hdr(skb);
@@ -212,9 +212,9 @@ static void tcf_ct_flow_table_process_conn(struct tcf_ct_flow_table *ct_ft,
 	if (iph->ttl <= 1)
 		return false;
 
-	if (!pskb_may_pull(skb, iph->protocol == IPPROTO_TCP ?
-			   thoff + sizeof(struct tcphdr) :
-			   thoff + sizeof(*ports)))
+	if (!pskb_network_may_pull(skb, iph->protocol == IPPROTO_TCP ?
+					thoff + sizeof(struct tcphdr) :
+					thoff + sizeof(*ports)))
 		return false;
 
 	iph = ip_hdr(skb);
@@ -241,7 +241,7 @@ static void tcf_ct_flow_table_process_conn(struct tcf_ct_flow_table *ct_ft,
 	struct ipv6hdr *ip6h;
 	unsigned int thoff;
 
-	if (!pskb_may_pull(skb, sizeof(*ip6h)))
+	if (!pskb_network_may_pull(skb, sizeof(*ip6h)))
 		return false;
 
 	ip6h = ipv6_hdr(skb);
@@ -254,9 +254,9 @@ static void tcf_ct_flow_table_process_conn(struct tcf_ct_flow_table *ct_ft,
 		return false;
 
 	thoff = sizeof(*ip6h);
-	if (!pskb_may_pull(skb, ip6h->nexthdr == IPPROTO_TCP ?
-			   thoff + sizeof(struct tcphdr) :
-			   thoff + sizeof(*ports)))
+	if (!pskb_network_may_pull(skb, ip6h->nexthdr == IPPROTO_TCP ?
+					thoff + sizeof(struct tcphdr) :
+					thoff + sizeof(*ports)))
 		return false;
 
 	ip6h = ipv6_hdr(skb);
-- 
1.8.3.1

Re: [PATCH net-next 1/2] net/sched: act_ct: Fix ipv6 lookup of offloaded connections

[Marcelo Ricardo Leitner]

On Wed, Mar 04, 2020 at 01:49:38PM +0200, Paul Blakey wrote:
> When checking the protocol number tcf_ct_flow_table_lookup() handles
> the flow as if it's always ipv4, while it can be ipv6.
> 
> Instead, refactor the code to fetch the tcp header, if available,
> in the relevant family (ipv4/ipv6) filler function, and do the
> check on the returned tcp header.
> 
> Fixes: 46475bb20f4b ("net/sched: act_ct: Software offload of established flows")
> Signed-off-by: Paul Blakey <paulb@mellanox.com>

Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

Re: [PATCH net-next 2/2] net/sched: act_ct: Use pskb_network_may_pull()

[Marcelo Ricardo Leitner]

On Wed, Mar 04, 2020 at 01:49:39PM +0200, Paul Blakey wrote:
> To make the filler functions more generic, use network
> relative skb pulling.
> 
> Signed-off-by: Paul Blakey <paulb@mellanox.com>

Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

Re: [PATCH net-next 1/2] net/sched: act_ct: Fix ipv6 lookup of offloaded connections

[Jiri Pirko]

Wed, Mar 04, 2020 at 12:49:38PM CET, paulb@mellanox.com wrote:
>When checking the protocol number tcf_ct_flow_table_lookup() handles
>the flow as if it's always ipv4, while it can be ipv6.
>
>Instead, refactor the code to fetch the tcp header, if available,
>in the relevant family (ipv4/ipv6) filler function, and do the
>check on the returned tcp header.
>
>Fixes: 46475bb20f4b ("net/sched: act_ct: Software offload of established flows")
>Signed-off-by: Paul Blakey <paulb@mellanox.com>

Reviewed-by: Jiri Pirko <jiri@mellanox.com>

Re: [PATCH net-next 2/2] net/sched: act_ct: Use pskb_network_may_pull()

[Jiri Pirko]

Wed, Mar 04, 2020 at 12:49:39PM CET, paulb@mellanox.com wrote:
>To make the filler functions more generic, use network
>relative skb pulling.
>
>Signed-off-by: Paul Blakey <paulb@mellanox.com>

Reviewed-by: Jiri Pirko <jiri@mellanox.com>

Re: [PATCH net-next 0/2] Fixes for tc act_ct software offload of established flows (diff v4->v6)

[David Miller]

From: Paul Blakey <paulb@mellanox.com>
Date: Wed,  4 Mar 2020 13:49:37 +0200

> v4 of the original patchset was accidentally merged while we moved ahead
> with v6 review. This two patches are the diff between v4 that was merged and
> v6 that was the final revision, which was acked by the community.

Series applied, thanks Paul.