[PATCH mptcp-next v2 04/21] mptcp: establish subflows from either end of connection

Kishen Maloor posted 21 patches 4 months, 1 week ago
Maintainers: "David S. Miller" <davem@davemloft.net>, Matthieu Baerts <matthieu.baerts@tessares.net>, Mat Martineau <mathew.j.martineau@linux.intel.com>, Jakub Kicinski <kuba@kernel.org>, Shuah Khan <shuah@kernel.org>
[PATCH mptcp-next v2 04/21] mptcp: establish subflows from either end of connection
Posted by Kishen Maloor 4 months, 1 week ago
This change updates internal logic to permit subflows to be
established from either the client or server ends of MPTCP
connections. This symmetry and added flexibility may be
harnessed by PM implementations running on either end in
creating new subflows.

The essence of this change lies in not relying on the
"server_side" flag (which continues to be available if needed).

v2: check for 3rd ACK retransmission only on passive side
of the MPJ handshake

Signed-off-by: Kishen Maloor <kishen.maloor@intel.com>
---
 net/mptcp/options.c  | 2 +-
 net/mptcp/protocol.c | 5 +----
 net/mptcp/protocol.h | 2 --
 3 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index cceba8c7806d..89808816db06 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -921,7 +921,7 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk,
 		if (TCP_SKB_CB(skb)->seq == subflow->ssn_offset + 1 &&
 		    TCP_SKB_CB(skb)->end_seq == TCP_SKB_CB(skb)->seq &&
 		    subflow->mp_join && (mp_opt->suboptions & OPTIONS_MPTCP_MPJ) &&
-		    READ_ONCE(msk->pm.server_side))
+		    !subflow->request_join)
 			tcp_send_ack(ssk);
 		goto fully_established;
 	}
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 4012f844eec1..408a05bff633 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -3248,15 +3248,12 @@ bool mptcp_finish_join(struct sock *ssk)
 		return false;
 	}
 
-	if (!msk->pm.server_side)
+	if (!list_empty(&subflow->node))
 		goto out;
 
 	if (!mptcp_pm_allow_new_subflow(msk))
 		goto err_prohibited;
 
