[PATCH mptcp-next v5 3/7] mptcp: close remote subflow when destroying it

Geliang Tang posted 7 patches 1 year, 2 months ago
Maintainers: Matthieu Baerts <matthieu.baerts@tessares.net>, "David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, Shuah Khan <shuah@kernel.org>
There is a newer version of this series
[PATCH mptcp-next v5 3/7] mptcp: close remote subflow when destroying it
Posted by Geliang Tang 1 year, 2 months ago
Not only close the local subflow but also send RM_ADDR by invoking
mptcp_pm_remove_addr() to close the remote subflow when a subflow is
destroyed by userspace PM.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/pm_userspace.c                        | 16 ++++++++++++++++
 tools/testing/selftests/net/mptcp/mptcp_join.sh |  2 +-
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 49f41a040485..8b077564e394 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -429,6 +429,22 @@ int mptcp_nl_cmd_sf_destroy(struct sk_buff *skb, struct genl_info *info)
 	ssk = mptcp_nl_find_ssk(msk, &addr_l, &addr_r);
 	if (ssk) {
 		struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
+		struct mptcp_pm_addr_entry *entry, *tmp;
+
+		spin_lock_bh(&msk->pm.lock);
+		list_for_each_entry_safe(entry, tmp, &msk->pm.userspace_pm_local_addr_list, list) {
+			if (mptcp_addresses_equal(&entry->addr, &addr_l, false)) {
+				struct mptcp_rm_list list = { .nr = 0 };
+
+				list.ids[list.nr++] = entry->addr.id;
+				mptcp_pm_remove_addr(msk, &list);
+				list_del_rcu(&entry->list);
+				kfree(entry);
+				msk->pm.local_addr_used--;
+				break;
+			}
+		}
+		spin_unlock_bh(&msk->pm.lock);
 
 		mptcp_subflow_shutdown(sk, ssk, RCV_SHUTDOWN | SEND_SHUTDOWN);
 		mptcp_close_ssk(sk, ssk, subflow);
diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index fafd19ec7e1f..506120401abe 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -3123,7 +3123,7 @@ userspace_tests()
 		pm_nl_set_limits $ns1 0 1
 		run_tests $ns1 $ns2 10.0.1.1 0 0 userspace_1 slow
 		chk_join_nr 1 1 1
-		chk_rm_nr 0 1
+		chk_rm_nr 1 1
 		kill_events_pids
 	fi
 }
-- 
2.35.3
Re: [PATCH mptcp-next v5 3/7] mptcp: close remote subflow when destroying it
Posted by Matthieu Baerts 1 year, 2 months ago
Hi Geliang,

On 14/03/2023 08:31, Geliang Tang wrote:
> Not only close the local subflow but also send RM_ADDR by invoking
> mptcp_pm_remove_addr() to close the remote subflow when a subflow is
> destroyed by userspace PM.

Should it not be the responsibility of the userspace PM daemon to send
this RM_ADDR?

Do you maybe have a use case where it is interesting to force sending
this RM_ADDR when asking for a destroy?

Maybe we could do that only if the userspace PM asks for that by setting
a new attribute or flag? (if there is an interest)

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net
Re: [PATCH mptcp-next v5 3/7] mptcp: close remote subflow when destroying it
Posted by Geliang Tang 1 year, 2 months ago
Hi Matt,

Thanks for your reviews.

We always remove address together with subflow, see
mptcp_nl_remove_subflow_and_signal_addr() or
mptcp_pm_remove_addrs_and_subflows().

