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

Message ID 1519889150-113512-1-git-send-email-dlu998@gmail.com
State Superseded
Delegated to: Justin Pettit
Headers show
Series
  • [ovs-dev,v1] conntrack-tcp: Handle tcp session reuse.
Related show

Commit Message

Darrell Ball March 1, 2018, 7:25 a.m.
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@gmail.com>
---
 lib/conntrack-tcp.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

Comments

Ben Pfaff March 9, 2018, 7:22 p.m. | #1
On Wed, Feb 28, 2018 at 11:25:50PM -0800, Darrell Ball 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@gmail.com>

Darrell, who should review this?  Justin, are you the right person?
Darrell Ball March 9, 2018, 7:29 p.m. | #2
Windows folks have also been looking at this, as this file is mostly a common port from bsd.
I’ll check with Sai

On 3/9/18, 11:22 AM, "ovs-dev-bounces@openvswitch.org on behalf of Ben Pfaff" <ovs-dev-bounces@openvswitch.org on behalf of blp@ovn.org> wrote:

    On Wed, Feb 28, 2018 at 11:25:50PM -0800, Darrell Ball 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@gmail.com>

    
    Darrell, who should review this?  Justin, are you the right person?
    _______________________________________________
    dev mailing list
    dev@openvswitch.org
    https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=cwtm0y5t5FEfGZdPOmWiiE5ODS8BoNvdrbv6oFCZNEk&s=yHLhMPNWgZwAe36ObZ2obrSzPWqVbJZOg5ofA18IS_k&e=
Ben Pfaff April 10, 2018, 11:49 p.m. | #3
Did you find anyone to take a look?

On Fri, Mar 09, 2018 at 07:29:06PM +0000, Darrell Ball wrote:
> Windows folks have also been looking at this, as this file is mostly a common port from bsd.
> I’ll check with Sai
> 
> On 3/9/18, 11:22 AM, "ovs-dev-bounces@openvswitch.org on behalf of Ben Pfaff" <ovs-dev-bounces@openvswitch.org on behalf of blp@ovn.org> wrote:
> 
>     On Wed, Feb 28, 2018 at 11:25:50PM -0800, Darrell Ball 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@gmail.com>
>     
>     Darrell, who should review this?  Justin, are you the right person?
>     _______________________________________________
>     dev mailing list
>     dev@openvswitch.org
>     https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=cwtm0y5t5FEfGZdPOmWiiE5ODS8BoNvdrbv6oFCZNEk&s=yHLhMPNWgZwAe36ObZ2obrSzPWqVbJZOg5ofA18IS_k&e=
>     
>
Darrell Ball April 11, 2018, 12:13 a.m. | #4
Not yet; I will check with Justin when he gets back from PTO

On 4/10/18, 4:50 PM, "Ben Pfaff" <blp@ovn.org> wrote:

    Did you find anyone to take a look?
    
    On Fri, Mar 09, 2018 at 07:29:06PM +0000, Darrell Ball wrote:
    > Windows folks have also been looking at this, as this file is mostly a common port from bsd.
    > I’ll check with Sai
    > 
    > On 3/9/18, 11:22 AM, "ovs-dev-bounces@openvswitch.org on behalf of Ben Pfaff" <ovs-dev-bounces@openvswitch.org on behalf of blp@ovn.org> wrote:
    > 
    >     On Wed, Feb 28, 2018 at 11:25:50PM -0800, Darrell Ball 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@gmail.com>
    >     
    >     Darrell, who should review this?  Justin, are you the right person?
    >     _______________________________________________
    >     dev mailing list
    >     dev@openvswitch.org
    >     https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=cwtm0y5t5FEfGZdPOmWiiE5ODS8BoNvdrbv6oFCZNEk&s=yHLhMPNWgZwAe36ObZ2obrSzPWqVbJZOg5ofA18IS_k&e=
    >     
    >
Justin Pettit May 14, 2018, 9:57 p.m. | #5
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@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@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@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)
Darrell Ball May 15, 2018, 1:44 a.m. | #6
Thanks very much for the review Justin.

Darrell


On Mon, May 14, 2018 at 2:57 PM, Justin Pettit <jpettit@ovn.org> wrote:

> 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


Thanks; I do add comments below '---' from time to time :-) but you are
right, this does not belong in the commit message.
I also added a 'Fixes' tag for completeness.


> .
>
> -=-=-=-=-=-=-=-=-=-=-
> ...
> would be lessened and code complexity increased.
>
> Signed-off-by: Darrell Ball <dlu998@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.
>

Thanks
i sent a V2 here:
https://patchwork.ozlabs.org/patch/913386/



>
> --Justin
>
>
> > On Feb 28, 2018, at 11:25 PM, Darrell Ball <dlu998@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@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;
>      }
>

Because we need to still process the state later on in the function, it is
probably better that we keep the above 2 lines unchanged.


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

Thanks
Sometimes for clarity (or sometimes for no valid reason :-)) I don't use
the ternary operator.
I rolled in this change for V2.


>
>  #define MAXACKWINDOW (0xffff + 1500)    /* 1500 is an arbitrary fudge
> factor */
>      if (SEQ_GEQ(src->seqhi, end)
>
>
>
>
>
>

Patch

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 */