| Message ID | 4a8ac95282ec2b551a64134f24b2e2fd7d82973c.1719589916.git.pabeni@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | mptcp: fix signal endpoint readd | expand |
| Context | Check | Description |
|---|---|---|
| matttbe/build | success | Build and static analysis OK |
| matttbe/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 58 lines checked |
| matttbe/shellcheck | success | MPTCP selftests files have not been modified |
| matttbe/KVM_Validation__normal | success | Success! ✅ |
| matttbe/KVM_Validation__debug | success | Success! ✅ |
| matttbe/KVM_Validation__btf__only_bpftest_all_ | success | Success! ✅ |
On Fri, 28 Jun 2024, Paolo Abeni wrote: > Currently the per connection announced address counter is never > decreased. As a consequence, after connection establishment, if > the NL PM deletes an endpoint and adds a new/different one, no > additional subflow is created for the new endpoint even if the > current limits allow that. > > Address the issue properly updating the signaled address counter > every time one of such address is removed. > > Fixes: 01cacb00b35c ("mptcp: add netlink-based PM") > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > --- > slightly unrelated: is unclear to me why mptcp_pm_remove_addrs() does > things slightly differently from mptcp_pm_remove_addrs_and_subflows(), > with the latter IMHO doing the right thing (TM) Hi Paolo - I don't remember why mptcp_pm_remove_addrs() has the extra code to send the RM_ADDR for addresses that are in the conn_list but not the anno_list. Maybe to allow a way to sent RM_ADDR as a "request for the peer to initiate a disconnect" even on implicitly added address IDs (from outgoing subflow connections instead of ADD_ADDRs)? > --- > net/mptcp/pm_netlink.c | 27 +++++++++++++++++++-------- > 1 file changed, 19 insertions(+), 8 deletions(-) > > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c > index ea9e5817b9e9..624f9ea66aea 100644 > --- a/net/mptcp/pm_netlink.c > +++ b/net/mptcp/pm_netlink.c > @@ -1401,6 +1401,7 @@ static bool mptcp_pm_remove_anno_addr(struct mptcp_sock *msk, > ret = remove_anno_list_by_saddr(msk, addr); > if (ret || force) { > spin_lock_bh(&msk->pm.lock); > + msk->pm.add_addr_signaled--; Looks like this only be decremented if ret is true, since that indicates a list element was removed and add_addr_signaled is supposed (?) to track the length of the anno_list. - Mat > mptcp_pm_remove_addr(msk, &list); > spin_unlock_bh(&msk->pm.lock); > } > @@ -1534,16 +1535,25 @@ void mptcp_pm_remove_addrs(struct mptcp_sock *msk, struct list_head *rm_list) > { > struct mptcp_rm_list alist = { .nr = 0 }; > struct mptcp_pm_addr_entry *entry; > + int anno_nr = 0; > > list_for_each_entry(entry, rm_list, list) { > - if ((remove_anno_list_by_saddr(msk, &entry->addr) || > - lookup_subflow_by_saddr(&msk->conn_list, &entry->addr)) && > - alist.nr < MPTCP_RM_IDS_MAX) > - alist.ids[alist.nr++] = entry->addr.id; > + if (alist.nr >= MPTCP_RM_IDS_MAX) > + break; > + > + /* only delete if either announced or matching a subflow */ > + if (remove_anno_list_by_saddr(msk, &entry->addr)) > + anno_nr++; > + else if (!lookup_subflow_by_saddr(&msk->conn_list, > + &entry->addr)) > + continue; > + > + alist.ids[alist.nr++] = entry->addr.id; > } > > if (alist.nr) { > spin_lock_bh(&msk->pm.lock); > + msk->pm.add_addr_signaled -= anno_nr; > mptcp_pm_remove_addr(msk, &alist); > spin_unlock_bh(&msk->pm.lock); > } > @@ -1556,17 +1566,18 @@ static void mptcp_pm_remove_addrs_and_subflows(struct mptcp_sock *msk, > struct mptcp_pm_addr_entry *entry; > > list_for_each_entry(entry, rm_list, list) { > - if (lookup_subflow_by_saddr(&msk->conn_list, &entry->addr) && > - slist.nr < MPTCP_RM_IDS_MAX) > + if (slist.nr < MPTCP_RM_IDS_MAX && > + lookup_subflow_by_saddr(&msk->conn_list, &entry->addr)) > slist.ids[slist.nr++] = entry->addr.id; > > - if (remove_anno_list_by_saddr(msk, &entry->addr) && > - alist.nr < MPTCP_RM_IDS_MAX) > + if (alist.nr < MPTCP_RM_IDS_MAX && > + remove_anno_list_by_saddr(msk, &entry->addr)) > alist.ids[alist.nr++] = entry->addr.id; > } > > if (alist.nr) { > spin_lock_bh(&msk->pm.lock); > + msk->pm.add_addr_signaled -= alist.nr; > mptcp_pm_remove_addr(msk, &alist); > spin_unlock_bh(&msk->pm.lock); > } > -- > 2.45.1 > > >
Hi Paolo, Mat, On 01/07/2024 20:08, Mat Martineau wrote: > On Fri, 28 Jun 2024, Paolo Abeni wrote: > >> Currently the per connection announced address counter is never >> decreased. As a consequence, after connection establishment, if >> the NL PM deletes an endpoint and adds a new/different one, no >> additional subflow is created for the new endpoint even if the >> current limits allow that. >> >> Address the issue properly updating the signaled address counter >> every time one of such address is removed. Thank you for having looked at that! >> Fixes: 01cacb00b35c ("mptcp: add netlink-based PM") >> Signed-off-by: Paolo Abeni <pabeni@redhat.com> >> --- >> slightly unrelated: is unclear to me why mptcp_pm_remove_addrs() does >> things slightly differently from mptcp_pm_remove_addrs_and_subflows(), >> with the latter IMHO doing the right thing (TM) > > Hi Paolo - > > I don't remember why mptcp_pm_remove_addrs() has the extra code to send > the RM_ADDR for addresses that are in the conn_list but not the > anno_list. Maybe to allow a way to sent RM_ADDR as a "request for the > peer to initiate a disconnect" even on implicitly added address IDs > (from outgoing subflow connections instead of ADD_ADDRs)? mptcp_pm_remove_addrs() is only used by the userspace PM, it should probably be moved to pm_userspace.c in fact (but it uses helpers from pm_netlink.c, I guess that's why it is there). My understanding is that it also checks the local subflows list because the userspace can ask to send a remove ADDR for addresses that have been "implicitly" announced via the MPJoin as well. Maybe the modification in mptcp_pm_remove_addrs() should be in a different commit? With a different Fixes tag then: Fixes: 8b1c94da1e48 ("mptcp: only send RM_ADDR in nl_cmd_remove") > >> --- >> net/mptcp/pm_netlink.c | 27 +++++++++++++++++++-------- >> 1 file changed, 19 insertions(+), 8 deletions(-) >> >> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c >> index ea9e5817b9e9..624f9ea66aea 100644 >> --- a/net/mptcp/pm_netlink.c >> +++ b/net/mptcp/pm_netlink.c >> @@ -1401,6 +1401,7 @@ static bool mptcp_pm_remove_anno_addr(struct >> mptcp_sock *msk, >> ret = remove_anno_list_by_saddr(msk, addr); >> if (ret || force) { >> spin_lock_bh(&msk->pm.lock); >> + msk->pm.add_addr_signaled--; > > Looks like this only be decremented if ret is true, since that indicates > a list element was removed and add_addr_signaled is supposed (?) to > track the length of the anno_list. Good point. Should we not also set the corresponding bit in: msk->pm.id_avail_signals_bitmap Linked to my series: https://lore.kernel.org/mptcp/20240621-mptcp-pm-avail-v1-0-b692d5eb89b5@kernel.org/T/ Otherwise, we will not be able to re-use the same ID, no? And if we change add_addr_signaled counter here after having removed an address, maybe now we are no longer at the signalled limit, should we then call mptcp_pm_create_subflow_or_signal_addr()? Cheers, Matt
Hi Paolo, On 02/07/2024 10:51, Matthieu Baerts wrote: > Hi Paolo, Mat, > > On 01/07/2024 20:08, Mat Martineau wrote: >> On Fri, 28 Jun 2024, Paolo Abeni wrote: >> >>> Currently the per connection announced address counter is never >>> decreased. As a consequence, after connection establishment, if >>> the NL PM deletes an endpoint and adds a new/different one, no >>> additional subflow is created for the new endpoint even if the >>> current limits allow that. >>> >>> Address the issue properly updating the signaled address counter >>> every time one of such address is removed. > > Thank you for having looked at that! > >>> Fixes: 01cacb00b35c ("mptcp: add netlink-based PM") >>> Signed-off-by: Paolo Abeni <pabeni@redhat.com> >>> --- >>> slightly unrelated: is unclear to me why mptcp_pm_remove_addrs() does >>> things slightly differently from mptcp_pm_remove_addrs_and_subflows(), >>> with the latter IMHO doing the right thing (TM) >> >> Hi Paolo - >> >> I don't remember why mptcp_pm_remove_addrs() has the extra code to send >> the RM_ADDR for addresses that are in the conn_list but not the >> anno_list. Maybe to allow a way to sent RM_ADDR as a "request for the >> peer to initiate a disconnect" even on implicitly added address IDs >> (from outgoing subflow connections instead of ADD_ADDRs)? > > mptcp_pm_remove_addrs() is only used by the userspace PM, it should > probably be moved to pm_userspace.c in fact (but it uses helpers from > pm_netlink.c, I guess that's why it is there). My understanding is that > it also checks the local subflows list because the userspace can ask to > send a remove ADDR for addresses that have been "implicitly" announced > via the MPJoin as well. > > Maybe the modification in mptcp_pm_remove_addrs() should be in a > different commit? With a different Fixes tag then: > > Fixes: 8b1c94da1e48 ("mptcp: only send RM_ADDR in nl_cmd_remove") > >> >>> --- >>> net/mptcp/pm_netlink.c | 27 +++++++++++++++++++-------- >>> 1 file changed, 19 insertions(+), 8 deletions(-) >>> >>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c >>> index ea9e5817b9e9..624f9ea66aea 100644 >>> --- a/net/mptcp/pm_netlink.c >>> +++ b/net/mptcp/pm_netlink.c >>> @@ -1401,6 +1401,7 @@ static bool mptcp_pm_remove_anno_addr(struct >>> mptcp_sock *msk, >>> ret = remove_anno_list_by_saddr(msk, addr); >>> if (ret || force) { >>> spin_lock_bh(&msk->pm.lock); >>> + msk->pm.add_addr_signaled--; >> >> Looks like this only be decremented if ret is true, since that indicates >> a list element was removed and add_addr_signaled is supposed (?) to >> track the length of the anno_list. > > Good point. > > Should we not also set the corresponding bit in: > > msk->pm.id_avail_signals_bitmap > > Linked to my series: > > > https://lore.kernel.org/mptcp/20240621-mptcp-pm-avail-v1-0-b692d5eb89b5@kernel.org/T/ > > Otherwise, we will not be able to re-use the same ID, no? It is a bit confusing, but it looks like the bitmap is modified in some cases: - mptcp_pm_remove_anno_addr() is only called from mptcp_nl_remove_subflow_and_signal_addr(), but for 2 different cases: - If there are no additional subflows (list_empty(&msk->conn_list)), force == false → the bitmap is not modified - If there are additional subflows, and a matching one, force could be set to true (if not implicit), but also mptcp_pm_remove_subflow() will be called. In this case, the bitmap is modified. I wonder if it would not be clearer to set the available "signals" bit in mptcp_pm_remove_addr(), and set the available "subflows" bit in mptcp_pm_nl_rm_addr_or_subflow(MPTCP_MIB_RMSUBFLOW) -- what the v1 of my patch is doing [1] -- so one is dedicated to remove endpoints with the 'signal' flag, and the other one for endpoints with the 'subflow' flag. WDYT? [1] https://lore.kernel.org/r/20240621-mptcp-pm-avail-v1-2-b692d5eb89b5@kernel.org > And if we change add_addr_signaled counter here after having removed an > address, maybe now we are no longer at the signalled limit, should we > then call mptcp_pm_create_subflow_or_signal_addr()? > > Cheers, > Matt Cheers, Matt
On Tue, 2024-07-02 at 10:51 +0200, Matthieu Baerts wrote: > Hi Paolo, Mat, > > On 01/07/2024 20:08, Mat Martineau wrote: > > On Fri, 28 Jun 2024, Paolo Abeni wrote: > > > > > Currently the per connection announced address counter is never > > > decreased. As a consequence, after connection establishment, if > > > the NL PM deletes an endpoint and adds a new/different one, no > > > additional subflow is created for the new endpoint even if the > > > current limits allow that. > > > > > > Address the issue properly updating the signaled address counter > > > every time one of such address is removed. > > Thank you for having looked at that! > > > > Fixes: 01cacb00b35c ("mptcp: add netlink-based PM") > > > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > > > --- > > > slightly unrelated: is unclear to me why mptcp_pm_remove_addrs() does > > > things slightly differently from mptcp_pm_remove_addrs_and_subflows(), > > > with the latter IMHO doing the right thing (TM) > > > > Hi Paolo - > > > > I don't remember why mptcp_pm_remove_addrs() has the extra code to send > > the RM_ADDR for addresses that are in the conn_list but not the > > anno_list. Maybe to allow a way to sent RM_ADDR as a "request for the > > peer to initiate a disconnect" even on implicitly added address IDs > > (from outgoing subflow connections instead of ADD_ADDRs)? > > mptcp_pm_remove_addrs() is only used by the userspace PM, it should > probably be moved to pm_userspace.c in fact (but it uses helpers from > pm_netlink.c, I guess that's why it is there). My understanding is that > it also checks the local subflows list because the userspace can ask to > send a remove ADDR for addresses that have been "implicitly" announced > via the MPJoin as well. > > Maybe the modification in mptcp_pm_remove_addrs() should be in a > different commit? With a different Fixes tag then: > > Fixes: 8b1c94da1e48 ("mptcp: only send RM_ADDR in nl_cmd_remove") > > > > > > --- > > > net/mptcp/pm_netlink.c | 27 +++++++++++++++++++-------- > > > 1 file changed, 19 insertions(+), 8 deletions(-) > > > > > > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c > > > index ea9e5817b9e9..624f9ea66aea 100644 > > > --- a/net/mptcp/pm_netlink.c > > > +++ b/net/mptcp/pm_netlink.c > > > @@ -1401,6 +1401,7 @@ static bool mptcp_pm_remove_anno_addr(struct > > > mptcp_sock *msk, > > > ret = remove_anno_list_by_saddr(msk, addr); > > > if (ret || force) { > > > spin_lock_bh(&msk->pm.lock); > > > + msk->pm.add_addr_signaled--; > > > > Looks like this only be decremented if ret is true, since that indicates > > a list element was removed and add_addr_signaled is supposed (?) to > > track the length of the anno_list. > > Good point. > > Should we not also set the corresponding bit in: > > msk->pm.id_avail_signals_bitmap > > Linked to my series: > > > https://lore.kernel.org/mptcp/20240621-mptcp-pm-avail-v1-0-b692d5eb89b5@kernel.org/T/ > > Otherwise, we will not be able to re-use the same ID, no? This is for net, your series is rightfully for net-next, I guess you should do the id_avail_signals_bitmap accounting only on the latter. Thanks, Paolo
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c index ea9e5817b9e9..624f9ea66aea 100644 --- a/net/mptcp/pm_netlink.c +++ b/net/mptcp/pm_netlink.c @@ -1401,6 +1401,7 @@ static bool mptcp_pm_remove_anno_addr(struct mptcp_sock *msk, ret = remove_anno_list_by_saddr(msk, addr); if (ret || force) { spin_lock_bh(&msk->pm.lock); + msk->pm.add_addr_signaled--; mptcp_pm_remove_addr(msk, &list); spin_unlock_bh(&msk->pm.lock); } @@ -1534,16 +1535,25 @@ void mptcp_pm_remove_addrs(struct mptcp_sock *msk, struct list_head *rm_list) { struct mptcp_rm_list alist = { .nr = 0 }; struct mptcp_pm_addr_entry *entry; + int anno_nr = 0; list_for_each_entry(entry, rm_list, list) { - if ((remove_anno_list_by_saddr(msk, &entry->addr) || - lookup_subflow_by_saddr(&msk->conn_list, &entry->addr)) && - alist.nr < MPTCP_RM_IDS_MAX) - alist.ids[alist.nr++] = entry->addr.id; + if (alist.nr >= MPTCP_RM_IDS_MAX) + break; + + /* only delete if either announced or matching a subflow */ + if (remove_anno_list_by_saddr(msk, &entry->addr)) + anno_nr++; + else if (!lookup_subflow_by_saddr(&msk->conn_list, + &entry->addr)) + continue; + + alist.ids[alist.nr++] = entry->addr.id; } if (alist.nr) { spin_lock_bh(&msk->pm.lock); + msk->pm.add_addr_signaled -= anno_nr; mptcp_pm_remove_addr(msk, &alist); spin_unlock_bh(&msk->pm.lock); } @@ -1556,17 +1566,18 @@ static void mptcp_pm_remove_addrs_and_subflows(struct mptcp_sock *msk, struct mptcp_pm_addr_entry *entry; list_for_each_entry(entry, rm_list, list) { - if (lookup_subflow_by_saddr(&msk->conn_list, &entry->addr) && - slist.nr < MPTCP_RM_IDS_MAX) + if (slist.nr < MPTCP_RM_IDS_MAX && + lookup_subflow_by_saddr(&msk->conn_list, &entry->addr)) slist.ids[slist.nr++] = entry->addr.id; - if (remove_anno_list_by_saddr(msk, &entry->addr) && - alist.nr < MPTCP_RM_IDS_MAX) + if (alist.nr < MPTCP_RM_IDS_MAX && + remove_anno_list_by_saddr(msk, &entry->addr)) alist.ids[alist.nr++] = entry->addr.id; } if (alist.nr) { spin_lock_bh(&msk->pm.lock); + msk->pm.add_addr_signaled -= alist.nr; mptcp_pm_remove_addr(msk, &alist); spin_unlock_bh(&msk->pm.lock); }
Currently the per connection announced address counter is never decreased. As a consequence, after connection establishment, if the NL PM deletes an endpoint and adds a new/different one, no additional subflow is created for the new endpoint even if the current limits allow that. Address the issue properly updating the signaled address counter every time one of such address is removed. Fixes: 01cacb00b35c ("mptcp: add netlink-based PM") Signed-off-by: Paolo Abeni <pabeni@redhat.com> --- slightly unrelated: is unclear to me why mptcp_pm_remove_addrs() does things slightly differently from mptcp_pm_remove_addrs_and_subflows(), with the latter IMHO doing the right thing (TM) --- net/mptcp/pm_netlink.c | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-)