So we always get "chk_rm_nr 1 1" in these tests:

        # single subflow, remove
        if reset "remove single subflow"; then
                pm_nl_set_limits $ns1 0 1
                pm_nl_set_limits $ns2 0 1
                pm_nl_add_endpoint $ns2 10.0.3.2 flags subflow
                run_tests $ns1 $ns2 10.0.1.1 0 0 -1 slow
                chk_join_nr 1 1 1
                chk_rm_nr 1 1
        fi

        # single address, remove
        if reset "remove single address"; then
                pm_nl_set_limits $ns1 0 1
                pm_nl_add_endpoint $ns1 10.0.2.1 flags signal
                pm_nl_set_limits $ns2 1 1
                run_tests $ns1 $ns2 10.0.1.1 0 -1 0 slow
                chk_join_nr 1 1 1
                chk_add_nr 1 1
                chk_rm_nr 1 1 invert
        fi

        # userspace pm add & remove address
        if reset_with_events "userspace pm add & remove address"; then
                set_userspace_pm $ns1
                pm_nl_set_limits $ns2 1 1
                run_tests $ns1 $ns2 10.0.1.1 0 userspace_1 0 slow
                chk_join_nr 1 1 1
                chk_add_nr 1 1
                chk_rm_nr 1 1 invert
                kill_events_pids
        fi

But the userspace remove subflow test got "chk_rm_nr 0 1":

        # userspace pm create destroy subflow
        if reset_with_events "userspace pm create destroy subflow"; then
                set_userspace_pm $ns2
                pm_nl_set_limits $ns1 0 1
                run_tests $ns1 $ns2 10.0.1.1 0 0 userspace_1 slow
                chk_join_nr 1 1 1
                chk_rm_nr 0 1
                kill_events_pids
        fi

"chk_rm_nr 0 1" means getting no rm_addr, but a rm_subflow.

This behavior is incorrect. This patch fix it. It looks through the
userspace_pm_local_addr_list to find the addr id, and send RM_ADDR
with this id.

So patch 2 is needed to add the address into
userspace_pm_local_addr_list, and patch 1 too, no clear the address
id.

Thanks,
-Geliang

Matthieu Baerts <matthieu.baerts@tessares.net> 于2023年3月17日周五 02:26写道:
>
> Hi Geliang,
>
> On 14/03/2023 08:31, Geliang Tang wrote:
> > Not only close the local subflow but also send RM_ADDR by invoking
> > mptcp_pm_remove_addr() to close the remote subflow when a subflow is
> > destroyed by userspace PM.
>
> Should it not be the responsibility of the userspace PM daemon to send
> this RM_ADDR?
>
> Do you maybe have a use case where it is interesting to force sending
> this RM_ADDR when asking for a destroy?
>
> Maybe we could do that only if the userspace PM asks for that by setting
> a new attribute or flag? (if there is an interest)
>
> Cheers,
> Matt
> --
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net
>
Re: [PATCH mptcp-next v5 3/7] mptcp: close remote subflow when destroying it
Posted by Matthieu Baerts 1 year, 2 months ago
Hi Geliang,

Thank you for your reply.

On 20/03/2023 12:41, Geliang Tang wrote:
> Hi Matt,
> 
> Thanks for your reviews.
> 
> We always remove address together with subflow, see
> mptcp_nl_remove_subflow_and_signal_addr() or
> mptcp_pm_remove_addrs_and_subflows().
> 
> So we always get "chk_rm_nr 1 1" in these tests:
> 
>         # single subflow, remove
>         if reset "remove single subflow"; then
>                 pm_nl_set_limits $ns1 0 1
>                 pm_nl_set_limits $ns2 0 1
>                 pm_nl_add_endpoint $ns2 10.0.3.2 flags subflow
>                 run_tests $ns1 $ns2 10.0.1.1 0 0 -1 slow
>                 chk_join_nr 1 1 1
>                 chk_rm_nr 1 1
>         fi
> 
>         # single address, remove
>         if reset "remove single address"; then
>                 pm_nl_set_limits $ns1 0 1
>                 pm_nl_add_endpoint $ns1 10.0.2.1 flags signal
>                 pm_nl_set_limits $ns2 1 1
>                 run_tests $ns1 $ns2 10.0.1.1 0 -1 0 slow
>                 chk_join_nr 1 1 1
>                 chk_add_nr 1 1
>                 chk_rm_nr 1 1 invert
>         fi
> 
>         # userspace pm add & remove address
>         if reset_with_events "userspace pm add & remove address"; then
>                 set_userspace_pm $ns1
>                 pm_nl_set_limits $ns2 1 1
>                 run_tests $ns1 $ns2 10.0.1.1 0 userspace_1 0 slow
>                 chk_join_nr 1 1 1
>                 chk_add_nr 1 1
>                 chk_rm_nr 1 1 invert
>                 kill_events_pids
>         fi
> 
> But the userspace remove subflow test got "chk_rm_nr 0 1":
> 
>         # userspace pm create destroy subflow
>         if reset_with_events "userspace pm create destroy subflow"; then
>                 set_userspace_pm $ns2
>                 pm_nl_set_limits $ns1 0 1
>                 run_tests $ns1 $ns2 10.0.1.1 0 0 userspace_1 slow
>                 chk_join_nr 1 1 1
>                 chk_rm_nr 0 1
>                 kill_events_pids
>         fi
> 
> "chk_rm_nr 0 1" means getting no rm_addr, but a rm_subflow.

