[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