[ovs-dev,v5] conntrack: ignore port for ICMP/ICMPv6 NAT.
diff mbox series

Message ID 1559774134-80336-1-git-send-email-dlu998@gmail.com
State New
Headers show
Series
  • [ovs-dev,v5] conntrack: ignore port for ICMP/ICMPv6 NAT.
Related show

Commit Message

Darrell Ball June 5, 2019, 10:35 p.m. UTC
From: solomon <liwei.solomon@gmail.com>

ICMP/ICMPv6 fails, if the src/dst port is set in a common NAT rule.
For example:
actions=ct(nat(dst=172.16.1.100:5000),commit,table=40)

Fixes: 4cd0481c9e8b ("conntrack: Fix wasted work for ICMP NAT.")
CC: Darrell Ball <dlu998@gmail.com>
Signed-off-by: solomon <liwei.solomon@gmail.com>
Signed-off-by: Darrell Ball <dlu998@gmail.com>
Co-authored-by: Darrell Ball <dlu998@gmail.com>
---

v5: Rebase
v4: Fix corrupted test.
v3: Add co-author tag.
v2: Add a test.

 lib/conntrack.c         | 12 ++++++++----
 tests/system-traffic.at | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+), 4 deletions(-)

Comments

solomon June 6, 2019, 12:55 a.m. UTC | #1
It looks good to meļ¼Œthank you.