Indeed but you have this behaviour only because the userspace PM didn't
send this RM ADDR, no? I agree it might be better to do so but it is not
mandatory if I'm not mistaken, especially for the client side. Then it
is up to the userspace PM to decide to send the RM ADDR or not and we
cannot force it I would say.

Or maybe the client side cannot send a RM ADDR? (on the other hand, the
address was not announced with an ADD_ADDR so why do you want to send a
RM_ADDR?)

(if there is a need, we can ease its use with an extra flag but I don't
think we should force this behaviour, it is up to the PM to do that, no?)

> This behavior is incorrect. This patch fix it. It looks through the
> userspace_pm_local_addr_list to find the addr id, and send RM_ADDR
> with this id.
> 
> So patch 2 is needed to add the address into
> userspace_pm_local_addr_list, and patch 1 too, no clear the address
> id.

Would it not work if you modify do_transfer() to use pm_nl_ctl to send
the RM_ADDR then do the destroy (I don't know if doing the opposite is
supported)?

But again, I don't think we need to send an RM_ADD if no ADD_ADDR have
been sent before by the client here.

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net
Re: [PATCH mptcp-next v5 3/7] mptcp: close remote subflow when destroying it
Posted by Geliang Tang 1 year, 2 months ago
Matthieu Baerts <matthieu.baerts@tessares.net> 于2023年3月21日周二 00:09写道:
>
> Hi Geliang,
>
> Thank you for your reply.
>
> On 20/03/2023 12:41, Geliang Tang wrote:
> > Hi Matt,
> >
> > Thanks for your reviews.
> >
> > We always remove address together with subflow, see
> > mptcp_nl_remove_subflow_and_signal_addr() or
> > mptcp_pm_remove_addrs_and_subflows().
> >
> > So we always get "chk_rm_nr 1 1" in these tests:
> >
> >         # single subflow, remove
> >         if reset "remove single subflow"; then
> >                 pm_nl_set_limits $ns1 0 1
> >                 pm_nl_set_limits $ns2 0 1
> >                 pm_nl_add_endpoint $ns2 10.0.3.2 flags subflow
> >                 run_tests $ns1 $ns2 10.0.1.1 0 0 -1 slow
> >                 chk_join_nr 1 1 1
> >                 chk_rm_nr 1 1
> >         fi
> >
> >         # single address, remove
> >         if reset "remove single address"; then
> >                 pm_nl_set_limits $ns1 0 1
> >                 pm_nl_add_endpoint $ns1 10.0.2.1 flags signal
> >                 pm_nl_set_limits $ns2 1 1
> >                 run_tests $ns1 $ns2 10.0.1.1 0 -1 0 slow
> >                 chk_join_nr 1 1 1
> >                 chk_add_nr 1 1
> >                 chk_rm_nr 1 1 invert
> >         fi
> >
> >         # userspace pm add & remove address
> >         if reset_with_events "userspace pm add & remove address"; then
> >                 set_userspace_pm $ns1
> >                 pm_nl_set_limits $ns2 1 1
> >                 run_tests $ns1 $ns2 10.0.1.1 0 userspace_1 0 slow
> >                 chk_join_nr 1 1 1
> >                 chk_add_nr 1 1
> >                 chk_rm_nr 1 1 invert
> >                 kill_events_pids
> >         fi
> >
> > But the userspace remove subflow test got "chk_rm_nr 0 1":
> >
> >         # userspace pm create destroy subflow
> >         if reset_with_events "userspace pm create destroy subflow"; then
> >                 set_userspace_pm $ns2
> >                 pm_nl_set_limits $ns1 0 1
> >                 run_tests $ns1 $ns2 10.0.1.1 0 0 userspace_1 slow
> >                 chk_join_nr 1 1 1
> >                 chk_rm_nr 0 1
> >                 kill_events_pids
> >         fi
> >
> > "chk_rm_nr 0 1" means getting no rm_addr, but a rm_subflow.
>
> Indeed but you have this behaviour only because the userspace PM didn't
> send this RM ADDR, no? I agree it might be better to do so but it is not
> mandatory if I'm not mistaken, especially for the client side. Then it
> is up to the userspace PM to decide to send the RM ADDR or not and we
> cannot force it I would say.
>
> Or maybe the client side cannot send a RM ADDR? (on the other hand, the
> address was not announced with an ADD_ADDR so why do you want to send a
> RM_ADDR?)
>
> (if there is a need, we can ease its use with an extra flag but I don't
> think we should force this behaviour, it is up to the PM to do that, no?)
>
> > This behavior is incorrect. This patch fix it. It looks through the
> > userspace_pm_local_addr_list to find the addr id, and send RM_ADDR
> > with this id.
> >
> > So patch 2 is needed to add the address into
> > userspace_pm_local_addr_list, and patch 1 too, no clear the address
> > id.
>
> Would it not work if you modify do_transfer() to use pm_nl_ctl to send
> the RM_ADDR then do the destroy (I don't know if doing the opposite is
> supported)?
>
> But again, I don't think we need to send an RM_ADD if no ADD_ADDR have
> been sent before by the client here.

