[MPTCP][PATCH mptcp-next] Squash to "mptcp: add deny_join_id0 in mptcp_options_received"

[MPTCP][PATCH mptcp-next] Squash to "mptcp: add deny_join_id0 in mptcp_options_received"

[Geliang Tang]

Move the deny_join_id0 test into check_fully_established as Paolo
suggested.

Signed-off-by: Geliang Tang <geliangtang@gmail.com>
---
 net/mptcp/options.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 0d30008f0313..63c1e18d61d5 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -910,6 +910,9 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk,
 		return false;
 	}
 
+	if (mp_opt->deny_join_id0)
+		WRITE_ONCE(msk->pm.remote_deny_join_id0, true);
+
 	if (unlikely(!READ_ONCE(msk->pm.server_side)))
 		pr_warn_once("bogus mpc option on established client sk");
 	mptcp_subflow_fully_established(subflow, mp_opt);
@@ -1051,8 +1054,6 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
 	}
 
 	mptcp_get_options(sk, skb, &mp_opt);
-	if (mp_opt.deny_join_id0)
-		WRITE_ONCE(msk->pm.remote_deny_join_id0, true);
 	if (!check_fully_established(msk, sk, subflow, skb, &mp_opt))
 		return;
 
-- 
2.31.1

Re: [MPTCP][PATCH mptcp-next] Squash to "mptcp: add deny_join_id0 in mptcp_options_received"

[Mat Martineau]

On Thu, 10 Jun 2021, Geliang Tang wrote:

> Move the deny_join_id0 test into check_fully_established as Paolo
> suggested.
>
> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> ---
> net/mptcp/options.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 0d30008f0313..63c1e18d61d5 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -910,6 +910,9 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk,
> 		return false;
> 	}
>
> +	if (mp_opt->deny_join_id0)
> +		WRITE_ONCE(msk->pm.remote_deny_join_id0, true);
> +
> 	if (unlikely(!READ_ONCE(msk->pm.server_side)))
> 		pr_warn_once("bogus mpc option on established client sk");
> 	mptcp_subflow_fully_established(subflow, mp_opt);
> @@ -1051,8 +1054,6 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
> 	}
>
> 	mptcp_get_options(sk, skb, &mp_opt);
> -	if (mp_opt.deny_join_id0)
> -		WRITE_ONCE(msk->pm.remote_deny_join_id0, true);
> 	if (!check_fully_established(msk, sk, subflow, skb, &mp_opt))
> 		return;
>
> -- 
> 2.31.1

Looks good, thanks Geliang.

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

--
Mat Martineau
Intel

Re: [MPTCP][PATCH mptcp-next] Squash to "mptcp: add deny_join_id0 in mptcp_options_received"

[Mat Martineau]

On Tue, 18 May 2021, Mat Martineau wrote:

> On Tue, 18 May 2021, Geliang Tang wrote:
>
>> Please add this line to the commit log:
>> 
>> '''
>> In mptcp_finish_join, add the incomming join address check too.
>> '''
>> 
>> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
>> ---
>> net/mptcp/protocol.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>> index c725e8f02533..5cebecc838ca 100644
>> --- a/net/mptcp/protocol.c
>> +++ b/net/mptcp/protocol.c
>> @@ -3116,7 +3116,8 @@ bool mptcp_finish_join(struct sock *ssk)
>> 	if (!msk->pm.server_side)
>> 		goto out;
>> 
>> -	if (!mptcp_pm_allow_new_subflow(msk)) {
>> +	if (!mptcp_pm_allow_new_subflow(msk) ||
>> +	    (READ_ONCE(msk->pm.remote_deny_join_id0) && !subflow->remote_id)) 
>> {
>
> This checks whether this side received a C==1 bit from the remote - but 
> that's already checked in mptcp_pm_create_subflow_or_signal_addr().
>
> What might be needed is a check in the opposite direction: if this side 
> *sent* C==1, and the incoming MP_JOIN is for the initial addr/port, that's 
> invalid.
>
> Not sure yet how we are going to interpret the RFC on this, will be 
> discussing at the meeting tomorrow.

I meant "our next scheduled community meeting" :)

>
>
>> 		subflow->reset_reason = MPTCP_RST_EPROHIBIT;
>> 		return false;
>> 	}
>> -- 
>> 2.31.1

--
Mat Martineau
Intel

Re: [MPTCP][PATCH mptcp-next] Squash to "mptcp: add deny_join_id0 in mptcp_options_received"

[Mat Martineau]

On Tue, 18 May 2021, Geliang Tang wrote:

> Please add this line to the commit log:
>
> '''
> In mptcp_finish_join, add the incomming join address check too.
> '''
>
> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> ---
> net/mptcp/protocol.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index c725e8f02533..5cebecc838ca 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -3116,7 +3116,8 @@ bool mptcp_finish_join(struct sock *ssk)
> 	if (!msk->pm.server_side)
> 		goto out;
>
> -	if (!mptcp_pm_allow_new_subflow(msk)) {
> +	if (!mptcp_pm_allow_new_subflow(msk) ||
> +	    (READ_ONCE(msk->pm.remote_deny_join_id0) && !subflow->remote_id)) {

This checks whether this side received a C==1 bit from the remote - but 
that's already checked in mptcp_pm_create_subflow_or_signal_addr().

What might be needed is a check in the opposite direction: if this side 
*sent* C==1, and the incoming MP_JOIN is for the initial addr/port, that's 
invalid.

Not sure yet how we are going to interpret the RFC on this, will be 
discussing at the meeting tomorrow.


> 		subflow->reset_reason = MPTCP_RST_EPROHIBIT;
> 		return false;
> 	}
> -- 
> 2.31.1

--
Mat Martineau
Intel

[MPTCP][PATCH mptcp-next] Squash to "mptcp: add deny_join_id0 in mptcp_options_received"

[Geliang Tang]

Please add this line to the commit log:

'''
In mptcp_finish_join, add the incomming join address check too.
'''

Signed-off-by: Geliang Tang <geliangtang@gmail.com>
---
 net/mptcp/protocol.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index c725e8f02533..5cebecc838ca 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -3116,7 +3116,8 @@ bool mptcp_finish_join(struct sock *ssk)
 	if (!msk->pm.server_side)
 		goto out;
 
-	if (!mptcp_pm_allow_new_subflow(msk)) {
+	if (!mptcp_pm_allow_new_subflow(msk) ||
+	    (READ_ONCE(msk->pm.remote_deny_join_id0) && !subflow->remote_id)) {
 		subflow->reset_reason = MPTCP_RST_EPROHIBIT;
 		return false;
 	}
-- 
2.31.1