Message ID | 1559774134-80336-1-git-send-email-dlu998@gmail.com |
---|---|
State | Accepted |
Commit | e32cd4c6292e81d047bafa882f0a1d1f3e7dc1f0 |
Headers | show |
Series | [ovs-dev,v5] conntrack: ignore port for ICMP/ICMPv6 NAT. | expand |
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() >
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.
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()