No, we must send an RM_ADDR to shutdown the subflow and close ssk on
the other side. I guess we should hear Paolo's opinion here. @Paolo
Abeni

>
> Cheers,
> Matt
> --
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net
Re: [PATCH mptcp-next v5 3/7] mptcp: close remote subflow when destroying it
Posted by Matthieu Baerts 1 year, 2 months ago
Hi Geliang,

On 21/03/2023 03:52, Geliang Tang wrote:
> Matthieu Baerts <matthieu.baerts@tessares.net> 于2023年3月21日周二 00:09写道:
>>
>> Hi Geliang,
>>
>> Thank you for your reply.
>>
>> On 20/03/2023 12:41, Geliang Tang wrote:
>>> Hi Matt,
>>>
>>> Thanks for your reviews.
>>>
>>> We always remove address together with subflow, see
>>> mptcp_nl_remove_subflow_and_signal_addr() or
>>> mptcp_pm_remove_addrs_and_subflows().
>>>
>>> So we always get "chk_rm_nr 1 1" in these tests:
>>>
>>>         # single subflow, remove
>>>         if reset "remove single subflow"; then
>>>                 pm_nl_set_limits $ns1 0 1
>>>                 pm_nl_set_limits $ns2 0 1
>>>                 pm_nl_add_endpoint $ns2 10.0.3.2 flags subflow
>>>                 run_tests $ns1 $ns2 10.0.1.1 0 0 -1 slow
>>>                 chk_join_nr 1 1 1
>>>                 chk_rm_nr 1 1
>>>         fi
>>>
>>>         # single address, remove
>>>         if reset "remove single address"; then
>>>                 pm_nl_set_limits $ns1 0 1
>>>                 pm_nl_add_endpoint $ns1 10.0.2.1 flags signal
>>>                 pm_nl_set_limits $ns2 1 1
>>>                 run_tests $ns1 $ns2 10.0.1.1 0 -1 0 slow
>>>                 chk_join_nr 1 1 1
>>>                 chk_add_nr 1 1
>>>                 chk_rm_nr 1 1 invert
>>>         fi
>>>
>>>         # userspace pm add & remove address
>>>         if reset_with_events "userspace pm add & remove address"; then
>>>                 set_userspace_pm $ns1
>>>                 pm_nl_set_limits $ns2 1 1
>>>                 run_tests $ns1 $ns2 10.0.1.1 0 userspace_1 0 slow
>>>                 chk_join_nr 1 1 1
>>>                 chk_add_nr 1 1
>>>                 chk_rm_nr 1 1 invert
>>>                 kill_events_pids
>>>         fi
>>>
>>> But the userspace remove subflow test got "chk_rm_nr 0 1":
>>>
>>>         # userspace pm create destroy subflow
>>>         if reset_with_events "userspace pm create destroy subflow"; then
>>>                 set_userspace_pm $ns2
>>>                 pm_nl_set_limits $ns1 0 1
>>>                 run_tests $ns1 $ns2 10.0.1.1 0 0 userspace_1 slow
>>>                 chk_join_nr 1 1 1
>>>                 chk_rm_nr 0 1
>>>                 kill_events_pids
>>>         fi
>>>
>>> "chk_rm_nr 0 1" means getting no rm_addr, but a rm_subflow.
>>
>> Indeed but you have this behaviour only because the userspace PM didn't
>> send this RM ADDR, no? I agree it might be better to do so but it is not
>> mandatory if I'm not mistaken, especially for the client side. Then it
>> is up to the userspace PM to decide to send the RM ADDR or not and we
>> cannot force it I would say.
>>
>> Or maybe the client side cannot send a RM ADDR? (on the other hand, the
>> address was not announced with an ADD_ADDR so why do you want to send a
>> RM_ADDR?)
>>
>> (if there is a need, we can ease its use with an extra flag but I don't
>> think we should force this behaviour, it is up to the PM to do that, no?)
>>
>>> This behavior is incorrect. This patch fix it. It looks through the
>>> userspace_pm_local_addr_list to find the addr id, and send RM_ADDR
>>> with this id.
>>>
>>> So patch 2 is needed to add the address into
>>> userspace_pm_local_addr_list, and patch 1 too, no clear the address
>>> id.
>>
>> Would it not work if you modify do_transfer() to use pm_nl_ctl to send
>> the RM_ADDR then do the destroy (I don't know if doing the opposite is
>> supported)?
>>
>> But again, I don't think we need to send an RM_ADD if no ADD_ADDR have
>> been sent before by the client here.
> 
> No, we must send an RM_ADDR to shutdown the subflow and close ssk on
> the other side. I guess we should hear Paolo's opinion here. @Paolo
> Abeni

