Fixed a bug introduced in V3 and added a second patch and sent V4. i.e. V3 is superceded
On Mon, Jan 21, 2019 at 4:22 PM Darrell Ball <[email protected]> wrote: > Ignore V2; I sent V3 with more improvements. > > On Mon, Jan 21, 2019 at 12:12 PM Darrell Ball <[email protected]> wrote: > >> From: David Marchand <[email protected]> >> >> When replacing the ipv4 address in repl_ftp_v4_addr(), the remaining size >> was incorrectly calculated which could lead to the wrong replacement >> adjustment. >> >> This goes unnoticed most of the time, unless you choose carefully your >> initial and replacement addresses. >> >> Example fail address combination with 10.1.1.200 DNAT'd to 10.1.100.1. >> >> Fix this by getting rid of remain_size since it is a derived variable >> and use the primary variable byte_str instead. >> >> A test is updated to exercise different initial and replacement addresses >> and another test is added. >> >> Fixes: bd5e81a0e596 ("Userspace Datapath: Add ALG infra and FTP.") >> Co-authored-by: Darrell Ball <[email protected]> >> Signed-off-by: Darrell Ball <[email protected]> >> Signed-off-by: David Marchand <[email protected]> >> --- >> lib/conntrack.c | 8 ++---- >> tests/system-traffic.at | 74 >> ++++++++++++++++++++++++++++++++++++++++++++----- >> 2 files changed, 70 insertions(+), 12 deletions(-) >> >> diff --git a/lib/conntrack.c b/lib/conntrack.c >> index f732b9e..f1e6ee2 100644 >> --- a/lib/conntrack.c >> +++ b/lib/conntrack.c >> @@ -2806,8 +2806,6 @@ repl_ftp_v4_addr(struct dp_packet *pkt, ovs_be32 >> v4_addr_rep, >> return 0; >> } >> >> - size_t remain_size = tcp_payload_length(pkt) - >> - addr_offset_from_ftp_data_start; >> int overall_delta = 0; >> char *byte_str = ftp_data_start + addr_offset_from_ftp_data_start; >> >> @@ -2817,13 +2815,13 @@ repl_ftp_v4_addr(struct dp_packet *pkt, ovs_be32 >> v4_addr_rep, >> char *next_delim = memchr(byte_str, ',', 4); >> ovs_assert(next_delim); >> int substr_size = next_delim - byte_str; >> - remain_size -= substr_size; >> >> /* Compose the new string for this octet, and replace it. */ >> - char rep_str[4]; >> uint8_t rep_byte = get_v4_byte_be(v4_addr_rep, i); >> + char rep_str[4]; >> int replace_size = sprintf(rep_str, "%d", rep_byte); >> - replace_substring(byte_str, substr_size, remain_size, >> + replace_substring(byte_str, substr_size, >> + (const char *) dp_packet_tail(pkt) - byte_str, >> rep_str, replace_size); >> overall_delta += replace_size - substr_size; >> >> diff --git a/tests/system-traffic.at b/tests/system-traffic.at >> index 3a62e17..3e3b9e7 100644 >> --- a/tests/system-traffic.at >> +++ b/tests/system-traffic.at >> @@ -4652,6 +4652,66 @@ >> 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 - IPv4 FTP Passive with DNAT 2]) >> +AT_SKIP_IF([test $HAVE_FTP = no]) >> +CHECK_CONNTRACK() >> +CHECK_CONNTRACK_NAT() >> +CHECK_CONNTRACK_ALG() >> + >> +OVS_TRAFFIC_VSWITCHD_START() >> + >> +ADD_NAMESPACES(at_ns0, at_ns1) >> + >> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/16") >> +NS_CHECK_EXEC([at_ns0], [ip link set dev p0 address e6:66:c1:11:11:11]) >> +NS_CHECK_EXEC([at_ns0], [arp -s 10.1.1.200 e6:66:c1:22:22:22]) >> +NS_CHECK_EXEC([at_ns0], [arp -s 10.1.100.1 e6:66:c1:22:22:22]) >> + >> +ADD_VETH(p1, at_ns1, br0, "10.1.100.1/16") >> +NS_CHECK_EXEC([at_ns1], [ip link set dev p1 address e6:66:c1:22:22:22]) >> +NS_CHECK_EXEC([at_ns1], [arp -s 10.1.1.1 e6:66:c1:11:11:11]) >> + >> +dnl Allow any traffic from ns0->ns1. >> +AT_DATA([flows.txt], [dnl >> +dnl track all IPv4 traffic and NAT any established traffic. >> +table=0 priority=10 ip, action=ct(nat,table=1) >> +table=0 priority=0 action=drop >> +dnl >> +dnl Table 1 >> +dnl >> +dnl Allow new FTP control connections. >> +table=1 in_port=1 ct_state=+new tcp nw_src=10.1.1.1 tp_dst=21 >> action=ct(alg=ftp,commit,nat(dst=10.1.100.1)),2 >> +dnl Allow related TCP connections from port 1. >> +table=1 in_port=1 ct_state=+new+rel tcp nw_src=10.1.1.1 >> action=ct(commit,nat),2 >> +dnl Allow established TCP connections both ways, post-NAT match. >> +table=1 in_port=1 ct_state=+est tcp nw_dst=10.1.100.1 action=2 >> +table=1 in_port=2 ct_state=+est tcp nw_dst=10.1.1.1 action=1 >> + >> +dnl Allow ICMP both ways. >> +table=1 priority=100 in_port=1 icmp, action=2 >> +table=1 priority=100 in_port=2 icmp, action=1 >> +table=1 priority=0, action=drop >> +]) >> + >> +AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt]) >> + >> +dnl Check that the stacks working to avoid races. >> +OVS_WAIT_UNTIL([ip netns exec at_ns0 ping -c 1 10.1.100.1 >/dev/null]) >> + >> +OVS_START_L7([at_ns1], [ftp]) >> + >> +dnl FTP requests from p0->p1 should work fine. >> +NS_CHECK_EXEC([at_ns0], [wget ftp://10.1.1.200 -t 3 -T 1 >> --retry-connrefused -v -o wget0.log]) >> + >> +dnl Discards CLOSE_WAIT and CLOSING >> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.200)], [0], >> [dnl >> >> +tcp,orig=(src=10.1.1.1,dst=10.1.1.200,sport=<cleared>,dport=<cleared>),reply=(src=10.1.100.1,dst=10.1.1.1,sport=<cleared>,dport=<cleared>),protoinfo=(state=<cleared>) >> >> +tcp,orig=(src=10.1.1.1,dst=10.1.1.200,sport=<cleared>,dport=<cleared>),reply=(src=10.1.100.1,dst=10.1.1.1,sport=<cleared>,dport=<cleared>),protoinfo=(state=<cleared>),helper=ftp >> +]) >> + >> +OVS_TRAFFIC_VSWITCHD_STOP >> +AT_CLEANUP >> + >> AT_SETUP([conntrack - IPv4 FTP Active with DNAT]) >> AT_SKIP_IF([test $HAVE_FTP = no]) >> CHECK_CONNTRACK() >> @@ -4722,12 +4782,12 @@ OVS_TRAFFIC_VSWITCHD_START() >> >> ADD_NAMESPACES(at_ns0, at_ns1) >> >> -ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24") >> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/16") >> NS_CHECK_EXEC([at_ns0], [ip link set dev p0 address e6:66:c1:11:11:11]) >> NS_CHECK_EXEC([at_ns0], [arp -s 10.1.1.2 e6:66:c1:22:22:22]) >> -NS_CHECK_EXEC([at_ns0], [arp -s 10.1.1.240 e6:66:c1:22:22:22]) >> +NS_CHECK_EXEC([at_ns0], [arp -s 10.1.120.240 e6:66:c1:22:22:22]) >> >> -ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24") >> +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/16") >> NS_CHECK_EXEC([at_ns1], [ip link set dev p1 address e6:66:c1:22:22:22]) >> NS_CHECK_EXEC([at_ns1], [arp -s 10.1.1.1 e6:66:c1:11:11:11]) >> >> @@ -4761,12 +4821,12 @@ OVS_WAIT_UNTIL([ip netns exec at_ns0 ping -c 1 >> 10.1.1.2 >/dev/null]) >> OVS_START_L7([at_ns1], [ftp]) >> >> dnl FTP requests from p0->p1 should work fine. >> -NS_CHECK_EXEC([at_ns0], [wget ftp://10.1.1.240 --no-passive-ftp -t 3 -T >> 1 --retry-connrefused -v -o wget0.log]) >> +NS_CHECK_EXEC([at_ns0], [wget ftp://10.1.120.240 --no-passive-ftp -t 3 >> -T 1 --retry-connrefused -v -o wget0.log]) >> >> dnl Discards CLOSE_WAIT and CLOSING >> -AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2)], [0], >> [dnl >> >> -tcp,orig=(src=10.1.1.1,dst=10.1.1.240,sport=<cleared>,dport=<cleared>),reply=(src=10.1.1.2,dst=10.1.1.1,sport=<cleared>,dport=<cleared>),protoinfo=(state=<cleared>),helper=ftp >> >> -tcp,orig=(src=10.1.1.2,dst=10.1.1.1,sport=<cleared>,dport=<cleared>),reply=(src=10.1.1.1,dst=10.1.1.240,sport=<cleared>,dport=<cleared>),protoinfo=(state=<cleared>) >> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.120.240)], >> [0], [dnl >> >> +tcp,orig=(src=10.1.1.1,dst=10.1.120.240,sport=<cleared>,dport=<cleared>),reply=(src=10.1.1.2,dst=10.1.1.1,sport=<cleared>,dport=<cleared>),protoinfo=(state=<cleared>),helper=ftp >> >> +tcp,orig=(src=10.1.1.2,dst=10.1.1.1,sport=<cleared>,dport=<cleared>),reply=(src=10.1.1.1,dst=10.1.120.240,sport=<cleared>,dport=<cleared>),protoinfo=(state=<cleared>) >> ]) >> >> OVS_TRAFFIC_VSWITCHD_STOP >> -- >> 1.9.1 >> >> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
