| Message ID | 356dce030d0adf2bf6b45888876e2c3f9f553a3a.1623691387.git.pabeni@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Mat Martineau |
| Headers | show |
| Series | [mptcp-net] mptcp: fix bad handling od 32 bit ack wrap-around. | expand |
On Mon, 14 Jun 2021, Paolo Abeni wrote: > When receiving 32 bits DSS ack from the peer, the MPTCP need > to expand them to 64 bits value. The current code is buggy > WRT detectiong 32 bits ack wrap-around: usaging the 'before()' > helper is just meaningless: when the wrap-around happes the > current 32 bit ack value is lower than the previous one, need > plain direct comparison. > > Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/204 > Fixes: cc9d25669866 ("mptcp: update per unacked sequence on pkt reception") > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > --- > net/mptcp/options.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/mptcp/options.c b/net/mptcp/options.c > index a05270996613..7db1e219f24f 100644 > --- a/net/mptcp/options.c > +++ b/net/mptcp/options.c > @@ -952,7 +952,7 @@ static u64 expand_ack(u64 old_ack, u64 cur_ack, bool use_64bit) > old_ack32 = (u32)old_ack; > cur_ack32 = (u32)cur_ack; > cur_ack = (old_ack & GENMASK_ULL(63, 32)) + cur_ack32; > - if (unlikely(before(cur_ack32, old_ack32))) > + if (unlikely(cur_ack32 < old_ack32)) > return cur_ack + (1LL << 32); > return cur_ack; > } I think you're right, but I haven't had a chance to test myself (since this is tagged as closing #204 it sounds like your tests are good!). While it's a good thing that mptcp_trunk helped us find this problem, maybe this also shows that mptcp_trunk should be more aggressive about switching to 64-bit ACKs? Or is this less about speed and only a wraparound bug? -- Mat Martineau Intel
On Mon, 2021-06-14 at 18:27 -0700, Mat Martineau wrote: > On Mon, 14 Jun 2021, Paolo Abeni wrote: > > > When receiving 32 bits DSS ack from the peer, the MPTCP need > > to expand them to 64 bits value. The current code is buggy > > WRT detectiong 32 bits ack wrap-around: usaging the 'before()' > > helper is just meaningless: when the wrap-around happes the > > current 32 bit ack value is lower than the previous one, need > > plain direct comparison. > > > > Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/204 > > Fixes: cc9d25669866 ("mptcp: update per unacked sequence on pkt reception") > > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > > --- > > net/mptcp/options.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/net/mptcp/options.c b/net/mptcp/options.c > > index a05270996613..7db1e219f24f 100644 > > --- a/net/mptcp/options.c > > +++ b/net/mptcp/options.c > > @@ -952,7 +952,7 @@ static u64 expand_ack(u64 old_ack, u64 cur_ack, bool use_64bit) > > old_ack32 = (u32)old_ack; > > cur_ack32 = (u32)cur_ack; > > cur_ack = (old_ack & GENMASK_ULL(63, 32)) + cur_ack32; > > - if (unlikely(before(cur_ack32, old_ack32))) > > + if (unlikely(cur_ack32 < old_ack32)) > > return cur_ack + (1LL << 32); > > return cur_ack; > > } > > I think you're right, but I haven't had a chance to test myself (since > this is tagged as closing #204 it sounds like your tests are good!). > > While it's a good thing that mptcp_trunk helped us find this problem, > maybe this also shows that mptcp_trunk should be more aggressive about > switching to 64-bit ACKs? Or is this less about speed and only a > wraparound bug? Without this patch single subflows transfer from net-next to mptcp.org stalls after a few GB - after the wrap-around. That happens because net-next misinterpret the ack for the wrapped seq as a very old one. This patch fixes the issue in my test. Not sure if the above answer your question. Cheers, Paolo
On Tue, 15 Jun 2021, Paolo Abeni wrote: > On Mon, 2021-06-14 at 18:27 -0700, Mat Martineau wrote: >> On Mon, 14 Jun 2021, Paolo Abeni wrote: >> >>> When receiving 32 bits DSS ack from the peer, the MPTCP need >>> to expand them to 64 bits value. The current code is buggy >>> WRT detectiong 32 bits ack wrap-around: usaging the 'before()' A couple of typo fixes: detecting, using... >>> helper is just meaningless: when the wrap-around happes the ...happens. And "of" in the subject line. >>> current 32 bit ack value is lower than the previous one, need >>> plain direct comparison. >>> >>> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/204 >>> Fixes: cc9d25669866 ("mptcp: update per unacked sequence on pkt reception") >>> Signed-off-by: Paolo Abeni <pabeni@redhat.com> >>> --- >>> net/mptcp/options.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c >>> index a05270996613..7db1e219f24f 100644 >>> --- a/net/mptcp/options.c >>> +++ b/net/mptcp/options.c >>> @@ -952,7 +952,7 @@ static u64 expand_ack(u64 old_ack, u64 cur_ack, bool use_64bit) >>> old_ack32 = (u32)old_ack; >>> cur_ack32 = (u32)cur_ack; >>> cur_ack = (old_ack & GENMASK_ULL(63, 32)) + cur_ack32; >>> - if (unlikely(before(cur_ack32, old_ack32))) >>> + if (unlikely(cur_ack32 < old_ack32)) >>> return cur_ack + (1LL << 32); >>> return cur_ack; >>> } >> >> I think you're right, but I haven't had a chance to test myself (since >> this is tagged as closing #204 it sounds like your tests are good!). >> >> While it's a good thing that mptcp_trunk helped us find this problem, >> maybe this also shows that mptcp_trunk should be more aggressive about >> switching to 64-bit ACKs? Or is this less about speed and only a >> wraparound bug? > > Without this patch single subflows transfer from net-next to mptcp.org > stalls after a few GB - after the wrap-around. That happens because > net-next misinterpret the ack for the wrapped seq as a very old one. > This patch fixes the issue in my test. > > Not sure if the above answer your question. That is helpful, thanks. Like we discussed on IRC, the new comparison could result in an old (but recent) ACK getting wrongly interpreted as a "future" 64-bit ACK, but one that is unlikely to be in the correct window - so it gets ignored. Would be good to have a comment explaining this. -- Mat Martineau Intel
diff --git a/net/mptcp/options.c b/net/mptcp/options.c index a05270996613..7db1e219f24f 100644 --- a/net/mptcp/options.c +++ b/net/mptcp/options.c @@ -952,7 +952,7 @@ static u64 expand_ack(u64 old_ack, u64 cur_ack, bool use_64bit) old_ack32 = (u32)old_ack; cur_ack32 = (u32)cur_ack; cur_ack = (old_ack & GENMASK_ULL(63, 32)) + cur_ack32; - if (unlikely(before(cur_ack32, old_ack32))) + if (unlikely(cur_ack32 < old_ack32)) return cur_ack + (1LL << 32); return cur_ack; }
When receiving 32 bits DSS ack from the peer, the MPTCP need to expand them to 64 bits value. The current code is buggy WRT detectiong 32 bits ack wrap-around: usaging the 'before()' helper is just meaningless: when the wrap-around happes the current 32 bit ack value is lower than the previous one, need plain direct comparison. Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/204 Fixes: cc9d25669866 ("mptcp: update per unacked sequence on pkt reception") Signed-off-by: Paolo Abeni <pabeni@redhat.com> --- net/mptcp/options.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)