On Wed, Dec 19, 2018 at 9:41 AM Darrell Ball <[email protected]> wrote:
> Thanks for working on this David. > > On Sat, Dec 15, 2018 at 9:39 AM David Marchand <[email protected]> > wrote: > >> The ftp alg relies on the attached nat information to the current >> connection to trigger the nat operation while it should take the >> information from the rule being evaluated. >> > > I cannot connect the commit message with the code change. > Could you add a test case, so I might understand ? > If you look at the postrecirc seqadj test, you can see the following rules: table=0 ip, action=ct(table=1) table=0 priority=100 arp arp_op=1 action=move:OXM_OF_ARP_TPA[[]]->NXM_NX_REG2[[]],resubmit(,8),goto_table:10 table=0 priority=10 arp action=normal table=0 priority=0 action=drop table=1 in_port=1 ct_state=+new, tcp, tp_dst=21, action=ct(alg=ftp,commit,nat(src=$2)),2 table=1 in_port=1 ct_state=+est, tcp, action=ct(nat),2 table=1 in_port=2 ct_state=+est, tcp, action=ct(nat),1 table=1 in_port=2 ct_state=+new+rel, tcp, action=ct(commit,nat),1 table=1 in_port=2 ct_state=+rel, icmp, action=ct(nat),1 table=1 priority=0, action=drop ... The packets for the established tcp command connection get natted at the evaluation of the first rule. table=0 ip, action=ct(table=1) Because the ct contains a nat action. > > >> >> Signed-off-by: David Marchand <[email protected]> >> --- >> lib/conntrack.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/lib/conntrack.c b/lib/conntrack.c >> index d08d0ea..41c56c1 100644 >> --- a/lib/conntrack.c >> +++ b/lib/conntrack.c >> @@ -3204,7 +3204,7 @@ handle_ftp_ctl(struct conntrack *ct, const struct >> conn_lookup_ctx *ctx, >> VLOG_WARN_RL(&rl, "Invalid FTP control packet format"); >> pkt->md.ct_state |= CS_TRACKED | CS_INVALID; >> return; >> - } else if (rc == CT_FTP_CTL_INTEREST) { >> + } else if (rc == CT_FTP_CTL_INTEREST && nat) { >> > > This line tries to defeat the original intent of the code towards common > code path and exercising. > I can move the whole block under a if (nat) test if you prefer, and then... > > >> uint16_t ip_len; >> int64_t new_skew; >> >> @@ -3232,7 +3232,7 @@ handle_ftp_ctl(struct conntrack *ct, const struct >> conn_lookup_ctx *ctx, >> new_skew + seq_skew, ctx->reply); >> } >> } >> - } else { >> + } else if (rc == CT_FTP_CTL_OTHER) { >> > > This line just compensates for the first line change to avoid hitting the > assert sanity check. > ... that would make this change disappear. -- David Marchand _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
