| Message ID | 1547047989-28356-2-git-send-email-david.marchand@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fixes for ftp alg in userspace dp | expand |
Thanks for the patch On Wed, Jan 9, 2019 at 7:33 AM David Marchand <david.marchand@redhat.com> wrote: The ftp alg deals with packets in two ways for the command connection: > either they are inspected and can be mangled when nat is enabled > (CT_FTP_CTL_INTEREST) or they just go through without being modified > (CT_FTP_CTL_OTHER). > > For CT_FTP_CTL_INTEREST packets, we must both adjust the packet tcp seq > number by the connection current offset, then prepare for the next > packets by setting an accumulated offset in the ct object. > The tests are updated so that ftp+NAT checks send multiple commands in a > single tcp command connection: > > 1/ Please update the commit message for the 2nd and 3rd paragraphs, including use case and a correction. The ftp alg deals with packets in two ways for the command connection: either they are inspected and can be mangled when nat is enabled (CT_FTP_CTL_INTEREST) or they just go through without being modified (CT_FTP_CTL_OTHER). For CT_FTP_CTL_INTEREST packets, we must both adjust the packet tcp seq number by the connection current offset, then prepare for the next packets by setting an accumulated offset in the ct object. However, this was not done for multiple CT_FTP_CTL_INTEREST packets for the same connection. This is relevant for handling multiple child data connections that also need natting. The tests are updated so that some ftp+NAT tests send multiple port commands or other similar commands for a single control connection. Wget is not able to do this, so switch to lftp. 2/ A couple conntrack.c comments. 3/ I requested changing the number of 'ls' commands below, since it is a stronger test. 4/ I added another test below for reverse skew. 5/ Please consider adding me as co-author > Signed-off-by: David Marchand <david.marchand@redhat.com> > --- > Changelog since v2: > - skip tests relying on lftp when absent > - removed unneeded temp seq_skew variable by moving conn_seq_skew_set > at the end of the function and rely on the value in the conn object, > then removed unneeded do_seq_skew_adj > > --- > Vagrantfile | 9 ++++--- > Vagrantfile-FreeBSD | 2 +- > lib/conntrack.c | 69 > ++++++++++++++++++++++++------------------------- > tests/atlocal.in | 3 +++ > tests/system-traffic.at | 12 ++++++++- > 5 files changed, 55 insertions(+), 40 deletions(-) > > diff --git a/Vagrantfile b/Vagrantfile > index 0192f66..fbd772a 100644 > --- a/Vagrantfile > +++ b/Vagrantfile > @@ -12,7 +12,8 @@ dnf -y install autoconf automake openssl-devel libtool \ > python-twisted python-zope-interface \ > desktop-file-utils groff graphviz rpmdevtools nc curl \ > wget python-six pyftpdlib checkpolicy selinux-policy-devel > \ > - libcap-ng-devel kernel-devel-`uname -r` ethtool > python-tftpy > + libcap-ng-devel kernel-devel-`uname -r` ethtool > python-tftpy \ > + lftp > echo "search extra update built-in" >/etc/depmod.d/search_path.conf > SCRIPT > > @@ -28,7 +29,8 @@ aptitude -y install -R \ > wget python-six ethtool \ > libcap-ng-dev libssl-dev python-dev openssl \ > python-pyftpdlib python-flake8 python-tftpy \ > - linux-headers-`uname -r` > + linux-headers-`uname -r` \ > + lftp > SCRIPT > > $bootstrap_centos = <<SCRIPT > @@ -37,7 +39,8 @@ yum -y install autoconf automake openssl-devel libtool \ > python-twisted-core python-zope-interface \ > desktop-file-utils groff graphviz rpmdevtools nc curl \ > wget python-six pyftpdlib checkpolicy selinux-policy-devel > \ > - libcap-ng-devel kernel-devel-`uname -r` ethtool net-tools > + libcap-ng-devel kernel-devel-`uname -r` ethtool net-tools \ > + lftp > SCRIPT > > $configure_ovs = <<SCRIPT > diff --git a/Vagrantfile-FreeBSD b/Vagrantfile-FreeBSD > index 8f00abe..52599ee 100644 > --- a/Vagrantfile-FreeBSD > +++ b/Vagrantfile-FreeBSD > @@ -12,7 +12,7 @@ Vagrant.require_version ">=1.7.0" > $bootstrap_freebsd = <<SCRIPT > sed -e 's/\#DEFAULT_ALWAYS_YES = false/DEFAULT_ALWAYS_YES = true/g' -e > 's/\#ASSUME_ALWAYS_YES = false/ASSUME_ALWAYS_YES = true/g' > /usr/local/etc/pkg.conf > /tmp/pkg.conf > mv -f /tmp/pkg.conf /usr/local/etc/pkg.conf > -pkg install automake libtool wget python py27-six gmake > +pkg install automake libtool wget python py27-six gmake lftp > SCRIPT > > $configure_ovs = <<SCRIPT > diff --git a/lib/conntrack.c b/lib/conntrack.c > index 6f6021a..4717950 100644 > --- a/lib/conntrack.c > +++ b/lib/conntrack.c > @@ -3170,9 +3170,8 @@ repl_ftp_v6_addr(struct dp_packet *pkt, struct > ct_addr v6_addr_rep, > > static void > handle_ftp_ctl(struct conntrack *ct, const struct conn_lookup_ctx *ctx, > - struct dp_packet *pkt, > - const struct conn *conn_for_expectation, > - long long now, enum ftp_ctl_pkt ftp_ctl, bool nat) > + struct dp_packet *pkt, const struct conn *ec, long long > now, > + enum ftp_ctl_pkt ftp_ctl, bool nat) > { > struct ip_header *l3_hdr = dp_packet_l3(pkt); > ovs_be32 v4_addr_rep = 0; > @@ -3180,31 +3179,24 @@ handle_ftp_ctl(struct conntrack *ct, const struct > conn_lookup_ctx *ctx, > size_t addr_offset_from_ftp_data_start = 0; > size_t addr_size = 0; > char *ftp_data_start; > - bool do_seq_skew_adj = true; > enum ct_alg_mode mode = CT_FTP_MODE_ACTIVE; > > if (detect_ftp_ctl_type(ctx, pkt) != ftp_ctl) { > return; > } > > - if (!nat || !conn_for_expectation->seq_skew) { > - do_seq_skew_adj = false; > - } > - > struct ovs_16aligned_ip6_hdr *nh6 = dp_packet_l3(pkt); > int64_t seq_skew = 0; > > - if (ftp_ctl == CT_FTP_CTL_OTHER) { > - seq_skew = conn_for_expectation->seq_skew; > - } else if (ftp_ctl == CT_FTP_CTL_INTEREST) { > + if (ftp_ctl == CT_FTP_CTL_INTEREST) { > enum ftp_ctl_pkt rc; > if (ctx->key.dl_type == htons(ETH_TYPE_IPV6)) { > - rc = process_ftp_ctl_v6(ct, pkt, conn_for_expectation, > + rc = process_ftp_ctl_v6(ct, pkt, ec, > &v6_addr_rep, &ftp_data_start, > &addr_offset_from_ftp_data_start, > &addr_size, &mode); > } else { > - rc = process_ftp_ctl_v4(ct, pkt, conn_for_expectation, > + rc = process_ftp_ctl_v4(ct, pkt, ec, > &v4_addr_rep, &ftp_data_start, > &addr_offset_from_ftp_data_start); > } > @@ -3217,62 +3209,64 @@ handle_ftp_ctl(struct conntrack *ct, const struct > conn_lookup_ctx *ctx, > uint16_t ip_len; > > if (ctx->key.dl_type == htons(ETH_TYPE_IPV6)) { > - seq_skew = repl_ftp_v6_addr(pkt, v6_addr_rep, > ftp_data_start, > - > addr_offset_from_ftp_data_start, > - addr_size, mode); > + if (nat) { > + seq_skew = repl_ftp_v6_addr(pkt, v6_addr_rep, > + ftp_data_start, > + addr_offset_from_ftp_data_start, > + addr_size, mode); > + } > + > if (seq_skew) { > ip_len = ntohs(nh6->ip6_ctlun.ip6_un1.ip6_un1_plen); > ip_len += seq_skew; > Please compress to ip_len = ntohs(nh6->ip6_ctlun.ip6_un1.ip6_un1_plen) + seq_skew; > nh6->ip6_ctlun.ip6_un1.ip6_un1_plen = htons(ip_len); > - conn_seq_skew_set(ct, &conn_for_expectation->key, now, > - seq_skew, ctx->reply); > } > } else { > - seq_skew = repl_ftp_v4_addr(pkt, v4_addr_rep, > ftp_data_start, > - > addr_offset_from_ftp_data_start); > - ip_len = ntohs(l3_hdr->ip_tot_len); > + if (nat) { > + seq_skew = repl_ftp_v4_addr(pkt, v4_addr_rep, > + ftp_data_start, > + addr_offset_from_ftp_data_start); > + } > if (seq_skew) { > + ip_len = ntohs(l3_hdr->ip_tot_len); > ip_len += seq_skew; > Please compress to: ip_len = ntohs(l3_hdr->ip_tot_len) + seq_skew; > l3_hdr->ip_csum = recalc_csum16(l3_hdr->ip_csum, > l3_hdr->ip_tot_len, > htons(ip_len)); > l3_hdr->ip_tot_len = htons(ip_len); > - conn_seq_skew_set(ct, &conn_for_expectation->key, now, > - seq_skew, ctx->reply); > } > } > } else { > OVS_NOT_REACHED(); > } > - } else { > - OVS_NOT_REACHED(); > } > > struct tcp_header *th = dp_packet_l4(pkt); > > - if (do_seq_skew_adj && seq_skew != 0) { > - if (ctx->reply != conn_for_expectation->seq_skew_dir) { > + if (nat && ec->seq_skew != 0) { > + if (ctx->reply != ec->seq_skew_dir) { > > uint32_t tcp_ack = ntohl(get_16aligned_be32(&th->tcp_ack)); > > - if ((seq_skew > 0) && (tcp_ack < seq_skew)) { > + if ((ec->seq_skew > 0) && (tcp_ack < ec->seq_skew)) { > /* Should not be possible; will be marked invalid. */ > tcp_ack = 0; > - } else if ((seq_skew < 0) && (UINT32_MAX - tcp_ack < > -seq_skew)) { > - tcp_ack = (-seq_skew) - (UINT32_MAX - tcp_ack); > + } else if ((ec->seq_skew < 0) && > + (UINT32_MAX - tcp_ack < -ec->seq_skew)) { > + tcp_ack = (-ec->seq_skew) - (UINT32_MAX - tcp_ack); > } else { > - tcp_ack -= seq_skew; > + tcp_ack -= ec->seq_skew; > } > ovs_be32 new_tcp_ack = htonl(tcp_ack); > put_16aligned_be32(&th->tcp_ack, new_tcp_ack); > } else { > uint32_t tcp_seq = ntohl(get_16aligned_be32(&th->tcp_seq)); > - if ((seq_skew > 0) && (UINT32_MAX - tcp_seq < seq_skew)) { > - tcp_seq = seq_skew - (UINT32_MAX - tcp_seq); > - } else if ((seq_skew < 0) && (tcp_seq < -seq_skew)) { > + if ((ec->seq_skew > 0) && (UINT32_MAX - tcp_seq < > ec->seq_skew)) { > + tcp_seq = ec->seq_skew - (UINT32_MAX - tcp_seq); > + } else if ((ec->seq_skew < 0) && (tcp_seq < -ec->seq_skew)) { > /* Should not be possible; will be marked invalid. */ > tcp_seq = 0; > } else { > - tcp_seq += seq_skew; > + tcp_seq += ec->seq_skew; > } > ovs_be32 new_tcp_seq = htonl(tcp_seq); > put_16aligned_be32(&th->tcp_seq, new_tcp_seq); > @@ -3290,6 +3284,11 @@ handle_ftp_ctl(struct conntrack *ct, const struct > conn_lookup_ctx *ctx, > uint8_t pad = dp_packet_l2_pad_size(pkt); > th->tcp_csum = csum_finish( > csum_continue(tcp_csum, th, tail - (char *) th - pad)); > + > + if (seq_skew) { > + conn_seq_skew_set(ct, &ec->key, now, seq_skew + ec->seq_skew, > + ctx->reply); > + } > } > > static void > diff --git a/tests/atlocal.in b/tests/atlocal.in > index 388f79e..5eff0a0 100644 > --- a/tests/atlocal.in > +++ b/tests/atlocal.in > @@ -193,6 +193,9 @@ fi > # Set HAVE_TCPDUMP > find_command tcpdump > > +# Set HAVE_LFTP > +find_command lftp > + > CURL_OPT="-g -v --max-time 1 --retry 2 --retry-delay 1 --connect-timeout > 1" > > # Determine whether "diff" supports "normal" diffs. (busybox diff does > not.) > diff --git a/tests/system-traffic.at b/tests/system-traffic.at > index b2ab800..bcaa1a5 100644 > --- a/tests/system-traffic.at > +++ b/tests/system-traffic.at > @@ -4226,6 +4226,7 @@ dnl NAT, using the provided flow table. > m4_define([CHECK_FTP_NAT], > [AT_SETUP([conntrack - FTP NAT $1]) > AT_SKIP_IF([test $HAVE_FTP = no]) > + AT_SKIP_IF([test $HAVE_LFTP = no]) > CHECK_CONNTRACK() > CHECK_CONNTRACK_NAT() > CHECK_CONNTRACK_ALG() > @@ -4246,7 +4247,16 @@ m4_define([CHECK_FTP_NAT], > 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.2 -4 --no-passive-ftp -t > 3 -T 1 --retry-connrefused -v --server-response --no-remove-listing -o > wget0.log -d]) > + AT_DATA([ftp.cmd], [dnl > +set net:max-retries 1 > +set net:timeout 1 > +set ftp:passive-mode off > +cache off > +connect ftp://anonymous:@10.1.1.2 > +ls > +ls > +]) > + NS_CHECK_EXEC([at_ns0], [lftp -f ftp.cmd > lftp.log]) > Please increase the number of 'ls' commands to 4 as mentioned earlier + AT_DATA([ftp.cmd], [dnl +set net:max-retries 1 +set net:timeout 1 +set ftp:passive-mode off +cache off +connect ftp://anonymous:@10.1.1.2 <ftp://anonymous@10.1.1.2/> +ls +ls +ls +ls +]) + NS_CHECK_EXEC([at_ns0], [lftp -f ftp.cmd > lftp.log]) > > dnl Discards CLOSE_WAIT and CLOSING > AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2)], > [0], [$4]) > -- > 1.8.3.1 > Please add this test: (I had written this test when I reviewed patch 4 of version 1 of the series, but did not get a chance to send the review comments out before the series was updated.) AT_SETUP([conntrack - IPv4 FTP Active with DNAT with reverse skew]) 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/24") 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]) ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24") 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.1.2)),2 dnl Allow related TCP connections from port 1. table=1 in_port=2 ct_state=+new+rel tcp nw_src=10.1.1.2 action=ct(commit,nat),1 dnl Allow established TCP connections both ways, post-NAT match. table=1 in_port=1 ct_state=+est tcp nw_dst=10.1.1.2 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.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]) 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>) ]) OVS_TRAFFIC_VSWITCHD_STOP AT_CLEANUP
On Mon, Jan 14, 2019 at 7:32 AM Darrell Ball <dlu998@gmail.com> wrote: > Thanks for the patch > > On Wed, Jan 9, 2019 at 7:33 AM David Marchand <david.marchand@redhat.com> > wrote: > > The ftp alg deals with packets in two ways for the command connection: >> either they are inspected and can be mangled when nat is enabled >> (CT_FTP_CTL_INTEREST) or they just go through without being modified >> (CT_FTP_CTL_OTHER). >> >> For CT_FTP_CTL_INTEREST packets, we must both adjust the packet tcp seq >> number by the connection current offset, then prepare for the next >> packets by setting an accumulated offset in the ct object. > > >> The tests are updated so that ftp+NAT checks send multiple commands in a >> single tcp command connection: >> >> > 1/ Please update the commit message for the 2nd and 3rd paragraphs, > including use case and a correction. > > The ftp alg deals with packets in two ways for the command connection: > either they are inspected and can be mangled when nat is enabled > (CT_FTP_CTL_INTEREST) or they just go through without being modified > (CT_FTP_CTL_OTHER). > > For CT_FTP_CTL_INTEREST packets, we must both adjust the packet tcp seq > number by the connection current offset, then prepare for the next > packets by setting an accumulated offset in the ct object. However, this > was not > done for multiple CT_FTP_CTL_INTEREST packets for the same connection. > This is relevant for handling multiple child data connections that also > need natting. > > The tests are updated so that some ftp+NAT tests send multiple port > commands or > other similar commands for a single control connection. Wget is not able > to do this, > so switch to lftp. > > 2/ A couple conntrack.c comments. > > 3/ I requested changing the number of 'ls' commands below, since it is a > stronger test. > Sure. > 4/ I added another test below for reverse skew. > Well, adding a DNAT test at this point cannot work. I can move it to the second patch. > 5/ Please consider adding me as co-author > Ok. I will wait for your reply on the 32bits wrapping fix before sending a v4. Thanks.
On 1/14/19, 4:49 AM, "ovs-dev-bounces@openvswitch.org on behalf of David Marchand" <ovs-dev-bounces@openvswitch.org on behalf of david.marchand@redhat.com> wrote:
On Mon, Jan 14, 2019 at 7:32 AM Darrell Ball <dlu998@gmail.com> wrote:
> Thanks for the patch
>
> On Wed, Jan 9, 2019 at 7:33 AM David Marchand <david.marchand@redhat.com>
> wrote:
>
> The ftp alg deals with packets in two ways for the command connection:
>> either they are inspected and can be mangled when nat is enabled
>> (CT_FTP_CTL_INTEREST) or they just go through without being modified
>> (CT_FTP_CTL_OTHER).
>>
>> For CT_FTP_CTL_INTEREST packets, we must both adjust the packet tcp seq
>> number by the connection current offset, then prepare for the next
>> packets by setting an accumulated offset in the ct object.
>
>
>> The tests are updated so that ftp+NAT checks send multiple commands in a
>> single tcp command connection:
>>
>>
> 1/ Please update the commit message for the 2nd and 3rd paragraphs,
> including use case and a correction.
>
> The ftp alg deals with packets in two ways for the command connection:
> either they are inspected and can be mangled when nat is enabled
> (CT_FTP_CTL_INTEREST) or they just go through without being modified
> (CT_FTP_CTL_OTHER).
>
> For CT_FTP_CTL_INTEREST packets, we must both adjust the packet tcp seq
> number by the connection current offset, then prepare for the next
> packets by setting an accumulated offset in the ct object. However, this
> was not
> done for multiple CT_FTP_CTL_INTEREST packets for the same connection.
> This is relevant for handling multiple child data connections that also
> need natting.
>
> The tests are updated so that some ftp+NAT tests send multiple port
> commands or
> other similar commands for a single control connection. Wget is not able
> to do this,
> so switch to lftp.
>
> 2/ A couple conntrack.c comments.
>
> 3/ I requested changing the number of 'ls' commands below, since it is a
> stronger test.
>
Sure.
> 4/ I added another test below for reverse skew.
>
Well, adding a DNAT test at this point cannot work.
I can move it to the second patch.
Sure
Btw, I can also bundle the patches together tomorrow morning, which may be easier.
> 5/ Please consider adding me as co-author
>
Ok.
I will wait for your reply on the 32bits wrapping fix before sending a v4.
Thanks.
--
David Marchand
_______________________________________________
dev mailing list
dev@openvswitch.org
https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-dev&data=02%7C01%7Cdball%40vmware.com%7C4ce7c17ebcc6404de5a108d67a1ec040%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636830669846619264&sdata=G7Hd0xuiM491btJDfHk5sT6mNodpUr2171gNtDRnnTE%3D&reserved=0
On Tue, Jan 15, 2019 at 8:20 AM Darrell Ball <dball@vmware.com> wrote: > > On 1/14/19, 4:49 AM, "ovs-dev-bounces@openvswitch.org on behalf of David > Marchand" <ovs-dev-bounces@openvswitch.org on behalf of > david.marchand@redhat.com> wrote: > > On Mon, Jan 14, 2019 at 7:32 AM Darrell Ball <dlu998@gmail.com> wrote: > > > 4/ I added another test below for reverse skew. > > > > Well, adding a DNAT test at this point cannot work. > I can move it to the second patch. > > Sure > > Btw, I can also bundle the patches together tomorrow morning, which may be > easier. > I am just about to send the patchset.
diff --git a/Vagrantfile b/Vagrantfile index 0192f66..fbd772a 100644 --- a/Vagrantfile +++ b/Vagrantfile @@ -12,7 +12,8 @@ dnf -y install autoconf automake openssl-devel libtool \ python-twisted python-zope-interface \ desktop-file-utils groff graphviz rpmdevtools nc curl \ wget python-six pyftpdlib checkpolicy selinux-policy-devel \ - libcap-ng-devel kernel-devel-`uname -r` ethtool python-tftpy + libcap-ng-devel kernel-devel-`uname -r` ethtool python-tftpy \ + lftp echo "search extra update built-in" >/etc/depmod.d/search_path.conf SCRIPT @@ -28,7 +29,8 @@ aptitude -y install -R \ wget python-six ethtool \ libcap-ng-dev libssl-dev python-dev openssl \ python-pyftpdlib python-flake8 python-tftpy \ - linux-headers-`uname -r` + linux-headers-`uname -r` \ + lftp SCRIPT $bootstrap_centos = <<SCRIPT @@ -37,7 +39,8 @@ yum -y install autoconf automake openssl-devel libtool \ python-twisted-core python-zope-interface \ desktop-file-utils groff graphviz rpmdevtools nc curl \ wget python-six pyftpdlib checkpolicy selinux-policy-devel \ - libcap-ng-devel kernel-devel-`uname -r` ethtool net-tools + libcap-ng-devel kernel-devel-`uname -r` ethtool net-tools \ + lftp SCRIPT $configure_ovs = <<SCRIPT diff --git a/Vagrantfile-FreeBSD b/Vagrantfile-FreeBSD index 8f00abe..52599ee 100644 --- a/Vagrantfile-FreeBSD +++ b/Vagrantfile-FreeBSD @@ -12,7 +12,7 @@ Vagrant.require_version ">=1.7.0" $bootstrap_freebsd = <<SCRIPT sed -e 's/\#DEFAULT_ALWAYS_YES = false/DEFAULT_ALWAYS_YES = true/g' -e 's/\#ASSUME_ALWAYS_YES = false/ASSUME_ALWAYS_YES = true/g' /usr/local/etc/pkg.conf > /tmp/pkg.conf mv -f /tmp/pkg.conf /usr/local/etc/pkg.conf -pkg install automake libtool wget python py27-six gmake +pkg install automake libtool wget python py27-six gmake lftp SCRIPT $configure_ovs = <<SCRIPT diff --git a/lib/conntrack.c b/lib/conntrack.c index 6f6021a..4717950 100644 --- a/lib/conntrack.c +++ b/lib/conntrack.c @@ -3170,9 +3170,8 @@ repl_ftp_v6_addr(struct dp_packet *pkt, struct ct_addr v6_addr_rep, static void handle_ftp_ctl(struct conntrack *ct, const struct conn_lookup_ctx *ctx, - struct dp_packet *pkt, - const struct conn *conn_for_expectation, - long long now, enum ftp_ctl_pkt ftp_ctl, bool nat) + struct dp_packet *pkt, const struct conn *ec, long long now, + enum ftp_ctl_pkt ftp_ctl, bool nat) { struct ip_header *l3_hdr = dp_packet_l3(pkt); ovs_be32 v4_addr_rep = 0; @@ -3180,31 +3179,24 @@ handle_ftp_ctl(struct conntrack *ct, const struct conn_lookup_ctx *ctx, size_t addr_offset_from_ftp_data_start = 0; size_t addr_size = 0; char *ftp_data_start; - bool do_seq_skew_adj = true; enum ct_alg_mode mode = CT_FTP_MODE_ACTIVE; if (detect_ftp_ctl_type(ctx, pkt) != ftp_ctl) { return; } - if (!nat || !conn_for_expectation->seq_skew) { - do_seq_skew_adj = false; - } - struct ovs_16aligned_ip6_hdr *nh6 = dp_packet_l3(pkt); int64_t seq_skew = 0; - if (ftp_ctl == CT_FTP_CTL_OTHER) { - seq_skew = conn_for_expectation->seq_skew; - } else if (ftp_ctl == CT_FTP_CTL_INTEREST) { + if (ftp_ctl == CT_FTP_CTL_INTEREST) { enum ftp_ctl_pkt rc; if (ctx->key.dl_type == htons(ETH_TYPE_IPV6)) { - rc = process_ftp_ctl_v6(ct, pkt, conn_for_expectation, + rc = process_ftp_ctl_v6(ct, pkt, ec, &v6_addr_rep, &ftp_data_start, &addr_offset_from_ftp_data_start, &addr_size, &mode); } else { - rc = process_ftp_ctl_v4(ct, pkt, conn_for_expectation, + rc = process_ftp_ctl_v4(ct, pkt, ec, &v4_addr_rep, &ftp_data_start, &addr_offset_from_ftp_data_start); } @@ -3217,62 +3209,64 @@ handle_ftp_ctl(struct conntrack *ct, const struct conn_lookup_ctx *ctx, uint16_t ip_len; if (ctx->key.dl_type == htons(ETH_TYPE_IPV6)) { - seq_skew = repl_ftp_v6_addr(pkt, v6_addr_rep, ftp_data_start, - addr_offset_from_ftp_data_start, - addr_size, mode); + if (nat) { + seq_skew = repl_ftp_v6_addr(pkt, v6_addr_rep, + ftp_data_start, + addr_offset_from_ftp_data_start, + addr_size, mode); + } + if (seq_skew) { ip_len = ntohs(nh6->ip6_ctlun.ip6_un1.ip6_un1_plen); ip_len += seq_skew; nh6->ip6_ctlun.ip6_un1.ip6_un1_plen = htons(ip_len); - conn_seq_skew_set(ct, &conn_for_expectation->key, now, - seq_skew, ctx->reply); } } else { - seq_skew = repl_ftp_v4_addr(pkt, v4_addr_rep, ftp_data_start, - addr_offset_from_ftp_data_start); - ip_len = ntohs(l3_hdr->ip_tot_len); + if (nat) { + seq_skew = repl_ftp_v4_addr(pkt, v4_addr_rep, + ftp_data_start, + addr_offset_from_ftp_data_start); + } if (seq_skew) { + ip_len = ntohs(l3_hdr->ip_tot_len); ip_len += seq_skew; l3_hdr->ip_csum = recalc_csum16(l3_hdr->ip_csum, l3_hdr->ip_tot_len, htons(ip_len)); l3_hdr->ip_tot_len = htons(ip_len); - conn_seq_skew_set(ct, &conn_for_expectation->key, now, - seq_skew, ctx->reply); } } } else { OVS_NOT_REACHED(); } - } else { - OVS_NOT_REACHED(); } struct tcp_header *th = dp_packet_l4(pkt); - if (do_seq_skew_adj && seq_skew != 0) { - if (ctx->reply != conn_for_expectation->seq_skew_dir) { + if (nat && ec->seq_skew != 0) { + if (ctx->reply != ec->seq_skew_dir) { uint32_t tcp_ack = ntohl(get_16aligned_be32(&th->tcp_ack)); - if ((seq_skew > 0) && (tcp_ack < seq_skew)) { + if ((ec->seq_skew > 0) && (tcp_ack < ec->seq_skew)) { /* Should not be possible; will be marked invalid. */ tcp_ack = 0; - } else if ((seq_skew < 0) && (UINT32_MAX - tcp_ack < -seq_skew)) { - tcp_ack = (-seq_skew) - (UINT32_MAX - tcp_ack); + } else if ((ec->seq_skew < 0) && + (UINT32_MAX - tcp_ack < -ec->seq_skew)) { + tcp_ack = (-ec->seq_skew) - (UINT32_MAX - tcp_ack); } else { - tcp_ack -= seq_skew; + tcp_ack -= ec->seq_skew; } ovs_be32 new_tcp_ack = htonl(tcp_ack); put_16aligned_be32(&th->tcp_ack, new_tcp_ack); } else { uint32_t tcp_seq = ntohl(get_16aligned_be32(&th->tcp_seq)); - if ((seq_skew > 0) && (UINT32_MAX - tcp_seq < seq_skew)) { - tcp_seq = seq_skew - (UINT32_MAX - tcp_seq); - } else if ((seq_skew < 0) && (tcp_seq < -seq_skew)) { + if ((ec->seq_skew > 0) && (UINT32_MAX - tcp_seq < ec->seq_skew)) { + tcp_seq = ec->seq_skew - (UINT32_MAX - tcp_seq); + } else if ((ec->seq_skew < 0) && (tcp_seq < -ec->seq_skew)) { /* Should not be possible; will be marked invalid. */ tcp_seq = 0; } else { - tcp_seq += seq_skew; + tcp_seq += ec->seq_skew; } ovs_be32 new_tcp_seq = htonl(tcp_seq); put_16aligned_be32(&th->tcp_seq, new_tcp_seq); @@ -3290,6 +3284,11 @@ handle_ftp_ctl(struct conntrack *ct, const struct conn_lookup_ctx *ctx, uint8_t pad = dp_packet_l2_pad_size(pkt); th->tcp_csum = csum_finish( csum_continue(tcp_csum, th, tail - (char *) th - pad)); + + if (seq_skew) { + conn_seq_skew_set(ct, &ec->key, now, seq_skew + ec->seq_skew, + ctx->reply); + } } static void diff --git a/tests/atlocal.in b/tests/atlocal.in index 388f79e..5eff0a0 100644 --- a/tests/atlocal.in +++ b/tests/atlocal.in @@ -193,6 +193,9 @@ fi # Set HAVE_TCPDUMP find_command tcpdump +# Set HAVE_LFTP +find_command lftp + CURL_OPT="-g -v --max-time 1 --retry 2 --retry-delay 1 --connect-timeout 1" # Determine whether "diff" supports "normal" diffs. (busybox diff does not.) diff --git a/tests/system-traffic.at b/tests/system-traffic.at index b2ab800..bcaa1a5 100644 --- a/tests/system-traffic.at +++ b/tests/system-traffic.at @@ -4226,6 +4226,7 @@ dnl NAT, using the provided flow table. m4_define([CHECK_FTP_NAT], [AT_SETUP([conntrack - FTP NAT $1]) AT_SKIP_IF([test $HAVE_FTP = no]) + AT_SKIP_IF([test $HAVE_LFTP = no]) CHECK_CONNTRACK() CHECK_CONNTRACK_NAT() CHECK_CONNTRACK_ALG() @@ -4246,7 +4247,16 @@ m4_define([CHECK_FTP_NAT], 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.2 -4 --no-passive-ftp -t 3 -T 1 --retry-connrefused -v --server-response --no-remove-listing -o wget0.log -d]) + AT_DATA([ftp.cmd], [dnl +set net:max-retries 1 +set net:timeout 1 +set ftp:passive-mode off +cache off +connect ftp://anonymous:@10.1.1.2 +ls +ls +]) + NS_CHECK_EXEC([at_ns0], [lftp -f ftp.cmd > lftp.log]) dnl Discards CLOSE_WAIT and CLOSING AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2)], [0], [$4])
The ftp alg deals with packets in two ways for the command connection: either they are inspected and can be mangled when nat is enabled (CT_FTP_CTL_INTEREST) or they just go through without being modified (CT_FTP_CTL_OTHER). For CT_FTP_CTL_INTEREST packets, we must both adjust the packet tcp seq number by the connection current offset, then prepare for the next packets by setting an accumulated offset in the ct object. The tests are updated so that ftp+NAT checks send multiple commands in a single tcp command connection: wget is not able to do this, so switch to lftp. Signed-off-by: David Marchand <david.marchand@redhat.com> --- Changelog since v2: - skip tests relying on lftp when absent - removed unneeded temp seq_skew variable by moving conn_seq_skew_set at the end of the function and rely on the value in the conn object, then removed unneeded do_seq_skew_adj --- Vagrantfile | 9 ++++--- Vagrantfile-FreeBSD | 2 +- lib/conntrack.c | 69 ++++++++++++++++++++++++------------------------- tests/atlocal.in | 3 +++ tests/system-traffic.at | 12 ++++++++- 5 files changed, 55 insertions(+), 40 deletions(-)