[PATCH mptcp-next v12 0/5] userspace pm remove id 0 subflow & address

[PATCH mptcp-next v12 0/5] userspace pm remove id 0 subflow & address

[Geliang Tang]

v12:
 - address Matt's comments in v11.

v11:
 - avoid sending RSTs.
 - rename 'id 0 subflow' to 'inital subflow'.

Geliang Tang (5):
  mptcp: avoid resetting when another subflow available
  selftests: mptcp: userspace pm remove initial subflow
  mptcp: userspace pm send RM_ADDR for ID 0
  mptcp: userspace pm rename remove_err to out
  selftests: mptcp: userspace pm send RM_ADDR for ID 0

 net/mptcp/pm_userspace.c                      | 45 ++++++++++++++++--
 net/mptcp/protocol.c                          |  4 +-
 .../testing/selftests/net/mptcp/mptcp_join.sh | 47 +++++++++++++++++++
 3 files changed, 91 insertions(+), 5 deletions(-)

-- 
2.35.3

[PATCH mptcp-next v12 1/5] mptcp: avoid resetting when another subflow available

[Geliang Tang]

When closing the msk->first socket in __mptcp_close_ssk(), if there's
another subflow available, it's better to avoid resetting it.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/protocol.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 30e0c29ae0a4..6346a164ed66 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2396,7 +2396,7 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
 		goto out_release;
 	}
 
-	dispose_it = msk->free_first || ssk != msk->first;
+	dispose_it = msk->free_first || ssk != msk->first || !list_is_singular(&msk->conn_list);
 	if (dispose_it)
 		list_del(&subflow->node);
 
@@ -2446,7 +2446,7 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
 
 	sock_put(ssk);
 
-	if (ssk == msk->first)
+	if (ssk == msk->first && list_is_singular(&msk->conn_list))
 		WRITE_ONCE(msk->first, NULL);
 
 out:
-- 
2.35.3

Re: [PATCH mptcp-next v12 1/5] mptcp: avoid resetting when another subflow available

[Matthieu Baerts]

Hi Geliang,

On 09/10/2023 08:02, Geliang Tang wrote:
> When closing the msk->first socket in __mptcp_close_ssk(), if there's
> another subflow available, it's better to avoid resetting it.
> 
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
>  net/mptcp/protocol.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 30e0c29ae0a4..6346a164ed66 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -2396,7 +2396,7 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
>  		goto out_release;
>  	}
>  
> -	dispose_it = msk->free_first || ssk != msk->first;
> +	dispose_it = msk->free_first || ssk != msk->first || !list_is_singular(&msk->conn_list);
>  	if (dispose_it)
>  		list_del(&subflow->node);
>  
> @@ -2446,7 +2446,7 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
>  
>  	sock_put(ssk);
>  
> -	if (ssk == msk->first)
> +	if (ssk == msk->first && list_is_singular(&msk->conn_list))
>  		WRITE_ONCE(msk->first, NULL);

I don't think we should 'dispose it' nor set 'msk->first' to NULL but
instead, we should not call tcp_disconnect() directly: if the connection
is still in ESTABLISHED, tcp_disconnect() will send a RST if I'm not
mistaken. That's what we want to avoid, no?

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH mptcp-next v12 1/5] mptcp: avoid resetting when another subflow available

[Geliang Tang]

On Mon, Oct 09, 2023 at 06:13:19PM +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 09/10/2023 08:02, Geliang Tang wrote:
> > When closing the msk->first socket in __mptcp_close_ssk(), if there's
> > another subflow available, it's better to avoid resetting it.
> > 
> > Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> > ---
> >  net/mptcp/protocol.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > index 30e0c29ae0a4..6346a164ed66 100644
> > --- a/net/mptcp/protocol.c
> > +++ b/net/mptcp/protocol.c
> > @@ -2396,7 +2396,7 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
> >  		goto out_release;
> >  	}
> >  
> > -	dispose_it = msk->free_first || ssk != msk->first;
> > +	dispose_it = msk->free_first || ssk != msk->first || !list_is_singular(&msk->conn_list);
> >  	if (dispose_it)
> >  		list_del(&subflow->node);
> >  
> > @@ -2446,7 +2446,7 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
> >  
> >  	sock_put(ssk);
> >  
> > -	if (ssk == msk->first)
> > +	if (ssk == msk->first && list_is_singular(&msk->conn_list))
> >  		WRITE_ONCE(msk->first, NULL);
> 
> I don't think we should 'dispose it' nor set 'msk->first' to NULL but

That's what this patch dose. Dispose it, not set msk->first to NULL.

> instead, we should not call tcp_disconnect() directly: if the connection

But this patch lacks the code to handle:

	ssk == msk->first && !list_is_singular(&msk->conn_list)

