[PATCH mptcp-next v11 0/4] userspace pm remove id 0 subflow & address

[PATCH mptcp-next v11 0/4] userspace pm remove id 0 subflow & address

[Geliang Tang]

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

Geliang Tang (4):
  mptcp: set next subflow to msk->first
  selftests: mptcp: userspace pm remove initial subflow
  mptcp: userspace pm remove id 0 address
  selftests: mptcp: userspace pm remove id 0 address

 net/mptcp/pm_userspace.c                      | 36 +++++++++++++++
 net/mptcp/protocol.c                          | 18 ++++++--
 .../testing/selftests/net/mptcp/mptcp_join.sh | 44 +++++++++++++++++++
 3 files changed, 95 insertions(+), 3 deletions(-)

-- 
2.35.3

[PATCH mptcp-next v11 1/4] mptcp: set next subflow to msk->first

[Geliang Tang]

When closing the msk->first socket in __mptcp_close_ssk(), the original
behaviour is to reset this socket. But if there's another subflow available
in this case, it's better to set the first available subflow to msk->first,
instead of resetting the msk->first.

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

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 30e0c29ae0a4..fde65d901f47 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 || msk->pm.subflows;
 	if (dispose_it)
 		list_del(&subflow->node);
 
@@ -2446,8 +2446,20 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
 
 	sock_put(ssk);
 
-	if (ssk == msk->first)
-		WRITE_ONCE(msk->first, NULL);
+	if (ssk == msk->first) {
+		struct mptcp_subflow_context *tmp;
+		struct sock *next = NULL;
+
+		mptcp_for_each_subflow(msk, tmp) {
+			struct sock *s = mptcp_subflow_tcp_sock(tmp);
+
+			if (s != msk->first) {
+				next = s;
+				break;
+			}
+		}
+		WRITE_ONCE(msk->first, next);
+	}
 
 out:
 	__mptcp_sync_sndbuf(sk);
-- 
2.35.3

Re: [PATCH mptcp-next v11 1/4] mptcp: set next subflow to msk->first

[Matthieu Baerts]

Hi Geliang,

+cc Paolo

On 08/10/2023 12:27, Geliang Tang wrote:
> When closing the msk->first socket in __mptcp_close_ssk(), the original
> behaviour is to reset this socket. But if there's another subflow available
> in this case, it's better to set the first available subflow to msk->first,
> instead of resetting the msk->first.

