On Thu, Jan 10, 2019 at 1:03 AM David Marchand <[email protected]> wrote:
> Hello, > > On Wed, Jan 9, 2019 at 4:44 AM Darrell Ball <[email protected]> wrote: > >> Fixes: bd5e81a0e596 ("Userspace Datapath: Add ALG infra and FTP.") >> Signed-off-by: Darrell Ball <[email protected]> >> --- >> >> 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 > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