Is my understanding correct?

How about dropping list_is_singular() in the next block, something like:

         * For established sockets, allow an additional timeout before closing,
         * as the protocol can still create more subflows.
         */
-       if (list_is_singular(&msk->conn_list) && msk->first &&
-           inet_sk_state_load(msk->first) == TCP_CLOSE) {
+       if (msk->first && inet_sk_state_load(msk->first) == TCP_CLOSE) {
                if (sk->sk_state != TCP_ESTABLISHED ||
                    msk->in_accept_queue || sock_flag(sk, SOCK_DEAD)) {
                        inet_sk_state_store(sk, TCP_CLOSE);

> is still in ESTABLISHED, tcp_disconnect() will send a RST if I'm not
> mistaken. That's what we want to avoid, no?

Yes, tcp_disconnect will send a RST.

Thanks,
-Geliang

> 
> Cheers,
> Matt
> -- 
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net

Re: [PATCH mptcp-next v12 1/5] mptcp: avoid resetting when another subflow available

[Matthieu Baerts]

Hi Geliang,

On 10/10/2023 08:13, Geliang Tang wrote:
> On Mon, Oct 09, 2023 at 06:13:19PM +0200, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 09/10/2023 08:02, Geliang Tang wrote:
>>> When closing the msk->first socket in __mptcp_close_ssk(), if there's
>>> another subflow available, it's better to avoid resetting it.
>>>
>>> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
>>> ---
>>>  net/mptcp/protocol.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>>> index 30e0c29ae0a4..6346a164ed66 100644
>>> --- a/net/mptcp/protocol.c
>>> +++ b/net/mptcp/protocol.c
>>> @@ -2396,7 +2396,7 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
>>>  		goto out_release;
>>>  	}
>>>  
>>> -	dispose_it = msk->free_first || ssk != msk->first;
>>> +	dispose_it = msk->free_first || ssk != msk->first || !list_is_singular(&msk->conn_list);
>>>  	if (dispose_it)
>>>  		list_del(&subflow->node);
>>>  
>>> @@ -2446,7 +2446,7 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
>>>  
>>>  	sock_put(ssk);
>>>  
>>> -	if (ssk == msk->first)
>>> +	if (ssk == msk->first && list_is_singular(&msk->conn_list))
>>>  		WRITE_ONCE(msk->first, NULL);
>>
>> I don't think we should 'dispose it' nor set 'msk->first' to NULL but
> 
> That's what this patch dose. Dispose it, not set msk->first to NULL.

Sorry, I meant: we should not 'dispose it' and we should not set
'msk->first' to NULL if we are not closing the whole MPTCP connection.

>> instead, we should not call tcp_disconnect() directly: if the connection
> 
> But this patch lacks the code to handle:
> 
> 	ssk == msk->first && !list_is_singular(&msk->conn_list)
> 
> Is my understanding correct?
> 
> How about dropping list_is_singular() in the next block, something like:
> 
>          * For established sockets, allow an additional timeout before closing,
>          * as the protocol can still create more subflows.
>          */
> -       if (list_is_singular(&msk->conn_list) && msk->first &&
> -           inet_sk_state_load(msk->first) == TCP_CLOSE) {
> +       if (msk->first && inet_sk_state_load(msk->first) == TCP_CLOSE) {
>                 if (sk->sk_state != TCP_ESTABLISHED ||
>                     msk->in_accept_queue || sock_flag(sk, SOCK_DEAD)) {
>                         inet_sk_state_store(sk, TCP_CLOSE);

I cannot tell like that, but what is important first is not to call
'tcp_disconnect()' on a non already closed socket. So we need something
else before the 'disconnect()'.

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH mptcp-next v12 1/5] mptcp: avoid resetting when another subflow available

[Paolo Abeni]

On Mon, 2023-10-09 at 14:02 +0800, Geliang Tang wrote:
> When closing the msk->first socket in __mptcp_close_ssk(), if there's
> another subflow available, it's better to avoid resetting it.
> 
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
>  net/mptcp/protocol.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 30e0c29ae0a4..6346a164ed66 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -2396,7 +2396,7 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
>  		goto out_release;
>  	}
>  
> -	dispose_it = msk->free_first || ssk != msk->first;
> +	dispose_it = msk->free_first || ssk != msk->first || !list_is_singular(&msk->conn_list);
>  	if (dispose_it)
>  		list_del(&subflow->node);

I'm sorry for the late feedback.

We can't do the above, Syzkaller already hit us in the past while
attempting that thing.

We need to be msk->first != NULL and a valid, derefereciable pointer up
to mptcp_destroy() - that is, up to when the msk socket is freed -
otherwise we will hit a number of UaF in many places.

Note that claring msk->first here, and add check for 'msk->first !=
NULL' before every msk access, will not save us, sometimes msk->first
is tested without the msk socket lock. 

I think there are 2 options here:

- the caller (PM NL/PM userspace) could explicitly avoid calling
mptcp_close_ssk() when removing subflow 0 (just shut it down)

- Add a new flags MPTCP_NO_DISCONNECT, and let the caller pass it here.
  When MPTCP_NO_DISCONNECT is set, we invoke tcp_shutdown() instead of
tcp_disconnect()

Both options are actually quite similar, the 2nd is possibly the
cleanest.

Note that looking here for 'this is the last subflow' and avoid the
disconnect otherwise, does not look correct/safe: we want to invoke
tcp_disconnect() even when first is _not_ the last subflow and we are
reaching mptcp_close_ssk() from a different caller - e.g. from
mptcp_disconnect()

/P

[PATCH mptcp-next v12 2/5] selftests: mptcp: userspace pm remove initial subflow

[Geliang Tang]

This patch adds a selftest for userpsace PM to remove the initial
subflow. Use userspace_pm_add_sf() to add a subflow, and pass initial
ip address to userspace_pm_rm_sf() to remove the initial subflow.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 .../testing/selftests/net/mptcp/mptcp_join.sh | 22 +++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index ae38b428e42e..d396b290e20b 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -3490,6 +3490,28 @@ userspace_tests()
 		kill_events_pids
 		wait $tests_pid
 	fi
+
+	# userspace pm remove initial subflow
+	if reset_with_events "userspace pm remove initial subflow" &&
+	   continue_if mptcp_lib_has_file '/proc/sys/net/mptcp/pm_type'; then
+		set_userspace_pm $ns2
+		pm_nl_set_limits $ns1 0 1
+		speed=10 \
+			run_tests $ns1 $ns2 10.0.1.1 &
+		local tests_pid=$!
+		wait_mpj $ns2
+		userspace_pm_add_sf $ns2 10.0.3.2 20
+		chk_join_nr 1 1 1
+		chk_mptcp_info subflows 1 subflows 1
+		chk_subflows_total 2 2
+		userspace_pm_rm_sf $ns2 10.0.1.2
+		chk_rm_nr 0 1
+		chk_rst_nr 0 0 invert
+		chk_mptcp_info subflows 1 subflows 1
+		chk_subflows_total 1 1
+		kill_events_pids
+		wait $tests_pid
+	fi
 }
 
 endpoint_tests()
-- 
2.35.3

[PATCH mptcp-net v12 3/5] mptcp: userspace pm send RM_ADDR for ID 0

[Geliang Tang]

This patch adds the ability to send RM_ADDR for local ID 0. Check
whether id 0 address is removed, if not, put id 0 into a removing
list, pass it to mptcp_pm_remove_addr() to remove id 0 address.

There is no reason not to allow the userspace to remove the initial
address (ID 0). This special case was not taken into account not
letting the userspace to delete all addresses as announced.

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/379
Fixes: d9a4594edabf ("mptcp: netlink: Add MPTCP_PM_CMD_REMOVE")
Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/pm_userspace.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 6b8083650bc1..ea50e694125d 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -211,6 +211,40 @@ int mptcp_pm_nl_announce_doit(struct sk_buff *skb, struct genl_info *info)
 	return err;
 }
 
+static int mptcp_userspace_remove_id_zero_address(struct mptcp_sock *msk,
+						  struct genl_info *info)
+{
+	struct mptcp_rm_list list = { .nr = 0 };
+	struct mptcp_subflow_context *subflow;
+	struct sock *sk = (struct sock *)msk;
+	bool has_id_0 = false;
+	int err = -EINVAL;
+
+	lock_sock(sk);
+	mptcp_for_each_subflow(msk, subflow) {
+		if (subflow->local_id == 0) {
+			has_id_0 = true;
+			break;
+		}
+	}
+	if (!has_id_0) {
+		GENL_SET_ERR_MSG(info, "address with id 0 not found");
+		goto remove_err;
+	}
+
+	list.ids[list.nr++] = 0;
+
+	spin_lock_bh(&msk->pm.lock);
+	mptcp_pm_remove_addr(msk, &list);
+	spin_unlock_bh(&msk->pm.lock);
+
+	err = 0;
+
+remove_err:
+	release_sock(sk);
+	return err;
+}
+
 int mptcp_pm_nl_remove_doit(struct sk_buff *skb, struct genl_info *info)
 {
 	struct nlattr *token = info->attrs[MPTCP_PM_ATTR_TOKEN];
@@ -245,6 +279,11 @@ int mptcp_pm_nl_remove_doit(struct sk_buff *skb, struct genl_info *info)
 		goto remove_err;
 	}
 
+	if (id_val == 0) {
+		err = mptcp_userspace_remove_id_zero_address(msk, info);
+		goto remove_err;
+	}
+
 	lock_sock(sk);
 
 	list_for_each_entry(entry, &msk->pm.userspace_pm_local_addr_list, list) {
-- 
2.35.3

[PATCH mptcp-next v12 4/5] mptcp: userspace pm rename remove_err to out

[Geliang Tang]

The value of 'err' will be not only '-EINVAL', but alse '0' most of the
time. So it's better to rename the lable 'remove_err' to 'out'.

Suggested-by: Matthieu Baerts <matttbe@kernel.org>
Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/pm_userspace.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index ea50e694125d..cdff3e631d2d 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -276,12 +276,12 @@ int mptcp_pm_nl_remove_doit(struct sk_buff *skb, struct genl_info *info)
 
 	if (!mptcp_pm_is_userspace(msk)) {
 		GENL_SET_ERR_MSG(info, "invalid request; userspace PM not selected");
-		goto remove_err;
+		goto out;
 	}
 
 	if (id_val == 0) {
 		err = mptcp_userspace_remove_id_zero_address(msk, info);
-		goto remove_err;
+		goto out;
 	}
 
 	lock_sock(sk);
@@ -296,7 +296,7 @@ int mptcp_pm_nl_remove_doit(struct sk_buff *skb, struct genl_info *info)
 	if (!match) {
 		GENL_SET_ERR_MSG(info, "address with specified id not found");
 		release_sock(sk);
-		goto remove_err;
+		goto out;
 	}
 
 	list_move(&match->list, &free_list);
@@ -310,7 +310,7 @@ int mptcp_pm_nl_remove_doit(struct sk_buff *skb, struct genl_info *info)
 	}
 
 	err = 0;
- remove_err:
+out:
 	sock_put(sk);
 	return err;
 }