Darrell Ball wrote:
> From: solomon <liwei.solomon@gmail.com>
> 
> ICMP/ICMPv6 fails, if the src/dst port is set in a common NAT rule.
> For example:
> actions=ct(nat(dst=172.16.1.100:5000),commit,table=40)
> 
> Fixes: 4cd0481c9e8b ("conntrack: Fix wasted work for ICMP NAT.")
> CC: Darrell Ball <dlu998@gmail.com>
> Signed-off-by: solomon <liwei.solomon@gmail.com>
> Signed-off-by: Darrell Ball <dlu998@gmail.com>
> Co-authored-by: Darrell Ball <dlu998@gmail.com>
> ---
> 
> v5: Rebase
> v4: Fix corrupted test.
> v3: Add co-author tag.
> v2: Add a test.
> 
>  lib/conntrack.c         | 12 ++++++++----
>  tests/system-traffic.at | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 56 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 67c3a58..5f60fea 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -2051,14 +2051,18 @@ nat_select_range_tuple(struct conntrack *ct, const struct conn *conn,
>      while (true) {
>          if (conn->nat_info->nat_action & NAT_ACTION_SRC) {
>              nat_conn->rev_key.dst.addr = ct_addr;
> -            nat_conn->rev_key.dst.port = htons(port);
> +            if (pat_enabled) {
> +                nat_conn->rev_key.dst.port = htons(port);
> +            }
>          } else {
>              nat_conn->rev_key.src.addr = ct_addr;
> -            nat_conn->rev_key.src.port = htons(port);
> +            if (pat_enabled) {
> +                nat_conn->rev_key.src.port = htons(port);
> +            }
>          }
>  
> -        bool found = conn_lookup(ct, &nat_conn->rev_key, time_msec(),
> -                                     NULL, NULL);
> +        bool found = conn_lookup(ct, &nat_conn->rev_key, time_msec(), NULL,
> +                                 NULL);
>          if (!found) {
>              return true;
>          } else if (pat_enabled && !all_ports_tried) {
> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> index 79d12fe..d23ee89 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -3994,6 +3994,54 @@ tcp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=<cleared>,dport=<cleared>),reply=(src=
>  OVS_TRAFFIC_VSWITCHD_STOP
>  AT_CLEANUP
>  
> +AT_SETUP([conntrack - SNAT with port range using ICMP])
> +dnl Check PAT is not attempted on ICMP packets causing corrupted packets.
> +CHECK_CONNTRACK()
> +CHECK_CONNTRACK_NAT()
> +OVS_TRAFFIC_VSWITCHD_START()
> +
> +ADD_NAMESPACES(at_ns0, at_ns1)
> +
> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
> +NS_CHECK_EXEC([at_ns0], [ip link set dev p0 address 80:88:88:88:88:88])
> +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
> +
> +dnl Allow any traffic from ns0->ns1. Only allow nd, return traffic from ns1->ns0.
> +AT_DATA([flows.txt], [dnl
> +in_port=1,ip,action=ct(commit,zone=1,nat(src=10.1.1.240-10.1.1.255:20000)),2
> +in_port=2,ct_state=-trk,ip,action=ct(table=0,zone=1,nat)
> +in_port=2,ct_state=+trk,ct_zone=1,action=1
> +dnl
> +dnl ARP
> +priority=100 arp arp_op=1 action=move:OXM_OF_ARP_TPA[[]]->NXM_NX_REG2[[]],resubmit(,8),goto_table:10
> +priority=10 arp action=normal
> +priority=0,action=drop
> +dnl
> +dnl MAC resolution table for IP in reg2, stores mac in OXM_OF_PKT_REG0
> +table=8,reg2=0x0a0101f0/0xfffffff0,action=load:0x808888888888->OXM_OF_PKT_REG0[[]]
> +table=8,priority=0,action=load:0->OXM_OF_PKT_REG0[[]]
> +dnl ARP responder mac filled in at OXM_OF_PKT_REG0, or 0 for normal action.
> +dnl TPA IP in reg2.
> +dnl Swaps the fields of the ARP message to turn a query to a response.
> +table=10 priority=100 arp xreg0=0 action=normal
> +table=10 priority=10,arp,arp_op=1,action=load:2->OXM_OF_ARP_OP[[]],move:OXM_OF_ARP_SHA[[]]->OXM_OF_ARP_THA[[]],move:OXM_OF_PKT_REG0[[0..47]]->OXM_OF_ARP_SHA[[]],move:OXM_OF_ARP_SPA[[]]->OXM_OF_ARP_TPA[[]],move:NXM_NX_REG2[[]]->OXM_OF_ARP_SPA[[]],move:NXM_OF_ETH_SRC[[]]->NXM_OF_ETH_DST[[]],move:OXM_OF_PKT_REG0[[0..47]]->NXM_OF_ETH_SRC[[]],move:NXM_OF_IN_PORT[[]]->NXM_NX_REG3[[0..15]],load:0->NXM_OF_IN_PORT[[]],output:NXM_NX_REG3[[0..15]]
> +table=10 priority=0 action=drop
> +])
> +
> +AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
> +
> +dnl ICMP requests from p0->p1 should work fine.
> +NS_CHECK_EXEC([at_ns0], [ping -c 1 10.1.1.2 | FORMAT_PING], [0], [dnl
> +1 packets transmitted, 1 received, 0% packet loss, time 0ms
> +])
> +
> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2) | sed -e 's/dst=10.1.1.2[[45]][[0-9]]/dst=10.1.1.2XX/'], [0], [dnl
> +icmp,orig=(src=10.1.1.1,dst=10.1.1.2,id=<cleared>,type=8,code=0),reply=(src=10.1.1.2,dst=10.1.1.2XX,id=<cleared>,type=0,code=0),zone=1
> +])
> +
> +OVS_TRAFFIC_VSWITCHD_STOP
> +AT_CLEANUP
> +
>  AT_SETUP([conntrack - SNAT with port range with exhaustion])
>  CHECK_CONNTRACK()
>  CHECK_CONNTRACK_NAT()
>
Ben Pfaff June 7, 2019, 6:30 p.m. UTC | #2
On Wed, Jun 05, 2019 at 03:35:34PM -0700, Darrell Ball wrote:
> From: solomon <liwei.solomon@gmail.com>
> 
> ICMP/ICMPv6 fails, if the src/dst port is set in a common NAT rule.
> For example:
> actions=ct(nat(dst=172.16.1.100:5000),commit,table=40)
> 
> Fixes: 4cd0481c9e8b ("conntrack: Fix wasted work for ICMP NAT.")
> CC: Darrell Ball <dlu998@gmail.com>
> Signed-off-by: solomon <liwei.solomon@gmail.com>
> Signed-off-by: Darrell Ball <dlu998@gmail.com>
> Co-authored-by: Darrell Ball <dlu998@gmail.com>

Thanks, solomon and Darrell.  I applied this to master.

Patch
diff mbox series

