mptcp.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
* [PATCH mptcp-net] mptcp: fix 32 bit DSN expansion
@ 2021-06-15 16:44 Paolo Abeni
  2021-06-16  1:28 ` Mat Martineau
  0 siblings, 1 reply; 2+ messages in thread
From: Paolo Abeni @ 2021-06-15 16:44 UTC (permalink / raw)
  To: mptcp; +Cc: Christoph Paasch

The current implementation of 32 bit DNS expansion is buggy,
and the fix is quite similar to what we did for ack expansion.

There is a small caveat: DNS can both increment and decrement
(on MPTCP re-injection) so we need to use more care to catch
wrap-around and we must additionally look for reverse wrap.

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/120
Fixes: 648ef4b88673 ("mptcp: Implement MPTCP receive path")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
@Christoph: sorry for the duplicate, I used a bad recipient
list for the previous attempt
---
 net/mptcp/subflow.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index d55f4ef736a5..004718126345 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -781,13 +781,19 @@ enum mapping_status {
 	MAPPING_DUMMY
 };
 
-static u64 expand_seq(u64 old_seq, u16 old_data_len, u64 seq)
+static u64 expand_seq(u64 old_seq, u64 cur_seq)
 {
-	if ((u32)seq == (u32)old_seq)
-		return old_seq;
+	u32 old_seq32 = (u32)old_seq;
+	u32 cur_seq32 = (u32)cur_seq;
 
-	/* Assume map covers data not mapped yet. */
-	return seq | ((old_seq + old_data_len + 1) & GENMASK_ULL(63, 32));
+	cur_seq = (old_seq & GENMASK_ULL(63, 32)) + cur_seq32;
+	if (unlikely(cur_seq32 < old_seq32 && before(old_seq32, cur_seq32)))
+		return cur_seq + (1LL << 32);
+
+	/* on re-injection we can have wrap around towards bottom */
+	if (unlikely(cur_seq32 > old_seq32 && after(old_seq32, cur_seq32)))
+		return cur_seq - (1LL << 32);
+	return cur_seq;
 }
 
 static void dbg_bad_map(struct mptcp_subflow_context *subflow, u32 ssn)
@@ -996,9 +1002,8 @@ static enum mapping_status get_mapping_status(struct sock *ssk,
 	}
 
 	if (!mpext->dsn64) {
-		map_seq = expand_seq(subflow->map_seq, subflow->map_data_len,
-				     mpext->data_seq);
-		pr_debug("expanded seq=%llu", subflow->map_seq);
+		map_seq = expand_seq(READ_ONCE(msk->ack_seq), mpext->data_seq);
+		pr_debug("expanded seq=%llu->%llu", mpext->data_seq, map_seq);
 	} else {
 		map_seq = mpext->data_seq;
 	}
-- 
2.26.3


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

* Re: [PATCH mptcp-net] mptcp: fix 32 bit DSN expansion
  2021-06-15 16:44 [PATCH mptcp-net] mptcp: fix 32 bit DSN expansion Paolo Abeni
@ 2021-06-16  1:28 ` Mat Martineau
  0 siblings, 0 replies; 2+ messages in thread
From: Mat Martineau @ 2021-06-16  1:28 UTC (permalink / raw)
  To: Paolo Abeni; +Cc: mptcp, Christoph Paasch

On Tue, 15 Jun 2021, Paolo Abeni wrote:

> The current implementation of 32 bit DNS expansion is buggy,

The "DNS" here (and below) puzzled me for a minute :)

> and the fix is quite similar to what we did for ack expansion.
>
> There is a small caveat: DNS can both increment and decrement
> (on MPTCP re-injection) so we need to use more care to catch
> wrap-around and we must additionally look for reverse wrap.
>
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/120
> Fixes: 648ef4b88673 ("mptcp: Implement MPTCP receive path")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> @Christoph: sorry for the duplicate, I used a bad recipient
> list for the previous attempt
> ---
> net/mptcp/subflow.c | 21 +++++++++++++--------
> 1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index d55f4ef736a5..004718126345 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -781,13 +781,19 @@ enum mapping_status {
> 	MAPPING_DUMMY
> };
>
> -static u64 expand_seq(u64 old_seq, u16 old_data_len, u64 seq)
> +static u64 expand_seq(u64 old_seq, u64 cur_seq)
> {
> -	if ((u32)seq == (u32)old_seq)
> -		return old_seq;
> +	u32 old_seq32 = (u32)old_seq;
> +	u32 cur_seq32 = (u32)cur_seq;
>
> -	/* Assume map covers data not mapped yet. */

This comment dates back to the original version of this function making 
single-subflow assumptions. The code did need to be revisited so thanks 
for working on this.

> -	return seq | ((old_seq + old_data_len + 1) & GENMASK_ULL(63, 32));
> +	cur_seq = (old_seq & GENMASK_ULL(63, 32)) + cur_seq32;
> +	if (unlikely(cur_seq32 < old_seq32 && before(old_seq32, cur_seq32)))
> +		return cur_seq + (1LL << 32);

The above should catch sequence numbers that look like they both "moved 
forward" and "wrapped around". Looks good.

If the adjustment is incorrect, then we're mistaking an old mapping for a 
new one - but only if it happens to be in-window.

> +
> +	/* on re-injection we can have wrap around towards bottom */
> +	if (unlikely(cur_seq32 > old_seq32 && after(old_seq32, cur_seq32)))
> +		return cur_seq - (1LL << 32);

And this logic looks ok for finding old or reinjected mappings that 
wrapped. The data will end up discarded, so if we guess wrong then the 
peer will send again if needed.

Looks good for the export branch, we can test some more there:

Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>

> +	return cur_seq;
> }
>
> static void dbg_bad_map(struct mptcp_subflow_context *subflow, u32 ssn)
> @@ -996,9 +1002,8 @@ static enum mapping_status get_mapping_status(struct sock *ssk,
> 	}
>
> 	if (!mpext->dsn64) {
> -		map_seq = expand_seq(subflow->map_seq, subflow->map_data_len,
> -				     mpext->data_seq);
> -		pr_debug("expanded seq=%llu", subflow->map_seq);
> +		map_seq = expand_seq(READ_ONCE(msk->ack_seq), mpext->data_seq);
> +		pr_debug("expanded seq=%llu->%llu", mpext->data_seq, map_seq);
> 	} else {
> 		map_seq = mpext->data_seq;
> 	}
> -- 
> 2.26.3
>
>
>

--
Mat Martineau
Intel

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

end of thread, other threads:[~2021-06-16  1:28 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-15 16:44 [PATCH mptcp-net] mptcp: fix 32 bit DSN expansion Paolo Abeni
2021-06-16  1:28 ` 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).