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

Darrell Ball dlu998 at gmail.com
Tue May 15 01:44:53 UTC 2018


Thanks very much for the review Justin.

Darrell


On Mon, May 14, 2018 at 2:57 PM, Justin Pettit <jpettit at 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 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.
>

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



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

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


More information about the dev mailing list