[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]

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

[MPTCP][PATCH mptcp-next] Squash to "selftests: mptcp: add deny_join_id0 testcases"

[Geliang Tang]

Update the testcases.

Signed-off-by: Geliang Tang <geliangtang@gmail.com>
---
 .../testing/selftests/net/mptcp/mptcp_join.sh | 35 ++++++++++++++-----
 1 file changed, 26 insertions(+), 9 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 17b385f011d2..fe46d666b18c 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -1491,24 +1491,41 @@ deny_join_id0_tests()
 	run_tests $ns1 $ns2 10.0.1.1
 	chk_join_nr "single subflow allow join id0 ns2" 0 0 0
 
-	# subflow and address allow join id0
+	# signal address allow join id0 ns1
+	reset_with_allow_join_id0 1 0
+	ip netns exec $ns1 ./pm_nl_ctl limits 1 1
+	ip netns exec $ns2 ./pm_nl_ctl limits 1 1
+	ip netns exec $ns1 ./pm_nl_ctl add 10.0.2.1 flags signal
+	run_tests $ns1 $ns2 10.0.1.1
+	chk_join_nr "signal address allow join id0 ns1" 1 1 0
+	chk_add_nr 1 1
+
+	# signal address allow join id0 ns2
+	reset_with_allow_join_id0 0 1
+	ip netns exec $ns1 ./pm_nl_ctl limits 1 1
+	ip netns exec $ns2 ./pm_nl_ctl limits 1 1
+	ip netns exec $ns1 ./pm_nl_ctl add 10.0.2.1 flags signal
+	run_tests $ns1 $ns2 10.0.1.1
+	chk_join_nr "signal address allow join id0 ns2" 1 1 1
+	chk_add_nr 1 1
+
+	# subflow and address allow join id0 ns1
 	reset_with_allow_join_id0 0 1
 	ip netns exec $ns1 ./pm_nl_ctl limits 2 2
 	ip netns exec $ns2 ./pm_nl_ctl limits 2 2
 	ip netns exec $ns1 ./pm_nl_ctl add 10.0.2.1 flags signal
 	ip netns exec $ns2 ./pm_nl_ctl add 10.0.3.2 flags subflow
 	run_tests $ns1 $ns2 10.0.1.1
-	chk_join_nr "subflow and address allow join id0" 2 2 2
+	chk_join_nr "subflow and address allow join id0 1" 2 2 2
 
-	# signal address allow join id0
-	# ADD_ADDRs are not affected by allow_join_id0 value.
-	reset_with_allow_join_id0 0 0
-	ip netns exec $ns1 ./pm_nl_ctl limits 1 1
-	ip netns exec $ns2 ./pm_nl_ctl limits 1 1
+	# subflow and address allow join id0 ns2
+	reset_with_allow_join_id0 1 0
+	ip netns exec $ns1 ./pm_nl_ctl limits 2 2
+	ip netns exec $ns2 ./pm_nl_ctl limits 2 2
 	ip netns exec $ns1 ./pm_nl_ctl add 10.0.2.1 flags signal
+	ip netns exec $ns2 ./pm_nl_ctl add 10.0.3.2 flags subflow
 	run_tests $ns1 $ns2 10.0.1.1
-	chk_join_nr "signal address allow join id0" 1 1 1
-	chk_add_nr 1 1
+	chk_join_nr "subflow and address allow join id0 2" 2 2 1
 }
 
 all_tests()
-- 
2.31.1

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

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 "selftests: mptcp: add deny_join_id0 testcases"

[Mat Martineau]

On Tue, 18 May 2021, Geliang Tang wrote:

> Update the testcases.
>

Since there are now other changes to v6, please include the selftest 
updates in v7 of the 'c' bit patchset. Thanks.

Mat


> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> ---
> .../testing/selftests/net/mptcp/mptcp_join.sh | 35 ++++++++++++++-----
> 1 file changed, 26 insertions(+), 9 deletions(-)
>
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 17b385f011d2..fe46d666b18c 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -1491,24 +1491,41 @@ deny_join_id0_tests()
> 	run_tests $ns1 $ns2 10.0.1.1
> 	chk_join_nr "single subflow allow join id0 ns2" 0 0 0
>
> -	# subflow and address allow join id0
> +	# signal address allow join id0 ns1
> +	reset_with_allow_join_id0 1 0
> +	ip netns exec $ns1 ./pm_nl_ctl limits 1 1
> +	ip netns exec $ns2 ./pm_nl_ctl limits 1 1
> +	ip netns exec $ns1 ./pm_nl_ctl add 10.0.2.1 flags signal
> +	run_tests $ns1 $ns2 10.0.1.1
> +	chk_join_nr "signal address allow join id0 ns1" 1 1 0
> +	chk_add_nr 1 1
> +
> +	# signal address allow join id0 ns2
> +	reset_with_allow_join_id0 0 1
> +	ip netns exec $ns1 ./pm_nl_ctl limits 1 1
> +	ip netns exec $ns2 ./pm_nl_ctl limits 1 1
> +	ip netns exec $ns1 ./pm_nl_ctl add 10.0.2.1 flags signal
> +	run_tests $ns1 $ns2 10.0.1.1
> +	chk_join_nr "signal address allow join id0 ns2" 1 1 1
> +	chk_add_nr 1 1
> +
> +	# subflow and address allow join id0 ns1
> 	reset_with_allow_join_id0 0 1
> 	ip netns exec $ns1 ./pm_nl_ctl limits 2 2
> 	ip netns exec $ns2 ./pm_nl_ctl limits 2 2
> 	ip netns exec $ns1 ./pm_nl_ctl add 10.0.2.1 flags signal
> 	ip netns exec $ns2 ./pm_nl_ctl add 10.0.3.2 flags subflow
> 	run_tests $ns1 $ns2 10.0.1.1
> -	chk_join_nr "subflow and address allow join id0" 2 2 2
> +	chk_join_nr "subflow and address allow join id0 1" 2 2 2
>
> -	# signal address allow join id0
> -	# ADD_ADDRs are not affected by allow_join_id0 value.
> -	reset_with_allow_join_id0 0 0
> -	ip netns exec $ns1 ./pm_nl_ctl limits 1 1
> -	ip netns exec $ns2 ./pm_nl_ctl limits 1 1
> +	# subflow and address allow join id0 ns2
> +	reset_with_allow_join_id0 1 0
> +	ip netns exec $ns1 ./pm_nl_ctl limits 2 2
> +	ip netns exec $ns2 ./pm_nl_ctl limits 2 2
> 	ip netns exec $ns1 ./pm_nl_ctl add 10.0.2.1 flags signal
> +	ip netns exec $ns2 ./pm_nl_ctl add 10.0.3.2 flags subflow
> 	run_tests $ns1 $ns2 10.0.1.1
> -	chk_join_nr "signal address allow join id0" 1 1 1
> -	chk_add_nr 1 1
> +	chk_join_nr "subflow and address allow join id0 2" 2 2 1
> }
>
> all_tests()
>

--
Mat Martineau
Intel

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

[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