Re: [PATCH mptcp-net] mptcp: fix 32 bit DSN expansion

[Mat Martineau <mathew.j.martineau@linux.intel.com>]

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