On Wed, Dec 19, 2018 at 1:10 AM David Marchand <david.march...@redhat.com>
wrote:
>
> 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.
>
Just create a test case that fails before a code change and passes with the
code change.
The test should be part of the same patch as the fix code.
>
>
>>
>>
>>>
>>> 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...
>
The intention is to run the code with and without nat, without special
casing.
This also exercises the code more and makes it harder to break. Without
nat, it should be a nop.
Since the number of packets hitting the code path is tiny, performance is
irrelevant.
>
>
>>
>>
>>> 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
