| 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. | expand |
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?
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=
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= > >
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=
>
>
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)
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) > > > > > >
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 */
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(-)