-	if (WARN_ON_ONCE(!list_empty(&subflow->node)))
-		goto err_prohibited;
-
 	/* active connections are already on conn_list.
 	 * If we can't acquire msk socket lock here, let the release callback
 	 * handle it
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index e2a67d3469f6..c50247673c7e 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -908,10 +908,8 @@ static inline bool mptcp_check_infinite_map(struct sk_buff *skb)
 static inline bool subflow_simultaneous_connect(struct sock *sk)
 {
 	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
-	struct sock *parent = subflow->conn;
 
 	return sk->sk_state == TCP_ESTABLISHED &&
-	       !mptcp_sk(parent)->pm.server_side &&
 	       !subflow->conn_finished;
 }
 
-- 
2.31.1


Re: [PATCH mptcp-next v2 04/21] mptcp: establish subflows from either end of connection
Posted by Mat Martineau 4 months, 1 week ago
On Wed, 12 Jan 2022, Kishen Maloor wrote:

> This change updates internal logic to permit subflows to be
> established from either the client or server ends of MPTCP
> connections. This symmetry and added flexibility may be
> harnessed by PM implementations running on either end in
> creating new subflows.
>
> The essence of this change lies in not relying on the
> "server_side" flag (which continues to be available if needed).
>

After this patch, server_side is unused except for some debug output. If 
it's really no longer referenced (see below first), may be better to just 
remove it.

> v2: check for 3rd ACK retransmission only on passive side
> of the MPJ handshake
>
> Signed-off-by: Kishen Maloor <kishen.maloor@intel.com>
> ---
> net/mptcp/options.c  | 2 +-
> net/mptcp/protocol.c | 5 +----
> net/mptcp/protocol.h | 2 --
> 3 files changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index cceba8c7806d..89808816db06 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -921,7 +921,7 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk,
> 		if (TCP_SKB_CB(skb)->seq == subflow->ssn_offset + 1 &&
> 		    TCP_SKB_CB(skb)->end_seq == TCP_SKB_CB(skb)->seq &&
> 		    subflow->mp_join && (mp_opt->suboptions & OPTIONS_MPTCP_MPJ) &&
> -		    READ_ONCE(msk->pm.server_side))
> +		    !subflow->request_join)
> 			tcp_send_ack(ssk);
> 		goto fully_established;
> 	}
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 4012f844eec1..408a05bff633 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -3248,15 +3248,12 @@ bool mptcp_finish_join(struct sock *ssk)
> 		return false;
> 	}
>
> -	if (!msk->pm.server_side)
> +	if (!list_empty(&subflow->node))
> 		goto out;
>
> 	if (!mptcp_pm_allow_new_subflow(msk))
> 		goto err_prohibited;
>
> -	if (WARN_ON_ONCE(!list_empty(&subflow->node)))
> -		goto err_prohibited;
> -
> 	/* active connections are already on conn_list.
> 	 * If we can't acquire msk socket lock here, let the release callback
> 	 * handle it
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index e2a67d3469f6..c50247673c7e 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -908,10 +908,8 @@ static inline bool mptcp_check_infinite_map(struct sk_buff *skb)
> static inline bool subflow_simultaneous_connect(struct sock *sk)
> {
> 	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
> -	struct sock *parent = subflow->conn;
>
> 	return sk->sk_state == TCP_ESTABLISHED &&
> -	       !mptcp_sk(parent)->pm.server_side &&
> 	       !subflow->conn_finished;
> }

Are changes in this function needed? This code was added to address
https://github.com/multipath-tcp/mptcp_net-next/issues/35
which was a weird case encountered with syzkaller where a socket tried to 
connect to itself, triggering the "simultaneous open" code path. It 
doesn't seem to be applicable to MP_JOINs.

--
Mat Martineau
Intel

Re: [PATCH mptcp-next v2 04/21] mptcp: establish subflows from either end of connection
Posted by Paolo Abeni 4 months, 1 week ago
On Fri, 2022-01-14 at 14:43 -0800, Mat Martineau wrote:
> On Wed, 12 Jan 2022, Kishen Maloor wrote:
> 
> > This change updates internal logic to permit subflows to be
> > established from either the client or server ends of MPTCP
> > connections. This symmetry and added flexibility may be
> > harnessed by PM implementations running on either end in
> > creating new subflows.
> > 
> > The essence of this change lies in not relying on the
> > "server_side" flag (which continues to be available if needed).
> > 
> 
> After this patch, server_side is unused except for some debug output. If 
> it's really no longer referenced (see below first), may be better to just 
> remove it.
> 
> > v2: check for 3rd ACK retransmission only on passive side
> > of the MPJ handshake
> > 
> > Signed-off-by: Kishen Maloor <kishen.maloor@intel.com>
> > ---
> > net/mptcp/options.c  | 2 +-
> > net/mptcp/protocol.c | 5 +----
> > net/mptcp/protocol.h | 2 --
> > 3 files changed, 2 insertions(+), 7 deletions(-)
> > 
> > diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> > index cceba8c7806d..89808816db06 100644
> > --- a/net/mptcp/options.c
> > +++ b/net/mptcp/options.c
> > @@ -921,7 +921,7 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk,
> > 		if (TCP_SKB_CB(skb)->seq == subflow->ssn_offset + 1 &&
> > 		    TCP_SKB_CB(skb)->end_seq == TCP_SKB_CB(skb)->seq &&
> > 		    subflow->mp_join && (mp_opt->suboptions & OPTIONS_MPTCP_MPJ) &&
> > -		    READ_ONCE(msk->pm.server_side))
> > +		    !subflow->request_join)
> > 			tcp_send_ack(ssk);
> > 		goto fully_established;
> > 	}
> > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > index 4012f844eec1..408a05bff633 100644
> > --- a/net/mptcp/protocol.c
> > +++ b/net/mptcp/protocol.c
> > @@ -3248,15 +3248,12 @@ bool mptcp_finish_join(struct sock *ssk)
> > 		return false;
> > 	}
> > 
> > -	if (!msk->pm.server_side)
> > +	if (!list_empty(&subflow->node))
> > 		goto out;
> > 
> > 	if (!mptcp_pm_allow_new_subflow(msk))
> > 		goto err_prohibited;
> > 
> > -	if (WARN_ON_ONCE(!list_empty(&subflow->node)))
> > -		goto err_prohibited;
> > -
> > 	/* active connections are already on conn_list.
> > 	 * If we can't acquire msk socket lock here, let the release callback
> > 	 * handle it
> > diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> > index e2a67d3469f6..c50247673c7e 100644
> > --- a/net/mptcp/protocol.h
> > +++ b/net/mptcp/protocol.h
> > @@ -908,10 +908,8 @@ static inline bool mptcp_check_infinite_map(struct sk_buff *skb)
> > static inline bool subflow_simultaneous_connect(struct sock *sk)
> > {
> > 	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
> > -	struct sock *parent = subflow->conn;
> > 
> > 	return sk->sk_state == TCP_ESTABLISHED &&
> > -	       !mptcp_sk(parent)->pm.server_side &&
> > 	       !subflow->conn_finished;
> > }
> 
> Are changes in this function needed? This code was added to address
> https://github.com/multipath-tcp/mptcp_net-next/issues/35
> which was a weird case encountered with syzkaller where a socket tried to 
> connect to itself, triggering the "simultaneous open" code path. It 
> doesn't seem to be applicable to MP_JOINs.

I *think* we can reach here even with MPJ, e.g. if we have port-based
endpoint matching the src address/port - possibly syzkaller could be
able to reach such scenario.

Additionally I fear dropping the '!mptcp_sk(parent)->pm.server_side &&'
check could re-introduce the initial issue, at least for the MPC
handshake.

/P


Re: [PATCH mptcp-next v2 04/21] mptcp: establish subflows from either end of connection
Posted by Kishen Maloor 4 months ago
On 1/17/22 12:59 AM, Paolo Abeni wrote:
> On Fri, 2022-01-14 at 14:43 -0800, Mat Martineau wrote:
>> On Wed, 12 Jan 2022, Kishen Maloor wrote:
>>
>>> This change updates internal logic to permit subflows to be
>>> established from either the client or server ends of MPTCP
>>> connections. This symmetry and added flexibility may be
>>> harnessed by PM implementations running on either end in
>>> creating new subflows.
>>>
>>> The essence of this change lies in not relying on the
>>> "server_side" flag (which continues to be available if needed).
>>>
>>
>> After this patch, server_side is unused except for some debug output. If 
>> it's really no longer referenced (see below first), may be better to just 
>> remove it.

I think we'll need it. 

One possible use is to inform the PM daemon of the type of application 
(client/server) associated with a connection.

As we're discussing the idea of creating listening sockets only upon request, the 
knowledge that we are on the client end of a connection is something a PM daemon could
use to request creation of listening sockets whenever it advertises an address.

>>
>>> v2: check for 3rd ACK retransmission only on passive side
>>> of the MPJ handshake
>>>
>>> Signed-off-by: Kishen Maloor <kishen.maloor@intel.com>
>>> ---
>>> net/mptcp/options.c  | 2 +-
>>> net/mptcp/protocol.c | 5 +----
>>> net/mptcp/protocol.h | 2 --
>>> 3 files changed, 2 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>>> index cceba8c7806d..89808816db06 100644
>>> --- a/net/mptcp/options.c
>>> +++ b/net/mptcp/options.c
>>> @@ -921,7 +921,7 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk,
>>> 		if (TCP_SKB_CB(skb)->seq == subflow->ssn_offset + 1 &&
>>> 		    TCP_SKB_CB(skb)->end_seq == TCP_SKB_CB(skb)->seq &&
>>> 		    subflow->mp_join && (mp_opt->suboptions & OPTIONS_MPTCP_MPJ) &&
>>> -		    READ_ONCE(msk->pm.server_side))
>>> +		    !subflow->request_join)
>>> 			tcp_send_ack(ssk);
>>> 		goto fully_established;
>>> 	}
>>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>>> index 4012f844eec1..408a05bff633 100644
>>> --- a/net/mptcp/protocol.c
>>> +++ b/net/mptcp/protocol.c
>>> @@ -3248,15 +3248,12 @@ bool mptcp_finish_join(struct sock *ssk)
>>> 		return false;
>>> 	}
>>>
>>> -	if (!msk->pm.server_side)
>>> +	if (!list_empty(&subflow->node))
>>> 		goto out;
>>>
>>> 	if (!mptcp_pm_allow_new_subflow(msk))
>>> 		goto err_prohibited;
>>>
>>> -	if (WARN_ON_ONCE(!list_empty(&subflow->node)))
>>> -		goto err_prohibited;
>>> -
>>> 	/* active connections are already on conn_list.
>>> 	 * If we can't acquire msk socket lock here, let the release callback
>>> 	 * handle it
>>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
>>> index e2a67d3469f6..c50247673c7e 100644
>>> --- a/net/mptcp/protocol.h
>>> +++ b/net/mptcp/protocol.h
>>> @@ -908,10 +908,8 @@ static inline bool mptcp_check_infinite_map(struct sk_buff *skb)
>>> static inline bool subflow_simultaneous_connect(struct sock *sk)
>>> {
>>> 	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
>>> -	struct sock *parent = subflow->conn;
>>>
>>> 	return sk->sk_state == TCP_ESTABLISHED &&
>>> -	       !mptcp_sk(parent)->pm.server_side &&
>>> 	       !subflow->conn_finished;
>>> }
>>
>> Are changes in this function needed? This code was added to address
>> https://github.com/multipath-tcp/mptcp_net-next/issues/35
>> which was a weird case encountered with syzkaller where a socket tried to 
>> connect to itself, triggering the "simultaneous open" code path. It 
>> doesn't seem to be applicable to MP_JOINs.
> 
> I *think* we can reach here even with MPJ, e.g. if we have port-based
> endpoint matching the src address/port - possibly syzkaller could be
> able to reach such scenario.
> 
> Additionally I fear dropping the '!mptcp_sk(parent)->pm.server_side &&'
> check could re-introduce the initial issue, at least for the MPC
> handshake.

The purpose of this commit is to permit the MPJ sequence to happen at either end of 
the connection. The change in mptcp_finish_join() had the most direct impact for enabling 
this, so I looked at other server_side checks mostly in terms of establishing that
bi-directional symmetry.

If the above condition holds only with MPCs, i.e. for the initial connection, then I think it would 
be fine to preserve the server_side check. But if it also holds for MPJs, then we might
have to account for both cases. Perhaps replace:

!mptcp_sk(parent)->pm.server_side)

with something like this?

((subflow->request_mptcp && !mptcp_sk(parent)->pm.server_side) ||
	subflow->request_join)

> 
> /P
> 


Re: [PATCH mptcp-next v2 04/21] mptcp: establish subflows from either end of connection
Posted by Paolo Abeni 4 months ago
On Tue, 2022-01-18 at 17:26 -0800, Kishen Maloor wrote:
> On 1/17/22 12:59 AM, Paolo Abeni wrote:
> > On Fri, 2022-01-14 at 14:43 -0800, Mat Martineau wrote:
> > > On Wed, 12 Jan 2022, Kishen Maloor wrote:
> > > 
> > > > This change updates internal logic to permit subflows to be
> > > > established from either the client or server ends of MPTCP
> > > > connections. This symmetry and added flexibility may be
> > > > harnessed by PM implementations running on either end in
> > > > creating new subflows.
> > > > 
> > > > The essence of this change lies in not relying on the
> > > > "server_side" flag (which continues to be available if needed).
> > > > 
> > > 
> > > After this patch, server_side is unused except for some debug output. If 
> > > it's really no longer referenced (see below first), may be better to just 
> > > remove it.
> 
> I think we'll need it. 
> 
> One possible use is to inform the PM daemon of the type of application 
> (client/server) associated with a connection.
> 
> As we're discussing the idea of creating listening sockets only upon request, the 
> knowledge that we are on the client end of a connection is something a PM daemon could
> use to request creation of listening sockets whenever it advertises an address.
> 
> > > 
> > > > v2: check for 3rd ACK retransmission only on passive side
> > > > of the MPJ handshake
> > > > 
> > > > Signed-off-by: Kishen Maloor <kishen.maloor@intel.com>
> > > > ---
> > > > net/mptcp/options.c  | 2 +-
> > > > net/mptcp/protocol.c | 5 +----
> > > > net/mptcp/protocol.h | 2 --
> > > > 3 files changed, 2 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> > > > index cceba8c7806d..89808816db06 100644
> > > > --- a/net/mptcp/options.c
> > > > +++ b/net/mptcp/options.c
> > > > @@ -921,7 +921,7 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk,
> > > > 		if (TCP_SKB_CB(skb)->seq == subflow->ssn_offset + 1 &&
> > > > 		    TCP_SKB_CB(skb)->end_seq == TCP_SKB_CB(skb)->seq &&
> > > > 		    subflow->mp_join && (mp_opt->suboptions & OPTIONS_MPTCP_MPJ) &&
> > > > -		    READ_ONCE(msk->pm.server_side))
> > > > +		    !subflow->request_join)
> > > > 			tcp_send_ack(ssk);
> > > > 		goto fully_established;
> > > > 	}
> > > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > > > index 4012f844eec1..408a05bff633 100644
> > > > --- a/net/mptcp/protocol.c
> > > > +++ b/net/mptcp/protocol.c
> > > > @@ -3248,15 +3248,12 @@ bool mptcp_finish_join(struct sock *ssk)
> > > > 		return false;
> > > > 	}
> > > > 
> > > > -	if (!msk->pm.server_side)
> > > > +	if (!list_empty(&subflow->node))
> > > > 		goto out;
> > > > 
> > > > 	if (!mptcp_pm_allow_new_subflow(msk))
> > > > 		goto err_prohibited;
> > > > 
> > > > -	if (WARN_ON_ONCE(!list_empty(&subflow->node)))
> > > > -		goto err_prohibited;
> > > > -
> > > > 	/* active connections are already on conn_list.
> > > > 	 * If we can't acquire msk socket lock here, let the release callback
> > > > 	 * handle it
> > > > diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> > > > index e2a67d3469f6..c50247673c7e 100644
> > > > --- a/net/mptcp/protocol.h
> > > > +++ b/net/mptcp/protocol.h
> > > > @@ -908,10 +908,8 @@ static inline bool mptcp_check_infinite_map(struct sk_buff *skb)
> > > > static inline bool subflow_simultaneous_connect(struct sock *sk)
> > > > {
> > > > 	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
> > > > -	struct sock *parent = subflow->conn;
> > > > 
> > > > 	return sk->sk_state == TCP_ESTABLISHED &&
> > > > -	       !mptcp_sk(parent)->pm.server_side &&
> > > > 	       !subflow->conn_finished;
> > > > }
> > > 
> > > Are changes in this function needed? This code was added to address
> > > https://github.com/multipath-tcp/mptcp_net-next/issues/35
> > > which was a weird case encountered with syzkaller where a socket tried to 
> > > connect to itself, triggering the "simultaneous open" code path. It 
> > > doesn't seem to be applicable to MP_JOINs.
> > 
> > I *think* we can reach here even with MPJ, e.g. if we have port-based
> > endpoint matching the src address/port - possibly syzkaller could be
> > able to reach such scenario.
> > 
> > Additionally I fear dropping the '!mptcp_sk(parent)->pm.server_side &&'
> > check could re-introduce the initial issue, at least for the MPC
> > handshake.
> 
> The purpose of this commit is to permit the MPJ sequence to happen at either end of 
> the connection. The change in mptcp_finish_join() had the most direct impact for enabling 
> this, so I looked at other server_side checks mostly in terms of establishing that
> bi-directional symmetry.
> 
> If the above condition holds only with MPCs, i.e. for the initial connection, then I think it would 
> be fine to preserve the server_side check. But if it also holds for MPJs, then we might
> have to account for both cases. Perhaps replace:
> 
> !mptcp_sk(parent)->pm.server_side)
> 
> with something like this?
> 
> ((subflow->request_mptcp && !mptcp_sk(parent)->pm.server_side) ||
> 	subflow->request_join)

It looks like 'request_mptcp' is 0 on passive sockets, so:

 	subflow->request_mptcp || subflow->request_join

should suffice.

Possibly factoring out an helper for the above condition would make the
code more readable.

/P


Re: [PATCH mptcp-next v2 04/21] mptcp: establish subflows from either end of connection
Posted by Kishen Maloor 4 months ago
On 1/19/22 4:01 AM, Paolo Abeni wrote:
> On Tue, 2022-01-18 at 17:26 -0800, Kishen Maloor wrote:
>> On 1/17/22 12:59 AM, Paolo Abeni wrote:
>>> On Fri, 2022-01-14 at 14:43 -0800, Mat Martineau wrote:
>>>> On Wed, 12 Jan 2022, Kishen Maloor wrote:
>>>>
>>>>> This change updates internal logic to permit subflows to be
>>>>> established from either the client or server ends of MPTCP
>>>>> connections. This symmetry and added flexibility may be
>>>>> harnessed by PM implementations running on either end in
>>>>> creating new subflows.
>>>>>
>>>>> The essence of this change lies in not relying on the
>>>>> "server_side" flag (which continues to be available if needed).
>>>>>
>>>>
>>>> After this patch, server_side is unused except for some debug output. If 
>>>> it's really no longer referenced (see below first), may be better to just 
>>>> remove it.
>>
>> I think we'll need it. 
>>
>> One possible use is to inform the PM daemon of the type of application 
>> (client/server) associated with a connection.
>>
>> As we're discussing the idea of creating listening sockets only upon request, the 
>> knowledge that we are on the client end of a connection is something a PM daemon could
>> use to request creation of listening sockets whenever it advertises an address.
>>
>>>>
>>>>> v2: check for 3rd ACK retransmission only on passive side
>>>>> of the MPJ handshake
>>>>>
>>>>> Signed-off-by: Kishen Maloor <kishen.maloor@intel.com>
>>>>> ---
>>>>> net/mptcp/options.c  | 2 +-
>>>>> net/mptcp/protocol.c | 5 +----
>>>>> net/mptcp/protocol.h | 2 --
>>>>> 3 files changed, 2 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>>>>> index cceba8c7806d..89808816db06 100644
>>>>> --- a/net/mptcp/options.c
>>>>> +++ b/net/mptcp/options.c
>>>>> @@ -921,7 +921,7 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk,
>>>>> 		if (TCP_SKB_CB(skb)->seq == subflow->ssn_offset + 1 &&
>>>>> 		    TCP_SKB_CB(skb)->end_seq == TCP_SKB_CB(skb)->seq &&
>>>>> 		    subflow->mp_join && (mp_opt->suboptions & OPTIONS_MPTCP_MPJ) &&
>>>>> -		    READ_ONCE(msk->pm.server_side))
>>>>> +		    !subflow->request_join)
>>>>> 			tcp_send_ack(ssk);
>>>>> 		goto fully_established;
>>>>> 	}
>>>>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>>>>> index 4012f844eec1..408a05bff633 100644
>>>>> --- a/net/mptcp/protocol.c
>>>>> +++ b/net/mptcp/protocol.c
>>>>> @@ -3248,15 +3248,12 @@ bool mptcp_finish_join(struct sock *ssk)
>>>>> 		return false;
>>>>> 	}
>>>>>
>>>>> -	if (!msk->pm.server_side)
>>>>> +	if (!list_empty(&subflow->node))
>>>>> 		goto out;
>>>>>
>>>>> 	if (!mptcp_pm_allow_new_subflow(msk))
>>>>> 		goto err_prohibited;
>>>>>
>>>>> -	if (WARN_ON_ONCE(!list_empty(&subflow->node)))
>>>>> -		goto err_prohibited;
>>>>> -
>>>>> 	/* active connections are already on conn_list.
>>>>> 	 * If we can't acquire msk socket lock here, let the release callback
>>>>> 	 * handle it
>>>>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
>>>>> index e2a67d3469f6..c50247673c7e 100644
>>>>> --- a/net/mptcp/protocol.h
>>>>> +++ b/net/mptcp/protocol.h
>>>>> @@ -908,10 +908,8 @@ static inline bool mptcp_check_infinite_map(struct sk_buff *skb)
>>>>> static inline bool subflow_simultaneous_connect(struct sock *sk)
>>>>> {
>>>>> 	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
>>>>> -	struct sock *parent = subflow->conn;
>>>>>
>>>>> 	return sk->sk_state == TCP_ESTABLISHED &&
>>>>> -	       !mptcp_sk(parent)->pm.server_side &&
>>>>> 	       !subflow->conn_finished;
>>>>> }
>>>>
>>>> Are changes in this function needed? This code was added to address
>>>> https://github.com/multipath-tcp/mptcp_net-next/issues/35
>>>> which was a weird case encountered with syzkaller where a socket tried to 
>>>> connect to itself, triggering the "simultaneous open" code path. It 
>>>> doesn't seem to be applicable to MP_JOINs.
>>>
>>> I *think* we can reach here even with MPJ, e.g. if we have port-based
>>> endpoint matching the src address/port - possibly syzkaller could be
>>> able to reach such scenario.
>>>
>>> Additionally I fear dropping the '!mptcp_sk(parent)->pm.server_side &&'
>>> check could re-introduce the initial issue, at least for the MPC
>>> handshake.
>>
>> The purpose of this commit is to permit the MPJ sequence to happen at either end of 
>> the connection. The change in mptcp_finish_join() had the most direct impact for enabling 
>> this, so I looked at other server_side checks mostly in terms of establishing that
>> bi-directional symmetry.
>>
>> If the above condition holds only with MPCs, i.e. for the initial connection, then I think it would 
>> be fine to preserve the server_side check. But if it also holds for MPJs, then we might
>> have to account for both cases. Perhaps replace:
>>
>> !mptcp_sk(parent)->pm.server_side)
>>
>> with something like this?
>>
>> ((subflow->request_mptcp && !mptcp_sk(parent)->pm.server_side) ||
>> 	subflow->request_join)
> 
> It looks like 'request_mptcp' is 0 on passive sockets, so:
> 
>  	subflow->request_mptcp || subflow->request_join
> 
> should suffice.

That's right. Makes sense.

> 
> Possibly factoring out an helper for the above condition would make the
> code more readable.

Thanks! Will do so.

> 
> /P
> 


Re: [PATCH mptcp-next v2 04/21] mptcp: establish subflows from either end of connection
Posted by Mat Martineau 4 months ago
On Wed, 19 Jan 2022, Kishen Maloor wrote:

> On 1/19/22 4:01 AM, Paolo Abeni wrote:
>> On Tue, 2022-01-18 at 17:26 -0800, Kishen Maloor wrote:
>>> On 1/17/22 12:59 AM, Paolo Abeni wrote:
>>>> On Fri, 2022-01-14 at 14:43 -0800, Mat Martineau wrote:
>>>>> On Wed, 12 Jan 2022, Kishen Maloor wrote:
>>>>>
>>>>>> This change updates internal logic to permit subflows to be
>>>>>> established from either the client or server ends of MPTCP
>>>>>> connections. This symmetry and added flexibility may be
>>>>>> harnessed by PM implementations running on either end in
>>>>>> creating new subflows.
>>>>>>
>>>>>> The essence of this change lies in not relying on the
>>>>>> "server_side" flag (which continues to be available if needed).
>>>>>>
>>>>>
>>>>> After this patch, server_side is unused except for some debug output. If
>>>>> it's really no longer referenced (see below first), may be better to just
>>>>> remove it.
>>>
>>> I think we'll need it.
>>>
>>> One possible use is to inform the PM daemon of the type of application
>>> (client/server) associated with a connection.
>>>
>>> As we're discussing the idea of creating listening sockets only upon request, the
>>> knowledge that we are on the client end of a connection is something a PM daemon could
>>> use to request creation of listening sockets whenever it advertises an address.
>>>

Ok, makes sense.

>>>>>
>>>>>> v2: check for 3rd ACK retransmission only on passive side
>>>>>> of the MPJ handshake
>>>>>>
>>>>>> Signed-off-by: Kishen Maloor <kishen.maloor@intel.com>
>>>>>> ---
>>>>>> net/mptcp/options.c  | 2 +-
>>>>>> net/mptcp/protocol.c | 5 +----
>>>>>> net/mptcp/protocol.h | 2 --
>>>>>> 3 files changed, 2 insertions(+), 7 deletions(-)
>>>>>>
>>>>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>>>>>> index cceba8c7806d..89808816db06 100644
>>>>>> --- a/net/mptcp/options.c
>>>>>> +++ b/net/mptcp/options.c
>>>>>> @@ -921,7 +921,7 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk,
>>>>>> 		if (TCP_SKB_CB(skb)->seq == subflow->ssn_offset + 1 &&
>>>>>> 		    TCP_SKB_CB(skb)->end_seq == TCP_SKB_CB(skb)->seq &&
>>>>>> 		    subflow->mp_join && (mp_opt->suboptions & OPTIONS_MPTCP_MPJ) &&
>>>>>> -		    READ_ONCE(msk->pm.server_side))
>>>>>> +		    !subflow->request_join)
>>>>>> 			tcp_send_ack(ssk);
>>>>>> 		goto fully_established;
>>>>>> 	}
>>>>>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>>>>>> index 4012f844eec1..408a05bff633 100644
>>>>>> --- a/net/mptcp/protocol.c
>>>>>> +++ b/net/mptcp/protocol.c
>>>>>> @@ -3248,15 +3248,12 @@ bool mptcp_finish_join(struct sock *ssk)
>>>>>> 		return false;
>>>>>> 	}
>>>>>>
>>>>>> -	if (!msk->pm.server_side)
>>>>>> +	if (!list_empty(&subflow->node))
>>>>>> 		goto out;
>>>>>>
>>>>>> 	if (!mptcp_pm_allow_new_subflow(msk))
>>>>>> 		goto err_prohibited;
>>>>>>
>>>>>> -	if (WARN_ON_ONCE(!list_empty(&subflow->node)))
>>>>>> -		goto err_prohibited;
>>>>>> -
>>>>>> 	/* active connections are already on conn_list.
>>>>>> 	 * If we can't acquire msk socket lock here, let the release callback
>>>>>> 	 * handle it
>>>>>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
>>>>>> index e2a67d3469f6..c50247673c7e 100644
>>>>>> --- a/net/mptcp/protocol.h
>>>>>> +++ b/net/mptcp/protocol.h
>>>>>> @@ -908,10 +908,8 @@ static inline bool mptcp_check_infinite_map(struct sk_buff *skb)
>>>>>> static inline bool subflow_simultaneous_connect(struct sock *sk)
>>>>>> {
>>>>>> 	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
>>>>>> -	struct sock *parent = subflow->conn;
>>>>>>
>>>>>> 	return sk->sk_state == TCP_ESTABLISHED &&
>>>>>> -	       !mptcp_sk(parent)->pm.server_side &&
>>>>>> 	       !subflow->conn_finished;
>>>>>> }
>>>>>
>>>>> Are changes in this function needed? This code was added to address
>>>>> https://github.com/multipath-tcp/mptcp_net-next/issues/35
>>>>> which was a weird case encountered with syzkaller where a socket tried to
>>>>> connect to itself, triggering the "simultaneous open" code path. It
>>>>> doesn't seem to be applicable to MP_JOINs.
>>>>
>>>> I *think* we can reach here even with MPJ, e.g. if we have port-based
>>>> endpoint matching the src address/port - possibly syzkaller could be
>>>> able to reach such scenario.
>>>>
>>>> Additionally I fear dropping the '!mptcp_sk(parent)->pm.server_side &&'
>>>> check could re-introduce the initial issue, at least for the MPC
>>>> handshake.
>>>
>>> The purpose of this commit is to permit the MPJ sequence to happen at either end of
>>> the connection. The change in mptcp_finish_join() had the most direct impact for enabling
>>> this, so I looked at other server_side checks mostly in terms of establishing that
>>> bi-directional symmetry.
>>>
>>> If the above condition holds only with MPCs, i.e. for the initial connection, then I think it would
>>> be fine to preserve the server_side check. But if it also holds for MPJs, then we might
>>> have to account for both cases. Perhaps replace:
>>>
>>> !mptcp_sk(parent)->pm.server_side)
>>>
>>> with something like this?
>>>
>>> ((subflow->request_mptcp && !mptcp_sk(parent)->pm.server_side) ||
>>> 	subflow->request_join)
>>
>> It looks like 'request_mptcp' is 0 on passive sockets, so:
>>
>>  	subflow->request_mptcp || subflow->request_join
>>
>> should suffice.
>
> That's right. Makes sense.
>

Yeah, good to simplify this even if 'pm.server_side' is still around.

>>
>> Possibly factoring out an helper for the above condition would make the
>> code more readable.
>
> Thanks! Will do so.
>



--
Mat Martineau
Intel