We discussed about that at the last meeting: we confirm that here, the
userspace daemon is the owner and then it is in charge of all actions
related to the PM including sending RM_ADDR then.

Typically, the RM_ADDR are sent when a previously announced address has
been removed. In the test you modified ("userspace pm create destroy
subflow"), it might even looks strange for the client to send a RM_ADDR
for a subflow it has created on an address it didn't explicitly
announced. The client no longer wants to use it, fine, it simply sends a
TCP FIN, no need to do more, no? (or at least, no need to force the send
of a RM_ADDR)

WDYT?

Maybe we could even think about changing the in-kernel PM not to send
the RM_ADDR if the address was not announced before?
(I think we should not change that, in the case of the in-kernel PM,
these RM_ADDR are sent only when the address has been explicitly removed
from the endpoints so it somehow makes sense)


While talking about that, I was thinking about this: for the moment, the
userspace PM can only send RM_ADDR for addresses it has previously
announced, right?
In this case, if the userspace PM daemon for the client side detects an
issue with its "2nd" address -- e.g. a Netlink event is sent to the
userspace daemon because a subflow got closed with an error or because
an IP address is no longer available -- the userspace PM daemon cannot
send an RM_ADDR to the server to tell it to stop sending data on this
subflow. I can create a new ticket on GitHub for that.

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