[PATCH mptcp-net] mptcp: fix bad handling od 32 bit ack wrap-around.

[PATCH mptcp-net] mptcp: fix bad handling od 32 bit ack wrap-around.

[Paolo Abeni]

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

Re: [PATCH mptcp-net] mptcp: fix bad handling od 32 bit ack wrap-around.

[Mat Martineau]

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

Re: [PATCH mptcp-net] mptcp: fix bad handling od 32 bit ack wrap-around.

[Paolo Abeni]

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

Re: [PATCH mptcp-net] mptcp: fix bad handling od 32 bit ack wrap-around.

[Mat Martineau]

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