Thanks very much for the review Justin. Darrell
On Mon, May 14, 2018 at 2:57 PM, Justin Pettit <[email protected]> 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 <[email protected]> > --- > 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 <[email protected]> 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 <[email protected]> > > --- > > 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) > > > > > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