diff --git a/lib/conntrack.c b/lib/conntrack.c
index 67c3a58..5f60fea 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -2051,14 +2051,18 @@  nat_select_range_tuple(struct conntrack *ct, const struct conn *conn,
     while (true) {
         if (conn->nat_info->nat_action & NAT_ACTION_SRC) {
             nat_conn->rev_key.dst.addr = ct_addr;
-            nat_conn->rev_key.dst.port = htons(port);
+            if (pat_enabled) {
+                nat_conn->rev_key.dst.port = htons(port);
+            }
         } else {
             nat_conn->rev_key.src.addr = ct_addr;
-            nat_conn->rev_key.src.port = htons(port);
+            if (pat_enabled) {
+                nat_conn->rev_key.src.port = htons(port);
+            }
         }
 
-        bool found = conn_lookup(ct, &nat_conn->rev_key, time_msec(),
-                                     NULL, NULL);
+        bool found = conn_lookup(ct, &nat_conn->rev_key, time_msec(), NULL,
+                                 NULL);
         if (!found) {
             return true;
         } else if (pat_enabled && !all_ports_tried) {
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 79d12fe..d23ee89 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -3994,6 +3994,54 @@  tcp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=<cleared>,dport=<cleared>),reply=(src=
 OVS_TRAFFIC_VSWITCHD_STOP
 AT_CLEANUP
 
+AT_SETUP([conntrack - SNAT with port range using ICMP])
+dnl Check PAT is not attempted on ICMP packets causing corrupted packets.
+CHECK_CONNTRACK()
+CHECK_CONNTRACK_NAT()
+OVS_TRAFFIC_VSWITCHD_START()
+
+ADD_NAMESPACES(at_ns0, at_ns1)
+
+ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
+NS_CHECK_EXEC([at_ns0], [ip link set dev p0 address 80:88:88:88:88:88])
+ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
+
+dnl Allow any traffic from ns0->ns1. Only allow nd, return traffic from ns1->ns0.
+AT_DATA([flows.txt], [dnl
+in_port=1,ip,action=ct(commit,zone=1,nat(src=10.1.1.240-10.1.1.255:20000)),2
+in_port=2,ct_state=-trk,ip,action=ct(table=0,zone=1,nat)
+in_port=2,ct_state=+trk,ct_zone=1,action=1
+dnl
+dnl ARP
+priority=100 arp arp_op=1 action=move:OXM_OF_ARP_TPA[[]]->NXM_NX_REG2[[]],resubmit(,8),goto_table:10
+priority=10 arp action=normal
+priority=0,action=drop
+dnl
+dnl MAC resolution table for IP in reg2, stores mac in OXM_OF_PKT_REG0
+table=8,reg2=0x0a0101f0/0xfffffff0,action=load:0x808888888888->OXM_OF_PKT_REG0[[]]
+table=8,priority=0,action=load:0->OXM_OF_PKT_REG0[[]]
+dnl ARP responder mac filled in at OXM_OF_PKT_REG0, or 0 for normal action.
+dnl TPA IP in reg2.
+dnl Swaps the fields of the ARP message to turn a query to a response.
+table=10 priority=100 arp xreg0=0 action=normal
+table=10 priority=10,arp,arp_op=1,action=load:2->OXM_OF_ARP_OP[[]],move:OXM_OF_ARP_SHA[[]]->OXM_OF_ARP_THA[[]],move:OXM_OF_PKT_REG0[[0..47]]->OXM_OF_ARP_SHA[[]],move:OXM_OF_ARP_SPA[[]]->OXM_OF_ARP_TPA[[]],move:NXM_NX_REG2[[]]->OXM_OF_ARP_SPA[[]],move:NXM_OF_ETH_SRC[[]]->NXM_OF_ETH_DST[[]],move:OXM_OF_PKT_REG0[[0..47]]->NXM_OF_ETH_SRC[[]],move:NXM_OF_IN_PORT[[]]->NXM_NX_REG3[[0..15]],load:0->NXM_OF_IN_PORT[[]],output:NXM_NX_REG3[[0..15]]
+table=10 priority=0 action=drop
+])
+
+AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
+
+dnl ICMP requests from p0->p1 should work fine.
+NS_CHECK_EXEC([at_ns0], [ping -c 1 10.1.1.2 | FORMAT_PING], [0], [dnl
+1 packets transmitted, 1 received, 0% packet loss, time 0ms
+])
+
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2) | sed -e 's/dst=10.1.1.2[[45]][[0-9]]/dst=10.1.1.2XX/'], [0], [dnl
+icmp,orig=(src=10.1.1.1,dst=10.1.1.2,id=<cleared>,type=8,code=0),reply=(src=10.1.1.2,dst=10.1.1.2XX,id=<cleared>,type=0,code=0),zone=1
+])
+
+OVS_TRAFFIC_VSWITCHD_STOP
+AT_CLEANUP
+
 AT_SETUP([conntrack - SNAT with port range with exhaustion])
 CHECK_CONNTRACK()
 CHECK_CONNTRACK_NAT()