On Wed, Jan 9, 2019 at 12:53 AM David Marchand <[email protected]> wrote:
> 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) {" > sure; do_seq_skew_adj is not needed anymore. > [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
