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.
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
