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.3Hi 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
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
>
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
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
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
© 2016 - 2025 Red Hat, Inc.