mptcp.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
* [PATCH mptcp-net] mptcp: fix bad handling od 32 bit ack wrap-around.
@ 2021-06-14 17:23 Paolo Abeni
  2021-06-15  1:27 ` Mat Martineau
  0 siblings, 1 reply; 4+ messages in thread
From: Paolo Abeni @ 2021-06-14 17:23 UTC (permalink / raw)
  To: mptcp

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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH mptcp-net] mptcp: fix bad handling od 32 bit ack wrap-around.
  2021-06-14 17:23 [PATCH mptcp-net] mptcp: fix bad handling od 32 bit ack wrap-around Paolo Abeni
@ 2021-06-15  1:27 ` Mat Martineau
  2021-06-15  9:09   ` Paolo Abeni
  0 siblings, 1 reply; 4+ messages in thread
From: Mat Martineau @ 2021-06-15  1:27 UTC (permalink / raw)
  To: Paolo Abeni; +Cc: mptcp

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH mptcp-net] mptcp: fix bad handling od 32 bit ack wrap-around.
  2021-06-15  1:27 ` Mat Martineau
@ 2021-06-15  9:09   ` Paolo Abeni
  2021-06-15 19:34     ` Mat Martineau
  0 siblings, 1 reply; 4+ messages in thread
From: Paolo Abeni @ 2021-06-15  9:09 UTC (permalink / raw)
  To: Mat Martineau; +Cc: mptcp

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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH mptcp-net] mptcp: fix bad handling od 32 bit ack wrap-around.
  2021-06-15  9:09   ` Paolo Abeni
@ 2021-06-15 19:34     ` Mat Martineau
  0 siblings, 0 replies; 4+ messages in thread
From: Mat Martineau @ 2021-06-15 19:34 UTC (permalink / raw)
  To: Paolo Abeni; +Cc: mptcp

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-06-15 19:34 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-14 17:23 [PATCH mptcp-net] mptcp: fix bad handling od 32 bit ack wrap-around Paolo Abeni
2021-06-15  1:27 ` Mat Martineau
2021-06-15  9:09   ` Paolo Abeni
2021-06-15 19:34     ` Mat Martineau

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).