diff mbox series

[ovs-dev,v2] conntrack: fix incorrect check nat_action_info in check_orig_tuple

Message ID 1625809032-436-1-git-send-email-wenxu@ucloud.cn
State Deferred
Headers show
Series [ovs-dev,v2] conntrack: fix incorrect check nat_action_info in check_orig_tuple | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot success github build: passed

Commit Message

wenxu July 9, 2021, 5:37 a.m. UTC 
From: wenxu <wenxu@ucloud.cn>

A case for client A 10.0.0.2 snat to 1.1.1.2 with following flows.

rule1: ovs-ofctl add-flow manbr "table=0,ct_state=-trk,ip,in_port=dpdk2, actions=ct(table=1, nat)"
rule2: ovs-ofctl add-flow manbr "table=0,table=1,ct_state=+trk+new,ip,in_port=dpdk2, actions=ct(commit, nat(src=1.1.1.2)),dpdk3"

When client A tcp connect to a non-exist server 1.1.1.3

The first syn packet will create the following conntrack 1
But the second syn packet will wrongly create the conntrack 2

tcp,orig=(src=10.0.0.2,dst=1.1.1.3,sport=15690,dport=5001),reply=(src=1.1.1.3,dst=1.1.1.2,sport=5001,dport=15690),protoinfo=(state=SYN_SENT)   #conntrack 1
tcp,orig=(src=1.1.1.2,dst=1.1.1.3,sport=15690,dport=5001),reply=(src=1.1.1.3,dst=1.1.1.2,sport=5001,dport=1024),protoinfo=(state=SYN_SENT)     #conntrack 2

The second syn packet gothrough rule1 and find the conntrack1 and
do nat. Then gothrough the rule2 will not find the only conntrack
for packet nated in the rule1
The check_orig_tuple is used to fix for this situation(packet
nated in the first rule). But it should't check the nat_action_info
in the rule2. It should only check the CS_SRC_NAT and CS_DST_NAT.

Signed-off-by: wenxu <wenxu@ucloud.cn>
---
 lib/conntrack.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

Comments

Simon Horman Oct. 9, 2023, 10:15 a.m. UTC | #1 
On Fri, Jul 09, 2021 at 01:37:12PM +0800, wenxu@ucloud.cn wrote:
> From: wenxu <wenxu@ucloud.cn>
> 
> A case for client A 10.0.0.2 snat to 1.1.1.2 with following flows.
> 
> rule1: ovs-ofctl add-flow manbr "table=0,ct_state=-trk,ip,in_port=dpdk2, actions=ct(table=1, nat)"
> rule2: ovs-ofctl add-flow manbr "table=0,table=1,ct_state=+trk+new,ip,in_port=dpdk2, actions=ct(commit, nat(src=1.1.1.2)),dpdk3"
> 
> When client A tcp connect to a non-exist server 1.1.1.3
> 
> The first syn packet will create the following conntrack 1
> But the second syn packet will wrongly create the conntrack 2
> 
> tcp,orig=(src=10.0.0.2,dst=1.1.1.3,sport=15690,dport=5001),reply=(src=1.1.1.3,dst=1.1.1.2,sport=5001,dport=15690),protoinfo=(state=SYN_SENT)   #conntrack 1
> tcp,orig=(src=1.1.1.2,dst=1.1.1.3,sport=15690,dport=5001),reply=(src=1.1.1.3,dst=1.1.1.2,sport=5001,dport=1024),protoinfo=(state=SYN_SENT)     #conntrack 2
> 
> The second syn packet gothrough rule1 and find the conntrack1 and
> do nat. Then gothrough the rule2 will not find the only conntrack
> for packet nated in the rule1
> The check_orig_tuple is used to fix for this situation(packet
> nated in the first rule). But it should't check the nat_action_info
> in the rule2. It should only check the CS_SRC_NAT and CS_DST_NAT.
> 
> Signed-off-by: wenxu <wenxu@ucloud.cn>

Hi Wenxu,

This patch appears to have gone stale in patchwork, for one reason or
another. If it is still relevant then I think it needs to be revisited,
by being reposted after appropriate preparation.

As such I'm marking this patch as "Deferred" in patchwork.

No action is required unless there is a desire to revisit this patch.
diff mbox series

Patch

diff --git a/lib/conntrack.c b/lib/conntrack.c
index 2e803ca..ace3e9a 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -1156,15 +1156,13 @@  handle_nat(struct dp_packet *pkt, struct conn *conn,
 static bool
 check_orig_tuple(struct conntrack *ct, struct dp_packet *pkt,
                  struct conn_lookup_ctx *ctx_in, long long now,
-                 struct conn **conn,
-                 const struct nat_action_info_t *nat_action_info)
+                 struct conn **conn)
 {
     if (!(pkt->md.ct_state & (CS_SRC_NAT | CS_DST_NAT)) ||
         (ctx_in->key.dl_type == htons(ETH_TYPE_IP) &&
          !pkt->md.ct_orig_tuple.ipv4.ipv4_proto) ||
         (ctx_in->key.dl_type == htons(ETH_TYPE_IPV6) &&
-         !pkt->md.ct_orig_tuple.ipv6.ipv6_proto) ||
-        nat_action_info) {
+         !pkt->md.ct_orig_tuple.ipv6.ipv6_proto)) {
         return false;
     }
 
@@ -1343,7 +1341,7 @@  process_one(struct conntrack *ct, struct dp_packet *pkt,
             handle_nat(pkt, conn, zone, ctx->reply, ctx->icmp_related);
         }
 
-    } else if (check_orig_tuple(ct, pkt, ctx, now, &conn, nat_action_info)) {
+    } else if (check_orig_tuple(ct, pkt, ctx, now, &conn)) {
         create_new_conn = conn_update_state(ct, pkt, ctx, conn, now);
     } else {
         if (ctx->icmp_related) {