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).

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

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index cceba8c7806d..ee13bb46dc38 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -920,8 +920,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->mp_join && (mp_opt->suboptions & OPTIONS_MPTCP_MPJ))
 			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

Hello,

On Thu, 2021-12-16 at 17:22 -0500, 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).
> 
> Signed-off-by: Kishen Maloor <kishen.maloor@intel.com>
> ---
>  net/mptcp/options.c  | 3 +--
>  net/mptcp/protocol.c | 5 +----
>  net/mptcp/protocol.h | 2 --
>  3 files changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index cceba8c7806d..ee13bb46dc38 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -920,8 +920,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->mp_join && (mp_opt->suboptions & OPTIONS_MPTCP_MPJ))
>  			tcp_send_ack(ssk);

This change looks dangerous to me ?!? Or at least would the client to
send an unneeded TCP pure ack as the 5th pkt in the MPJ handshake ?!?

I think we should still try to invoke tcp_send_ack() only if this peer
is passive side of the MPJ handshake. Possibly we need to use an
additional status bit in mptcp_subflow_context to track that.


Thanks!

Paolo

On 12/17/21 9:41 AM, Paolo Abeni wrote:
> Hello,
> 
> On Thu, 2021-12-16 at 17:22 -0500, 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).
>>
>> Signed-off-by: Kishen Maloor <kishen.maloor@intel.com>
>> ---
>>  net/mptcp/options.c  | 3 +--
>>  net/mptcp/protocol.c | 5 +----
>>  net/mptcp/protocol.h | 2 --
>>  3 files changed, 2 insertions(+), 8 deletions(-)
>>
>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>> index cceba8c7806d..ee13bb46dc38 100644
>> --- a/net/mptcp/options.c
>> +++ b/net/mptcp/options.c
>> @@ -920,8 +920,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->mp_join && (mp_opt->suboptions & OPTIONS_MPTCP_MPJ))
>>  			tcp_send_ack(ssk);
> 
> This change looks dangerous to me ?!? Or at least would the client to
> send an unneeded TCP pure ack as the 5th pkt in the MPJ handshake ?!?
> 

The purpose of this overall commit is to allow subflows to be established from either end,
i.e. irrespective of the client/server roles of the MPTCP application above.

> I think we should still try to invoke tcp_send_ack() only if this peer
> is passive side of the MPJ handshake. Possibly we need to use an
> additional status bit in mptcp_subflow_context to track that.
> 

Yes, possibly, if that mitigates the concern you raised. It does sound like your
suggestion would still keep with the goal of this commit.

> 
> Thanks!
> 
> Paolo
>