[ovs-dev] [patch v1] conntrack-tcp: Handle tcp session reuse.

Justin Pettit jpettit at ovn.org
Mon May 14 21:57:30 UTC 2018


Thank you for the patch.  Sorry it took so long to review.  What do you think of the incremental at the end of this message?  It doesn't change the functionality, but it shortens the code a tad, and I think improves the readability a bit.

Also, thanks for indicating that this should be backported to 2.7.  If you put those sorts of comments in between a pair of "---" like the following, it won't be included in the commit message.

-=-=-=-=-=-=-=-=-=-=-
...
would be lessened and code complexity increased.

Signed-off-by: Darrell Ball <dlu998 at gmail.com>
---
This issue originates in release 2.7.
---
lib/conntrack-tcp.c | 12 +++++++++---
...
-=-=-=-=-=-=-=-=-=-=-

Let me know if you agree with the incremental.  If so, I'll go ahead and apply this.

--Justin


> On Feb 28, 2018, at 11:25 PM, Darrell Ball <dlu998 at gmail.com> wrote:
> 
> Fix tcp sequence tracking for session reuse cases.  This can happen,
> for example by doing VM migration, where sequence tracking needs to
> be more permissive.  The solution is to be more permissive for
> session restart and session start only.  We don't differentiate
> session start here where we could be more strict, although we could,
> because the gain in protection is almost zero and the code modularity
> would be lessened and code complexity increased.
> This issue originates in release 2.7.
> 
> Signed-off-by: Darrell Ball <dlu998 at gmail.com>
> ---
> lib/conntrack-tcp.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/conntrack-tcp.c b/lib/conntrack-tcp.c
> index 04460c3..a0ddd65 100644
> --- a/lib/conntrack-tcp.c
> +++ b/lib/conntrack-tcp.c
> @@ -160,7 +160,6 @@ tcp_conn_update(struct conn *conn_, struct conntrack_bucket *ctb,
>     uint16_t win = ntohs(tcp->tcp_winsz);
>     uint32_t ack, end, seq, orig_seq;
>     uint32_t p_len = tcp_payload_length(pkt);
> -    int ackskew;
> 
>     if (tcp_invalid_flags(tcp_flags)) {
>         return CT_UPDATE_INVALID;
> @@ -195,11 +194,11 @@ tcp_conn_update(struct conn *conn_, struct conntrack_bucket *ctb,
>      */
> 
>     orig_seq = seq = ntohl(get_16aligned_be32(&tcp->tcp_seq));
> +    bool check_ackskew = true;
>     if (src->state < CT_DPIF_TCPS_SYN_SENT) {
>         /* First packet from this end. Set its state */
> 
>         ack = ntohl(get_16aligned_be32(&tcp->tcp_ack));
> -
>         end = seq + p_len;
>         if (tcp_flags & TCP_SYN) {
>             end++;
> @@ -232,6 +231,7 @@ tcp_conn_update(struct conn *conn_, struct conntrack_bucket *ctb,
>         if (src->seqhi == 1
>                 || SEQ_GEQ(end + MAX(1, dst->max_win << dws), src->seqhi)) {
>             src->seqhi = end + MAX(1, dst->max_win << dws);
> +            check_ackskew = false;
>         }
>         if (win > src->max_win) {
>             src->max_win = win;
> @@ -265,7 +265,13 @@ tcp_conn_update(struct conn *conn_, struct conntrack_bucket *ctb,
>         end = seq;
>     }
> 
> -    ackskew = dst->seqlo - ack;
> +    int ackskew;
> +    if (check_ackskew) {
> +        ackskew = dst->seqlo - ack;
> +    } else {
> +        ackskew = 0;
> +    }
> +
> #define MAXACKWINDOW (0xffff + 1500)    /* 1500 is an arbitrary fudge factor */
>     if (SEQ_GEQ(src->seqhi, end)
>         /* Last octet inside other's window space */
> -- 
> 1.9.1
> 

-=-=-=-=-=-=-=-=-=-=-

diff --git a/lib/conntrack-tcp.c b/lib/conntrack-tcp.c
index a0ddd65b4b71..bb7046bab595 100644
--- a/lib/conntrack-tcp.c
+++ b/lib/conntrack-tcp.c
@@ -250,13 +250,13 @@ tcp_conn_update(struct conn *conn_, struct conntrack_bucket *ctb,
 
     if ((tcp_flags & TCP_ACK) == 0) {
         /* Let it pass through the ack skew check */
-        ack = dst->seqlo;
+        check_ackskew = false;
     } else if ((ack == 0
                 && (tcp_flags & (TCP_ACK|TCP_RST)) == (TCP_ACK|TCP_RST))
                /* broken tcp stacks do not set ack */) {
         /* Many stacks (ours included) will set the ACK number in an
          * FIN|ACK if the SYN times out -- no sequence to ACK. */
-        ack = dst->seqlo;
+        check_ackskew = false;
     }
 
     if (seq == end) {
@@ -265,12 +265,7 @@ tcp_conn_update(struct conn *conn_, struct conntrack_bucket *ctb,
         end = seq;
     }
 
-    int ackskew;
-    if (check_ackskew) {
-        ackskew = dst->seqlo - ack;
-    } else {
-        ackskew = 0;
-    }
+    int ackskew = check_ackskew ? dst->seqlo - ack : 0;
 
 #define MAXACKWINDOW (0xffff + 1500)    /* 1500 is an arbitrary fudge factor */
     if (SEQ_GEQ(src->seqhi, end)







More information about the dev mailing list