[MPTCP] Re: [PATCH 3/4] mptcp: deal better with out-of-sequence mapping

[MPTCP] Re: [PATCH 3/4] mptcp: deal better with out-of-sequence mapping

[Matthieu Baerts]

[-- Attachment #1: Type: text/plain, Size: 1948 bytes --]

Hi Paolo,

On 17/10/2019 16:03, Paolo Abeni wrote:
> Hi,
> 
> On Thu, 2019-10-17 at 15:22 +0200, Matthieu Baerts wrote:
>> On 17/10/2019 11:00, Paolo Abeni wrote:
> 
> [...]
>>> +static bool skb_is_fully_mapped(struct sock *ssk, struct sk_buff *skb)
>>> +{
>>> +	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
>>> +	unsigned skb_consumed;
>>> +
>>> +	skb_consumed = tcp_sk(ssk)->copied_seq - TCP_SKB_CB(skb)->seq;
>>> +	if (WARN_ON_ONCE(skb_consumed >= skb->len))
>>> +		return true;
>>> +
>>> +	return skb->len - skb_consumed <= subflow->map_data_len -
>>> +					  mptcp_subflow_get_map_offset(subflow);
>>
>> Do we need to do something special here and/or above to deal with
>> wraparounds?
>>
>> By only looking at this chunk of code, it looks like skb_consumed can
>> have a huge value if one of the two seq numbers have done a wraparound.
> 
> Differences are ok even in case of wrap-around. AFAIK, we could have
> problem only if skb_consumed would be signed and the distance between
> 'copied_seq' and 'seq' would grow above 1<<31. So basically we can't ;)
> 
> 'seq' can be greated than 'copied_seq' just in case of some bug
> elsewhere (in TCP and/or in our calls to tcp_read_sock()), and we
> should get a warn on the next line, if so.

Thank you for this explanation!

>> Then we might have a warning.
>> Same below with map_data_len and the offset, no?
> 
> mptcp_subflow_get_map_offset() can be greater of map_data_len only if
> we hit a bug somewhere else. Other places using that algebra currently
> don't warn, so I did not add an explicit check here. I would avoid
> that, to avoid adding more warns in subflow.c and protocol.c

Sounds good to me! Thank you for this note!

Cheers,
Matt
-- 
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium

[MPTCP] Re: [PATCH 3/4] mptcp: deal better with out-of-sequence mapping

[Paolo Abeni]

[-- Attachment #1: Type: text/plain, Size: 1801 bytes --]

Hi,

On Thu, 2019-10-17 at 15:22 +0200, Matthieu Baerts wrote:
> On 17/10/2019 11:00, Paolo Abeni wrote:

[...]
> > +static bool skb_is_fully_mapped(struct sock *ssk, struct sk_buff *skb)
> > +{
> > +	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
> > +	unsigned skb_consumed;
> > +
> > +	skb_consumed = tcp_sk(ssk)->copied_seq - TCP_SKB_CB(skb)->seq;
> > +	if (WARN_ON_ONCE(skb_consumed >= skb->len))
> > +		return true;
> > +
> > +	return skb->len - skb_consumed <= subflow->map_data_len -
> > +					  mptcp_subflow_get_map_offset(subflow);
> 
> Do we need to do something special here and/or above to deal with 
> wraparounds?
> 
> By only looking at this chunk of code, it looks like skb_consumed can 
> have a huge value if one of the two seq numbers have done a wraparound. 

Differences are ok even in case of wrap-around. AFAIK, we could have
problem only if skb_consumed would be signed and the distance between
'copied_seq' and 'seq' would grow above 1<<31. So basically we can't ;)

'seq' can be greated than 'copied_seq' just in case of some bug
elsewhere (in TCP and/or in our calls to tcp_read_sock()), and we
should get a warn on the next line, if so.

> Then we might have a warning.
> Same below with map_data_len and the offset, no?

mptcp_subflow_get_map_offset() can be greater of map_data_len only if
we hit a bug somewhere else. Other places using that algebra currently
don't warn, so I did not add an explicit check here. I would avoid
that, to avoid adding more warns in subflow.c and protocol.c

> For the rest, it looks good to me! I can apply them to avoid problems 
> with the tests (even if for the moment, the tests are not launched 
> because the compilation with i386 and without IPV6 fail :-) )

:(

/P

[MPTCP] Re: [PATCH 3/4] mptcp: deal better with out-of-sequence mapping

[Matthieu Baerts]

[-- Attachment #1: Type: text/plain, Size: 2185 bytes --]

Hi Paolo,

Thank you for the patches!

On 17/10/2019 11:00, Paolo Abeni wrote:
> map colaescing is an evil early optimization, drop it.
> Instead ensure we process correctly any map we receive while
> there is a pending, active, one:
> 
> - allow replacing only with identic mapping
> - explicitly bail if the new map covers only data in future skbs
>    as we don't support map caching (yet)
> 
> Squash-to: "mptcp: Implement MPTCP receive path"
> Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
> ---
>   net/mptcp/subflow.c | 51 ++++++++++++++++++++++-----------------------
>   1 file changed, 25 insertions(+), 26 deletions(-)
> 
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 8e6e0f782f5e..3858187df4ce 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -180,6 +180,19 @@ static void warn_bad_map(struct mptcp_subflow_context *subflow, u32 ssn)
>   		  ssn, subflow->map_subflow_seq, subflow->map_data_len);
>   }
>   
> +static bool skb_is_fully_mapped(struct sock *ssk, struct sk_buff *skb)
> +{
> +	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
> +	unsigned skb_consumed;
> +
> +	skb_consumed = tcp_sk(ssk)->copied_seq - TCP_SKB_CB(skb)->seq; > +	if (WARN_ON_ONCE(skb_consumed >= skb->len))
> +		return true;
> +
> +	return skb->len - skb_consumed <= subflow->map_data_len -
> +					  mptcp_subflow_get_map_offset(subflow);

Do we need to do something special here and/or above to deal with 
wraparounds?

By only looking at this chunk of code, it looks like skb_consumed can 
have a huge value if one of the two seq numbers have done a wraparound. 
Then we might have a warning.
Same below with map_data_len and the offset, no?
But maybe it is not possible in this context.

For the rest, it looks good to me! I can apply them to avoid problems 
with the tests (even if for the moment, the tests are not launched 
because the compilation with i386 and without IPV6 fail :-) )

Cheers,
Matt
-- 
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium