| Message ID | 1547005387-56474-1-git-send-email-dlu998@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,v1] conntrack: Fix FTP seq_skew boundary adjustments. | expand |
Hello, On Wed, Jan 9, 2019 at 4:44 AM Darrell Ball <dlu998@gmail.com> wrote: > Fixes: bd5e81a0e596 ("Userspace Datapath: Add ALG infra and FTP.") > Signed-off-by: Darrell Ball <dlu998@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. 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)); } }
On Thu, Jan 10, 2019 at 1:03 AM David Marchand <david.marchand@redhat.com> wrote: > Hello, > > On Wed, Jan 9, 2019 at 4:44 AM Darrell Ball <dlu998@gmail.com> wrote: > >> Fixes: bd5e81a0e596 ("Userspace Datapath: Add ALG infra and FTP.") >> Signed-off-by: Darrell Ball <dlu998@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. > > > > 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 >
On Thu, Jan 10, 2019 at 12:58 PM Darrell Ball <dlu998@gmail.com> wrote: > > > On Thu, Jan 10, 2019 at 1:03 AM David Marchand <david.marchand@redhat.com> > wrote: > >> Hello, >> >> On Wed, Jan 9, 2019 at 4:44 AM Darrell Ball <dlu998@gmail.com> wrote: >> >>> Fixes: bd5e81a0e596 ("Userspace Datapath: Add ALG infra and FTP.") >>> Signed-off-by: Darrell Ball <dlu998@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 >> >
On Fri, Jan 11, 2019 at 8:05 PM Darrell Ball <dlu998@gmail.com> wrote: > On Thu, Jan 10, 2019 at 12:58 PM Darrell Ball <dlu998@gmail.com> wrote: > >> On Thu, Jan 10, 2019 at 1:03 AM David Marchand <david.marchand@redhat.com> >> wrote: >> >>> Hello, >>> >>> 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. > Ok, I had missed the int64_t for seq_skew. More than trusting the compiler, I understand we only want to deal with 32bits integers, since, at the end of the game, we want any offset bigger than this to wrap on the 32bits range. If we change it to int32_t, we are back with only 32bits integers: signed for the seq_skew variable, and unsigned for the seq/ack values => addition and substraction gives a uint32_t thanks to the "Usual arithmetic conversions" ? diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h index a344801..c261f80 100644 --- a/lib/conntrack-private.h +++ b/lib/conntrack-private.h @@ -99,7 +99,7 @@ struct conn { /* XXX: consider flattening. */ struct nat_action_info_t *nat_info; char *alg; - int seq_skew; + int32_t seq_skew; uint32_t mark; uint8_t conn_type; /* TCP sequence skew due to NATTing of FTP control messages. */ diff --git a/lib/conntrack.c b/lib/conntrack.c index 6f6021a..206f5f1 100644 --- a/lib/conntrack.c +++ b/lib/conntrack.c @@ -756,7 +756,7 @@ conn_lookup(struct conntrack *ct, const struct conn_key *key, long long now) static void conn_seq_skew_set(struct conntrack *ct, const struct conn_key *key, - long long now, int seq_skew, bool seq_skew_dir) + long long now, int32_t seq_skew, bool seq_skew_dir) { unsigned bucket = hash_to_bucket(conn_key_hash(key, ct->hash_basis)); ct_lock_lock(&ct->buckets[bucket].lock); @@ -3192,7 +3192,7 @@ handle_ftp_ctl(struct conntrack *ct, const struct conn_lookup_ctx *ctx, } struct ovs_16aligned_ip6_hdr *nh6 = dp_packet_l3(pkt); - int64_t seq_skew = 0; + int32_t seq_skew = 0; if (ftp_ctl == CT_FTP_CTL_OTHER) { seq_skew = conn_for_expectation->seq_skew; @@ -3251,31 +3251,13 @@ handle_ftp_ctl(struct conntrack *ct, const struct conn_lookup_ctx *ctx, if (do_seq_skew_adj && seq_skew != 0) { if (ctx->reply != conn_for_expectation->seq_skew_dir) { - 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; - } else if ((seq_skew < 0) && (UINT32_MAX - tcp_ack < -seq_skew)) { - tcp_ack = (-seq_skew) - (UINT32_MAX - tcp_ack); - } else { - tcp_ack -= 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 - seq_skew)); } 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); - } else if ((seq_skew < 0) && (tcp_seq < -seq_skew)) { - /* Should not be possible; will be marked invalid. */ - tcp_seq = 0; - } else { - tcp_seq += 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 + seq_skew)); } } What do you think ? Do you want me to send a patch ? (I could take it as part of my other fixes)
On Mon, Jan 14, 2019 at 3:55 AM David Marchand <david.marchand@redhat.com> wrote: > On Fri, Jan 11, 2019 at 8:05 PM Darrell Ball <dlu998@gmail.com> wrote: > >> On Thu, Jan 10, 2019 at 12:58 PM Darrell Ball <dlu998@gmail.com> wrote: >> >>> On Thu, Jan 10, 2019 at 1:03 AM David Marchand < >>> david.marchand@redhat.com> wrote: >>> >>>> Hello, >>>> >>>> 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. >> > > Ok, I had missed the int64_t for seq_skew. > > More than trusting the compiler, I understand we only want to deal with > 32bits integers, since, at the end of the game, we want any offset bigger > than this to wrap on the 32bits range. > If we change it to int32_t, we are back with only 32bits integers: signed > for the seq_skew variable, and unsigned for the seq/ack values => addition > and substraction gives a uint32_t thanks to the "Usual arithmetic > conversions" ? > > what I meant by the last response is I don't think there is an issue either way Generally, I have a bias in not trusting the compiler where unnecessary, but in this case, there is a marginal benefit. I forgot to send my patch which will make it more clear, which you might rebase and fold in at the end of your series. > diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h > index a344801..c261f80 100644 > --- a/lib/conntrack-private.h > +++ b/lib/conntrack-private.h > @@ -99,7 +99,7 @@ struct conn { > /* XXX: consider flattening. */ > struct nat_action_info_t *nat_info; > char *alg; > - int seq_skew; > + int32_t seq_skew; > uint32_t mark; > uint8_t conn_type; > /* TCP sequence skew due to NATTing of FTP control messages. */ > diff --git a/lib/conntrack.c b/lib/conntrack.c > index 6f6021a..206f5f1 100644 > --- a/lib/conntrack.c > +++ b/lib/conntrack.c > @@ -756,7 +756,7 @@ conn_lookup(struct conntrack *ct, const struct > conn_key *key, long long now) > > static void > conn_seq_skew_set(struct conntrack *ct, const struct conn_key *key, > - long long now, int seq_skew, bool seq_skew_dir) > + long long now, int32_t seq_skew, bool seq_skew_dir) > { > unsigned bucket = hash_to_bucket(conn_key_hash(key, ct->hash_basis)); > ct_lock_lock(&ct->buckets[bucket].lock); > @@ -3192,7 +3192,7 @@ handle_ftp_ctl(struct conntrack *ct, const struct > conn_lookup_ctx *ctx, > } > > struct ovs_16aligned_ip6_hdr *nh6 = dp_packet_l3(pkt); > - int64_t seq_skew = 0; > + int32_t seq_skew = 0; > > if (ftp_ctl == CT_FTP_CTL_OTHER) { > seq_skew = conn_for_expectation->seq_skew; > @@ -3251,31 +3251,13 @@ handle_ftp_ctl(struct conntrack *ct, const struct > conn_lookup_ctx *ctx, > > if (do_seq_skew_adj && seq_skew != 0) { > if (ctx->reply != conn_for_expectation->seq_skew_dir) { > - > 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; > - } else if ((seq_skew < 0) && (UINT32_MAX - tcp_ack < > -seq_skew)) { > - tcp_ack = (-seq_skew) - (UINT32_MAX - tcp_ack); > - } else { > - tcp_ack -= 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 - seq_skew)); > } 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); > - } else if ((seq_skew < 0) && (tcp_seq < -seq_skew)) { > - /* Should not be possible; will be marked invalid. */ > - tcp_seq = 0; > - } else { > - tcp_seq += 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 + seq_skew)); > } > } > > What do you think ? > Do you want me to send a patch ? (I could take it as part of my other > fixes) > > > -- > David Marchand >
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; }
Fixes: bd5e81a0e596 ("Userspace Datapath: Add ALG infra and FTP.") Signed-off-by: Darrell Ball <dlu998@gmail.com> --- Backport to 2.8. lib/conntrack.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)