Hello, On Wed, Jan 9, 2019 at 4:51 AM Darrell Ball <[email protected]> wrote:
> On Tue, Jan 8, 2019 at 2:57 PM Darrell Ball <[email protected]> wrote: > >> On Tue, Jan 8, 2019 at 2:13 AM David Marchand <[email protected]> >> wrote: >> >>> >>> Let me take an example with the test CHECK_FTP_SNAT_POST_RECIRC() + >>> address 10.1.1.240. >>> >>> When dealing with the first mangle operation (client sending its first >>> "PORT xxx" for the first "ls" command), the conn tcp seq skew value is at 0. >>> repl_ftp_v4_addr() returns 2 since it replaced 10.1.1.1 with 10.1.1.240. >>> The tcp seq numbers is kept untouched in the "if (do_seq_skew_adj && >>> seq_skew != 0)" block later in this function. >>> >>> When dealing with the second mangle operation (second "PORT xxx"), the >>> current conn tcp skew value is at 2. >>> repl_ftp_v4_addr() returns again 2 since it replaced 10.1.1.1 with >>> 10.1.1.240. >>> The tcp seq number is updated by 2 in the "if (do_seq_skew_adj && >>> seq_skew != 0)" block later in this function. >>> >>> Now, you could argue that the code you propose can work so far, but if >>> we add a new command again in this session, the tcp seq numbers sequence is >>> broken. >>> The command packet tcp seq number would be updated by the >>> repl_ftp_v4_addr() return value 2 and not by the accumulated value which >>> would be 4 for it to work. >>> >> >> @@ -3170,9 +3178,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; >> @@ -3187,24 +3194,22 @@ handle_ftp_ctl(struct conntrack *ct, const struct >> conn_lookup_ctx *ctx, >> return; >> } >> >> - if (!nat || !conn_for_expectation->seq_skew) { >> + if (!nat || !ec->seq_skew) { >> do_seq_skew_adj = false; >> } >> >> > How about removing this boolean ? See below. > 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); >> } >> @@ -3214,65 +3219,67 @@ handle_ftp_ctl(struct conntrack *ct, const struct >> conn_lookup_ctx *ctx, >> pkt->md.ct_state |= CS_TRACKED | CS_INVALID; >> return; >> } else if (rc == CT_FTP_CTL_INTEREST) { >> + >> 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; >> + 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 += seq_skew; >> + 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)); >> 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 (do_seq_skew_adj && ec->seq_skew != 0) { >> > > s/if (do_seq_skew_adj && ec->seq_skew != 0) {/if (do_seq_skew_adj) {/ > > since "ec->seq_skew != 0" is now redundant > Since the connection seq_skew is updated as the last thing in the function, we do not need the do_seq_skew_adj boolean anymore. We can change the test as "if (nat && ec->seq_skew != 0) {" [snip] > >> >> @@ -3290,6 +3297,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 >> (END) >> >> Will send a v3 with this change today. -- David Marchand _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