-- 
2.35.3

[PATCH mptcp-next v12 5/5] selftests: mptcp: userspace pm send RM_ADDR for ID 0

[Geliang Tang]

This patch adds a selftest for userpsace PM to remove id 0 address.
Use userspace_pm_add_addr() helper to add a id 10 address, then use
userspace_pm_rm_addr() helper to remove id 0 address.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 .../testing/selftests/net/mptcp/mptcp_join.sh | 25 +++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index d396b290e20b..55aca4e7f051 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -3512,6 +3512,31 @@ userspace_tests()
 		kill_events_pids
 		wait $tests_pid
 	fi
+
+	# userspace pm send RM_ADDR for ID 0
+	if reset_with_events "userspace pm send RM_ADDR for ID 0" &&
+	   continue_if mptcp_lib_has_file '/proc/sys/net/mptcp/pm_type'; then
+		set_userspace_pm $ns1
+		pm_nl_set_limits $ns2 1 1
+		speed=10 \
+			run_tests $ns1 $ns2 10.0.1.1 &
+		local tests_pid=$!
+		wait_mpj $ns1
+		userspace_pm_add_addr $ns1 10.0.2.1 10
+		chk_join_nr 1 1 1
+		chk_add_nr 1 1
+		chk_mptcp_info subflows 1 subflows 1
+		chk_subflows_total 2 2
+		chk_mptcp_info add_addr_signal 1 add_addr_accepted 1
+		userspace_pm_rm_addr $ns1 0
+		sleep 0.5
+		chk_rm_nr 1 0 invert
+		chk_rst_nr 0 0 invert
+		chk_mptcp_info subflows 1 subflows 1
+		chk_subflows_total 1 1
+		kill_events_pids
+		wait $tests_pid
+	fi
 }
 
 endpoint_tests()
-- 
2.35.3

Re: selftests: mptcp: userspace pm send RM_ADDR for ID 0: Tests Results

[MPTCP CI]

Hi Geliang,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal (except selftest_mptcp_join):
  - Unstable: 1 failed test(s): packetdrill_regressions 🔴:
  - Task: https://cirrus-ci.com/task/5543789025034240
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5543789025034240/summary/summary.txt

- KVM Validation: normal (only selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/6669688931876864
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6669688931876864/summary/summary.txt

- KVM Validation: debug (except selftest_mptcp_join):
  - Unstable: 1 failed test(s): packetdrill_regressions - Critical: 1 Call Trace(s) ❌:
  - Task: https://cirrus-ci.com/task/4628995350724608
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4628995350724608/summary/summary.txt

- KVM Validation: debug (only selftest_mptcp_join):
  - Unstable: 1 failed test(s): selftest_mptcp_join - Critical: 1 Call Trace(s) ❌:
  - Task: https://cirrus-ci.com/task/5754895257567232
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5754895257567232/summary/summary.txt

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/5f8f6660beb8


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-debug

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (Tessares)