I don't think we want to do that. If I remember well, we want to keep
having 'msk->first' pointing to the initial subflow for various reasons
(I don't remember exactly).

I think what we want to do is not to reset the initial subflow if there
are other subflows still available and we don't want to set it to 'NULL'
in such case.

Could it be possible to do that instead you think?

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

Re: [PATCH mptcp-next v11 1/4] mptcp: set next subflow to msk->first

[Geliang Tang]

On Sun, Oct 08, 2023 at 01:11:32PM +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> +cc Paolo
> 
> On 08/10/2023 12:27, Geliang Tang wrote:
> > When closing the msk->first socket in __mptcp_close_ssk(), the original
> > behaviour is to reset this socket. But if there's another subflow available
> > in this case, it's better to set the first available subflow to msk->first,
> > instead of resetting the msk->first.
> 
> I don't think we want to do that. If I remember well, we want to keep
> having 'msk->first' pointing to the initial subflow for various reasons
> (I don't remember exactly).
> 
> I think what we want to do is not to reset the initial subflow if there
> are other subflows still available and we don't want to set it to 'NULL'
> in such case.

Is it like this:

       if (ssk == msk->first && !msk->pm.subflows)
               WRITE_ONCE(msk->first, NULL);

This doesn't work, it will get a NULL pointer crash:

[  390.852957] MPTCP: msk=0000000089655116 snd_data_fin_enable=1 pending=0 snd_nxt=8683552930344602029 write_seq=8683552930344602029
[  390.852969] BUG: kernel NULL pointer dereference, address: 00000000000000a0
[  390.852973] MPTCP: msk=0000000089655116 state=7 flags=0
[  390.852980] #PF: supervisor read access in kernel mode
[  390.852989] #PF: error_code(0x0000) - not-present page
[  390.852990] MPTCP: msk=0000000089655116 rx queue empty=1:1 copied=0
[  390.852998] PGD 0 P4D 0
[  390.853010] Oops: 0000 [#3] PREEMPT SMP NOPTI
[  390.853020] CPU: 6 PID: 796 Comm: kworker/6:3 Tainted: G      D            6.6.0-rc4-mptcp+ #19
[  390.853030] Hardware name: LENOVO 20UASA0901/20UASA0901, BIOS N2WET38W (1.28 ) 08/30/2022
[  390.853037] Workqueue: events mptcp_worker
[  390.853056] RIP: 0010:mptcp_worker+0x113/0x3e0
[  390.853070] Code: 83 28 fa ff ff a8 01 75 71 f0 48 0f ba 73 d8 02 0f 82 9c 01 00 00 4c 8b a3 80 00 00 00 4d 85 e4 74 18 49 8b 84 24 c0 04 00 00 <48> 8b 80 a0 00 00 00 48 85 c0 0f 85 9f 00 00 00 48 89 ef e8 b5 f2
[  390.853079] RSP: 0018:ffffc90000527e38 EFLAGS: 00010286
[  390.853087] RAX: 0000000000000000 RBX: ffff88815c26e0f8 RCX: 0000000000000000
[  390.853094] RDX: 0000000100016275 RSI: 0000000000000246 RDI: ffff888108c70000
[  390.853100] RBP: ffff88815c26dac0 R08: 0000000000000800 R09: 0000000000000400
[  390.853106] R10: ffff88846d700000 R11: ffffffffb026f448 R12: ffff888106f5a000
[  390.853111] R13: ffff888100099c05 R14: ffff88815c26e100 R15: ffff888103fd36c0
[  390.853117] FS:  0000000000000000(0000) GS:ffff88846d700000(0000) knlGS:0000000000000000
[  390.853125] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  390.853131] CR2: 00000000000000a0 CR3: 00000001a21fc002 CR4: 00000000003706e0
[  390.853138] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  390.853144] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[  390.853149] Call Trace:
[  390.853155]  <TASK>
[  390.853164]  ? __die_body+0x1e/0x60
[  390.853177]  ? page_fault_oops+0x15b/0x470
[  390.853189]  ? pollwake+0x78/0xa0
[  390.853203]  ? __pfx_default_wake_function+0x10/0x10
[  390.853215]  ? exc_page_fault+0x71/0x160
[  390.853226]  ? asm_exc_page_fault+0x26/0x30
[  390.853240]  ? mptcp_worker+0x113/0x3e0
[  390.853253]  ? mptcp_worker+0xc8/0x3e0
[  390.853265]  ? __schedule+0x352/0xc20
[  390.853279]  process_scheduled_works+0x22b/0x360
[  390.853298]  worker_thread+0x147/0x2b0
[  390.853315]  ? __pfx_worker_thread+0x10/0x10
[  390.853331]  kthread+0xe5/0x120
[  390.853346]  ? __pfx_kthread+0x10/0x10
[  390.853362]  ret_from_fork+0x31/0x40
[  390.853372] MPTCP: msk=00000000c3583a71 state=11 flags=0
[  390.853375]  ? __pfx_kthread+0x10/0x10
[  390.853388]  ret_from_fork_asm+0x1b/0x30
[  390.853412]  </TASK>

Now ssk (msk->first) has been released, we must update msk->first.

Thanks,
-Geliang

> 
> Could it be possible to do that instead you think?
> 
> Cheers,
> Matt
> -- 
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net

Re: [PATCH mptcp-next v11 1/4] mptcp: set next subflow to msk->first

[Matthieu Baerts]

Hi Geliang,

On 08/10/2023 14:33, Geliang Tang wrote:
> On Sun, Oct 08, 2023 at 01:11:32PM +0200, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> +cc Paolo
>>
>> On 08/10/2023 12:27, Geliang Tang wrote:
>>> When closing the msk->first socket in __mptcp_close_ssk(), the original
>>> behaviour is to reset this socket. But if there's another subflow available
>>> in this case, it's better to set the first available subflow to msk->first,
>>> instead of resetting the msk->first.
>>
>> I don't think we want to do that. If I remember well, we want to keep
>> having 'msk->first' pointing to the initial subflow for various reasons
>> (I don't remember exactly).
>>
>> I think what we want to do is not to reset the initial subflow if there
>> are other subflows still available and we don't want to set it to 'NULL'
>> in such case.
> 
> Is it like this:
> 
>        if (ssk == msk->first && !msk->pm.subflows)
>                WRITE_ONCE(msk->first, NULL);

No, we cannot set it to NULL nor release it but we should avoid the
"reset" being sent.

I don't have access to the code for the moment but by looking at your
patch, I would say you should not modify 'dispose_it' nor 'msk->first'
but maybe check if you can avoid calling 'mptcp_subflow_ctx_reset()' if
'!dispose_it', maybe by checking something like
'list_is_singular(&msk->conn_list)'? (I think you should avoid looking
at the PM structure here).

Please tell me if it is not clear.

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

Re: [PATCH mptcp-next v11 1/4] mptcp: set next subflow to msk->first

[Geliang Tang]

On Sun, Oct 08, 2023 at 04:05:10PM +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 08/10/2023 14:33, Geliang Tang wrote:
> > On Sun, Oct 08, 2023 at 01:11:32PM +0200, Matthieu Baerts wrote:
> >> Hi Geliang,
> >>
> >> +cc Paolo
> >>
> >> On 08/10/2023 12:27, Geliang Tang wrote:
> >>> When closing the msk->first socket in __mptcp_close_ssk(), the original
> >>> behaviour is to reset this socket. But if there's another subflow available
> >>> in this case, it's better to set the first available subflow to msk->first,
> >>> instead of resetting the msk->first.
> >>
> >> I don't think we want to do that. If I remember well, we want to keep
> >> having 'msk->first' pointing to the initial subflow for various reasons
> >> (I don't remember exactly).
> >>
> >> I think what we want to do is not to reset the initial subflow if there
> >> are other subflows still available and we don't want to set it to 'NULL'
> >> in such case.
> > 
> > Is it like this:
> > 
> >        if (ssk == msk->first && !msk->pm.subflows)
> >                WRITE_ONCE(msk->first, NULL);

The code:

        if (ssk == msk->first && list_is_singular(&msk->conn_list))
                WRITE_ONCE(msk->first, NULL);

works, no NULL pointer crash with this now. I update it into v12.

> 
> No, we cannot set it to NULL nor release it but we should avoid the
> "reset" being sent.
> 
> I don't have access to the code for the moment but by looking at your
> patch, I would say you should not modify 'dispose_it' nor 'msk->first'
> but maybe check if you can avoid calling 'mptcp_subflow_ctx_reset()' if
> '!dispose_it', maybe by checking something like
> 'list_is_singular(&msk->conn_list)'? (I think you should avoid looking
> at the PM structure here).

Something like this, right:

                 * disconnect should never fail
                 */
                WARN_ON_ONCE(tcp_disconnect(ssk, 0));
-               mptcp_subflow_ctx_reset(subflow);
+               list_is_singular(&msk->conn_list)
+                       mptcp_subflow_ctx_reset(subflow);
                release_sock(ssk);
 
                goto out;

This doesn't work, if we only avoid calling mptcp_subflow_ctx_reset,
tcp_disconnect will send a RST too.

Thanks,
-Geliang

> 
> Please tell me if it is not clear.
> 
> Cheers,
> Matt
> -- 
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net

Re: [PATCH mptcp-next v11 1/4] mptcp: set next subflow to msk->first

[Matthieu Baerts]

Hi Geliang,

On 10/10/2023 08:04, Geliang Tang wrote:
> On Sun, Oct 08, 2023 at 04:05:10PM +0200, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 08/10/2023 14:33, Geliang Tang wrote:
>>> On Sun, Oct 08, 2023 at 01:11:32PM +0200, Matthieu Baerts wrote:
>>>> Hi Geliang,
>>>>
>>>> +cc Paolo
>>>>
>>>> On 08/10/2023 12:27, Geliang Tang wrote:
>>>>> When closing the msk->first socket in __mptcp_close_ssk(), the original
>>>>> behaviour is to reset this socket. But if there's another subflow available
>>>>> in this case, it's better to set the first available subflow to msk->first,
>>>>> instead of resetting the msk->first.
>>>>
>>>> I don't think we want to do that. If I remember well, we want to keep
>>>> having 'msk->first' pointing to the initial subflow for various reasons
>>>> (I don't remember exactly).
>>>>
>>>> I think what we want to do is not to reset the initial subflow if there
>>>> are other subflows still available and we don't want to set it to 'NULL'
>>>> in such case.
>>>
>>> Is it like this:
>>>
>>>        if (ssk == msk->first && !msk->pm.subflows)
>>>                WRITE_ONCE(msk->first, NULL);
> 
> The code:
> 
>         if (ssk == msk->first && list_is_singular(&msk->conn_list))
>                 WRITE_ONCE(msk->first, NULL);
> 
> works, no NULL pointer crash with this now. I update it into v12.

If I'm not mistaken, we can only set 'msk->first' back to NULL when
closing the MPTCP connection. We cannot do that if there are other
subflows still alive.

>> No, we cannot set it to NULL nor release it but we should avoid the
>> "reset" being sent.
>>
>> I don't have access to the code for the moment but by looking at your
>> patch, I would say you should not modify 'dispose_it' nor 'msk->first'
>> but maybe check if you can avoid calling 'mptcp_subflow_ctx_reset()' if
>> '!dispose_it', maybe by checking something like
>> 'list_is_singular(&msk->conn_list)'? (I think you should avoid looking
>> at the PM structure here).
> 
> Something like this, right:
> 
>                  * disconnect should never fail
>                  */
>                 WARN_ON_ONCE(tcp_disconnect(ssk, 0));
> -               mptcp_subflow_ctx_reset(subflow);
> +               list_is_singular(&msk->conn_list)
> +                       mptcp_subflow_ctx_reset(subflow);
>                 release_sock(ssk);
>  
>                 goto out;
> 
> This doesn't work, if we only avoid calling mptcp_subflow_ctx_reset,
> tcp_disconnect will send a RST too.

Sorry, yes I meant to say if we should skip both, tcp_disconnect()
included. We should only close the connection but not release it.

(or we need to close, then call disconnect? I didn't check that)

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

[PATCH mptcp-next v11 2/4] 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 | 21 +++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index ae38b428e42e..ce691aeca99e 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -3490,6 +3490,27 @@ 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_mptcp_info subflows 1 subflows 1
+		chk_subflows_total 2 2
+		kill_events_pids
+		wait $tests_pid
+	fi
 }
 
 endpoint_tests()
-- 
2.35.3

Re: [PATCH mptcp-next v11 2/4] selftests: mptcp: userspace pm remove initial subflow

[Matthieu Baerts]

Hi Geliang,

On 08/10/2023 12:27, Geliang Tang wrote:
> 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 | 21 +++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index ae38b428e42e..ce691aeca99e 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -3490,6 +3490,27 @@ 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_mptcp_info subflows 1 subflows 1
> +		chk_subflows_total 2 2

Here, we should have 1 subflow in total, no?

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

Re: [PATCH mptcp-next v11 2/4] selftests: mptcp: userspace pm remove initial subflow

[Matthieu Baerts]

Hi Geliang,

On 08/10/2023 12:27, Geliang Tang wrote:
> 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 | 21 +++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index ae38b428e42e..ce691aeca99e 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -3490,6 +3490,27 @@ 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_mptcp_info subflows 1 subflows 1
> +		chk_subflows_total 2 2

Can you also check that no RST has been sent please?

Cheers,
Matt

> +		kill_events_pids
> +		wait $tests_pid
> +	fi
>  }
>  
>  endpoint_tests()

-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

[PATCH mptcp-next v11 3/4] mptcp: userspace pm remove id 0 address

[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 | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 6b8083650bc1..ed9db6a4e228 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -211,6 +211,37 @@ 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 out;
+	}
+
+	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;
+out:
+	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 +276,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

Re: [PATCH mptcp-next v11 3/4] mptcp: userspace pm remove id 0 address

[Matthieu Baerts]

Hi Geliang,

On 08/10/2023 12:27, Geliang Tang wrote:
> 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.

Thank you for the next version!

> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/379
> Fixes: d9a4594edabf ("mptcp: netlink: Add MPTCP_PM_CMD_REMOVE")

(This should target mptcp-net)

> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
>  net/mptcp/pm_userspace.c | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index 6b8083650bc1..ed9db6a4e228 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -211,6 +211,37 @@ 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 out;

(here it can be 'goto remove_err')

> +	}
> +
> +	list.ids[list.nr++] = 0;

detail: I think it is best to add a new line before spin_lock_bh() and
after spin_unlock_bh(), to clearly show the block with the "dangerous"
section with lock/unlock.

> +	spin_lock_bh(&msk->pm.lock);
> +	mptcp_pm_remove_addr(msk, &list);
> +	spin_unlock_bh(&msk->pm.lock);
> +	err = 0;

(and a new line here too)

> +out:
> +	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 +276,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;

Here it is best to use 'goto out' I think because we expect not to have
errors most of the time, no?

> +	}
> +
>  	lock_sock(sk);
>  
>  	list_for_each_entry(entry, &msk->pm.userspace_pm_local_addr_list, list) {

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

[PATCH mptcp-next v11 4/4] selftests: mptcp: userspace pm remove id 0 address

[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 | 23 +++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index ce691aeca99e..30b9e367cefa 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -3511,6 +3511,29 @@ userspace_tests()
 		kill_events_pids
 		wait $tests_pid
 	fi
+
+	# userspace pm remove id 0 address
+	if reset_with_events "userspace pm remove id 0 address" &&
+	   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
+		chk_rm_nr 1 0 invert
+		chk_mptcp_info subflows 1 subflows 1
+		chk_subflows_total 2 2
+		kill_events_pids
+		wait $tests_pid
+	fi
 }
 
 endpoint_tests()
-- 
2.35.3

Re: [PATCH mptcp-next v11 4/4] selftests: mptcp: userspace pm remove id 0 address

[Matthieu Baerts]

Hi Geliang,

On 08/10/2023 12:27, Geliang Tang wrote:
> 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 | 23 +++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index ce691aeca99e..30b9e367cefa 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -3511,6 +3511,29 @@ userspace_tests()
>  		kill_events_pids
>  		wait $tests_pid
>  	fi
> +
> +	# userspace pm remove id 0 address
> +	if reset_with_events "userspace pm remove id 0 address" &&

Maybe clearer to say: "userspace pm send RM_ADDR for ID 0", no?

> +	   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
> +		chk_rm_nr 1 0 invert
> +		chk_mptcp_info subflows 1 subflows 1
> +		chk_subflows_total 2 2

(Is it really 2? I guess this is due to your patch 1/4 and we should
have 2 subflows in total here, no?)

After the server has sent a RM_ADDR for ID0, the client with the Netlink
PM will react by closing the subflow, no? If yes, please add a comment
explaining why the subflow has been removed after having sent a RM_ADDR
because it can be confusing.

Also, can you check that no RST has been sent?

Cheers,
Matt

> +		kill_events_pids
> +		wait $tests_pid
> +	fi
>  }
>  
>  endpoint_tests()

-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: selftests: mptcp: userspace pm remove id 0 address: 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):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/4593464562679808
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4593464562679808/summary/summary.txt

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

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

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

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


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)