On Wed, Dec 19, 2018 at 9:41 AM Darrell Ball <dlu...@gmail.com> wrote:
> Thanks for working on this David.
>
> On Sat, Dec 15, 2018 at 9:39 AM David Marchand <david.march...@redhat.com>
> 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 <david.march...@redhat.com>
>> ---
>> 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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
