On Thu, Jan 10, 2019 at 12:58 PM Darrell Ball <dlu...@gmail.com> wrote:
>
>
> On Thu, Jan 10, 2019 at 1:03 AM David Marchand <david.march...@redhat.com>
> wrote:
>
>> Hello,
>>
>> On Wed, Jan 9, 2019 at 4:44 AM Darrell Ball <dlu...@gmail.com> wrote:
>>
>>> Fixes: bd5e81a0e596 ("Userspace Datapath: Add ALG infra and FTP.")
>>> Signed-off-by: Darrell Ball <dlu...@gmail.com>
>>> ---
>>>
>>> Backport to 2.8.
>>>
>>> lib/conntrack.c | 10 ++++------
>>> 1 file changed, 4 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/lib/conntrack.c b/lib/conntrack.c
>>> index 6f6021a..e992b77 100644
>>> --- a/lib/conntrack.c
>>> +++ b/lib/conntrack.c
>>> @@ -3255,10 +3255,9 @@ handle_ftp_ctl(struct conntrack *ct, const struct
>>> conn_lookup_ctx *ctx,
>>> uint32_t tcp_ack = ntohl(get_16aligned_be32(&th->tcp_ack));
>>>
>>> if ((seq_skew > 0) && (tcp_ack < seq_skew)) {
>>> - /* Should not be possible; will be marked invalid. */
>>> - tcp_ack = 0;
>>> + tcp_ack = UINT32_MAX - (seq_skew - tcp_ack - 1);
>>> } else if ((seq_skew < 0) && (UINT32_MAX - tcp_ack <
>>> -seq_skew)) {
>>> - tcp_ack = (-seq_skew) - (UINT32_MAX - tcp_ack);
>>> + tcp_ack = (-seq_skew) - (UINT32_MAX - tcp_ack) - 1;
>>> } else {
>>> tcp_ack -= seq_skew;
>>> }
>>> @@ -3267,10 +3266,9 @@ handle_ftp_ctl(struct conntrack *ct, const struct
>>> conn_lookup_ctx *ctx,
>>> } else {
>>> uint32_t tcp_seq = ntohl(get_16aligned_be32(&th->tcp_seq));
>>> if ((seq_skew > 0) && (UINT32_MAX - tcp_seq < seq_skew)) {
>>> - tcp_seq = seq_skew - (UINT32_MAX - tcp_seq);
>>> + tcp_seq = seq_skew - (UINT32_MAX - tcp_seq) - 1;
>>> } else if ((seq_skew < 0) && (tcp_seq < -seq_skew)) {
>>> - /* Should not be possible; will be marked invalid. */
>>> - tcp_seq = 0;
>>> + tcp_seq = UINT32_MAX - ((-seq_skew) - tcp_seq - 1);
>>> } else {
>>> tcp_seq += seq_skew;
>>> }
>>> --
>>> 1.9.1
>>>
>>>
>> Ah, now that I think about it, I had seen some packets with ack == 0 but
>> did not reproduce (it was in my todolist).
>> Good catch.
>>
>> Just wondering, can't we rely integer promotion then truncation on 32bits
>> ?
>> My test program tends to show it works.
>>
>
> I believe a compiler is free to convert the operands to int64 and AFAIK
> overflow/underflow is undefined for signed variables.
>
nevermind; it appears we can trust the compiler in this case and simplify.
>
>
>>
>>
>
>>
>
>> Something like:
>>
>> diff --git a/lib/conntrack.c b/lib/conntrack.c
>> index eb36353..04ddf5e 100644
>> --- a/lib/conntrack.c
>> +++ b/lib/conntrack.c
>> @@ -3329,32 +3329,13 @@ handle_ftp_ctl(struct conntrack *ct, const struct
>> conn_lookup_ctx *ctx,
>>
>> if (nat && ec->seq_skew != 0) {
>> if (ctx->reply != ec->seq_skew_dir) {
>> -
>> uint32_t tcp_ack = ntohl(get_16aligned_be32(&th->tcp_ack));
>>
>> - if ((ec->seq_skew > 0) && (tcp_ack < ec->seq_skew)) {
>> - /* Should not be possible; will be marked invalid. */
>> - tcp_ack = 0;
>> - } else if ((ec->seq_skew < 0) &&
>> - (UINT32_MAX - tcp_ack < -ec->seq_skew)) {
>> - tcp_ack = (-ec->seq_skew) - (UINT32_MAX - tcp_ack);
>> - } else {
>> - tcp_ack -= ec->seq_skew;
>> - }
>> - ovs_be32 new_tcp_ack = htonl(tcp_ack);
>> - put_16aligned_be32(&th->tcp_ack, new_tcp_ack);
>> + put_16aligned_be32(&th->tcp_ack, htonl(tcp_ack -
>> ec->seq_skew));
>> } else {
>> uint32_t tcp_seq = ntohl(get_16aligned_be32(&th->tcp_seq));
>> - if ((ec->seq_skew > 0) && (UINT32_MAX - tcp_seq <
>> ec->seq_skew)) {
>> - tcp_seq = ec->seq_skew - (UINT32_MAX - tcp_seq);
>> - } else if ((ec->seq_skew < 0) && (tcp_seq < -ec->seq_skew)) {
>> - /* Should not be possible; will be marked invalid. */
>> - tcp_seq = 0;
>> - } else {
>> - tcp_seq += ec->seq_skew;
>> - }
>> - ovs_be32 new_tcp_seq = htonl(tcp_seq);
>> - put_16aligned_be32(&th->tcp_seq, new_tcp_seq);
>> +
>> + put_16aligned_be32(&th->tcp_seq, htonl(tcp_seq +
>> ec->seq_skew));
>> }
>> }
>>
>>
>> --
>> David Marchand
>>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
