diff mbox series

[ovs-dev] ovs:conntrack: ignore port for icmp/icmpv6 protocol

Message ID 11b206a7-e805-ea30-6dca-c872b8b5f6e1@gmail.com
State Changes Requested
Headers show
Series [ovs-dev] ovs:conntrack: ignore port for icmp/icmpv6 protocol | expand

Commit Message

solomon May 30, 2019, 8:16 a.m. UTC 
conntrack will not work for icmp/icmpv6 protocol, if the src/dst port is set in nat.
like this:
actions=ct(nat(dst=172.16.1.100:5000),commit,table=40)

This patch fix this. This bug is introduced by commit 4cd0481c9e.

commit 4cd0481c9e8b30bca5c0394f4e94ae126bde4908
Author: Darrell Ball <dlu998@gmail.com>
Date:   Mon Feb 25 15:36:31 2019 -0800

    conntrack: Fix wasted work for ICMP NAT.


Signed-off-by: solomon <liwei.solomon@gmail.com>
---
 lib/conntrack.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

Darrell Ball May 30, 2019, 2:49 p.m. UTC | #1 
Thanks Solomon; fix looks good.

Can you:

1/ Change subject to:

 "conntrack: ignore port for ICMP/ICMPv6 NAT."

2/ Change commit message body to:

ICMP/ICMPv6 fails, if the src/dst port is set in a common NAT rule. like
this:
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>

3/ Add the following test with the same patch with added
co-author/signed-off tags

Signed-off-by: Darrell Ball <dlu998@gmail.com>
Co-authored-by: Darrell Ball <dlu998@gmail.com>

or if you prefer, I can add the test as a separate patch later without any
co-authors.
Either way is fine.

AT_SETUP([conntrack - SNAT with port range using ICMP])
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


On Thu, May 30, 2019 at 1:16 AM solomon <shanwei88@gmail.com> wrote:

> conntrack will not work for icmp/icmpv6 protocol, if the src/dst port is
> set in nat.
> like this:
> actions=ct(nat(dst=172.16.1.100:5000),commit,table=40)
>
> This patch fix this. This bug is introduced by commit 4cd0481c9e.
>
> commit 4cd0481c9e8b30bca5c0394f4e94ae126bde4908
> Author: Darrell Ball <dlu998@gmail.com>
> Date:   Mon Feb 25 15:36:31 2019 -0800
>
>     conntrack: Fix wasted work for ICMP NAT.
>
>
> Signed-off-by: solomon <liwei.solomon@gmail.com>
> ---
>  lib/conntrack.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index d7d48a43a..9d6b8a358 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -2039,10 +2039,14 @@ 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);
> +            }
>          }
>
>          uint32_t conn_hash = conn_key_hash(&nat_conn->rev_key,
> --
> 2.20.1
>
solomon May 31, 2019, 3:22 a.m. UTC | #2 
Thank you very much for your suggestions and testcases.
I sent a v2 patch，ref to https://mail.openvswitch.org/pipermail/ovs-dev/2019-May/359385.html


Darrell Ball wrote:
> Thanks Solomon; fix looks good.
> 
> Can you:
> 
> 1/ Change subject to:
> 
>  "conntrack: ignore port for ICMP/ICMPv6 NAT."
> 
> 2/ Change commit message body to:
> 
> ICMP/ICMPv6 fails, if the src/dst port is set in a common NAT rule. like
> this:
> 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>
> 
> 3/ Add the following test with the same patch with added
> co-author/signed-off tags
> 
> Signed-off-by: Darrell Ball <dlu998@gmail.com>
> Co-authored-by: Darrell Ball <dlu998@gmail.com>
> 
> or if you prefer, I can add the test as a separate patch later without any
> co-authors.
> Either way is fine.
> 
> AT_SETUP([conntrack - SNAT with port range using ICMP])
> 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
> 
> 
> On Thu, May 30, 2019 at 1:16 AM solomon <shanwei88@gmail.com> wrote:
> 
>> conntrack will not work for icmp/icmpv6 protocol, if the src/dst port is
>> set in nat.
>> like this:
>> actions=ct(nat(dst=172.16.1.100:5000),commit,table=40)
>>
>> This patch fix this. This bug is introduced by commit 4cd0481c9e.
>>
>> commit 4cd0481c9e8b30bca5c0394f4e94ae126bde4908
>> Author: Darrell Ball <dlu998@gmail.com>
>> Date:   Mon Feb 25 15:36:31 2019 -0800
>>
>>     conntrack: Fix wasted work for ICMP NAT.
>>
>>
>> Signed-off-by: solomon <liwei.solomon@gmail.com>
>> ---
>>  lib/conntrack.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/conntrack.c b/lib/conntrack.c
>> index d7d48a43a..9d6b8a358 100644
>> --- a/lib/conntrack.c
>> +++ b/lib/conntrack.c
>> @@ -2039,10 +2039,14 @@ 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);
>> +            }
>>          }
>>
>>          uint32_t conn_hash = conn_key_hash(&nat_conn->rev_key,
>> --
>> 2.20.1
>>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
diff mbox series

Patch

diff --git a/lib/conntrack.c b/lib/conntrack.c
index d7d48a43a..9d6b8a358 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -2039,10 +2039,14 @@  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);
+            }
         }
 
         uint32_t conn_hash = conn_key_hash(&nat_conn->rev_key,