Message ID | 20240726-mptcp-pm-avail-v5-4-fb1117ddeef6@kernel.org (mailing list archive) |
---|---|
State | Accepted, archived |
Commit | 5be0e90ef7d39d4c607026f05fd429fb75309345 |
Delegated to: | Matthieu Baerts |
Headers | show |
Series | mptcp: fix endpoints with 'signal' and 'subflow' flags | expand |
Context | Check | Description |
---|---|---|
matttbe/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 64 lines checked |
matttbe/shellcheck | success | MPTCP selftests files have not been modified |
matttbe/build | success | Build and static analysis OK |
matttbe/KVM_Validation__normal | success | Success! ✅ |
matttbe/KVM_Validation__debug | success | Success! ✅ |
matttbe/KVM_Validation__btf__only_bpftest_all_ | success | Success! ✅ |
Hi Matt, On Fri, 2024-07-26 at 16:28 +0200, Matthieu Baerts (NGI0) wrote: > Adding the following warning ... > > WARN_ON_ONCE(msk->pm.local_addr_used == 0) > > ... before decrementing the local_addr_used counter helped to find a > bug > when running the "remove single address" subtest from the > mptcp_join.sh > selftests. > > Removing a 'signal' endpoint will trigger the removal of all subflows > linked to this endpoint via mptcp_pm_nl_rm_addr_or_subflow() with > rm_type == MPTCP_MIB_RMSUBFLOW. This will decrement the > local_addr_used > counter, which is wrong in this case because this counter is linked > to > 'subflow' endpoints, and here it is a 'signal' endpoint that is being > removed. > > Now, the counter is decremented, only if the ID is being used outside > of mptcp_pm_nl_rm_addr_or_subflow(), only for 'subflow' endpoints, > and > if the ID is not 0 -- local_addr_used is not taking into account > these > ones. This marking of the ID as being available, and the decrement is > done no matter if a subflow using this ID is currently available, > because the subflow could have been closed before. > > Fixes: 06faa2271034 ("mptcp: remove multi addresses and subflows in > PM") > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> > --- > net/mptcp/pm_netlink.c | 26 +++++++++++++++++--------- > 1 file changed, 17 insertions(+), 9 deletions(-) > > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c > index 8a28fdaf3bb6..3ea417b52ff4 100644 > --- a/net/mptcp/pm_netlink.c > +++ b/net/mptcp/pm_netlink.c > @@ -833,10 +833,10 @@ static void > mptcp_pm_nl_rm_addr_or_subflow(struct mptcp_sock *msk, > if (rm_type == MPTCP_MIB_RMSUBFLOW) > __MPTCP_INC_STATS(sock_net(sk), > rm_type); > } > - if (rm_type == MPTCP_MIB_RMSUBFLOW) > - __set_bit(rm_id ? rm_id : msk- > >mpc_endpoint_id, msk->pm.id_avail_bitmap); > - else if (rm_type == MPTCP_MIB_RMADDR) > + > + if (rm_type == MPTCP_MIB_RMADDR) > __MPTCP_INC_STATS(sock_net(sk), rm_type); > + > if (!removed) > continue; > > @@ -846,8 +846,6 @@ static void mptcp_pm_nl_rm_addr_or_subflow(struct > mptcp_sock *msk, > if (rm_type == MPTCP_MIB_RMADDR) { > msk->pm.add_addr_accepted--; > WRITE_ONCE(msk->pm.accept_addr, true); > - } else if (rm_type == MPTCP_MIB_RMSUBFLOW) { > - msk->pm.local_addr_used--; Is it necessary to call __mark_subflow_endp_available() here? I'm not sure. > } > } > } > @@ -1443,6 +1441,14 @@ static bool mptcp_pm_remove_anno_addr(struct > mptcp_sock *msk, > return ret; > } > > +static void __mark_subflow_endp_available(struct mptcp_sock *msk, u8 > id) > +{ > + /* If it was marked as used, and not ID 0, decrement > local_addr_used */ > + if (!__test_and_set_bit(id ? : msk->mpc_endpoint_id, msk- > >pm.id_avail_bitmap) && > + id && !WARN_ON_ONCE(msk->pm.local_addr_used == 0)) > + msk->pm.local_addr_used--; > +} > + > static int mptcp_nl_remove_subflow_and_signal_addr(struct net *net, > const struct > mptcp_pm_addr_entry *entry) > { > @@ -1476,11 +1482,11 @@ static int > mptcp_nl_remove_subflow_and_signal_addr(struct net *net, > spin_lock_bh(&msk->pm.lock); > mptcp_pm_nl_rm_subflow_received(msk, &list); > spin_unlock_bh(&msk->pm.lock); > - } else if (entry->flags & > MPTCP_PM_ADDR_FLAG_SUBFLOW) { > - /* If the subflow has been used, but now > closed */ > + } > + > + if (entry->flags & MPTCP_PM_ADDR_FLAG_SUBFLOW) { > spin_lock_bh(&msk->pm.lock); > - if (!__test_and_set_bit(entry->addr.id, msk- > >pm.id_avail_bitmap)) > - msk->pm.local_addr_used--; > + __mark_subflow_endp_available(msk, entry- > >addr.id); > spin_unlock_bh(&msk->pm.lock); > } > > @@ -1518,6 +1524,7 @@ static int > mptcp_nl_remove_id_zero_address(struct net *net, > spin_lock_bh(&msk->pm.lock); > mptcp_pm_remove_addr(msk, &list); > mptcp_pm_nl_rm_subflow_received(msk, &list); > + __mark_subflow_endp_available(msk, 0); > spin_unlock_bh(&msk->pm.lock); > release_sock(sk); > > @@ -1919,6 +1926,7 @@ static void mptcp_pm_nl_fullmesh(struct > mptcp_sock *msk, > > spin_lock_bh(&msk->pm.lock); > mptcp_pm_nl_rm_subflow_received(msk, &list); > + __mark_subflow_endp_available(msk, addr->id); > mptcp_pm_create_subflow_or_signal_addr(msk); > spin_unlock_bh(&msk->pm.lock); > } >
Hi Geliang, Thank you for the review. On 29/07/2024 03:09, Geliang Tang wrote: > Hi Matt, > > On Fri, 2024-07-26 at 16:28 +0200, Matthieu Baerts (NGI0) wrote: >> Adding the following warning ... >> >> WARN_ON_ONCE(msk->pm.local_addr_used == 0) >> >> ... before decrementing the local_addr_used counter helped to find a >> bug >> when running the "remove single address" subtest from the >> mptcp_join.sh >> selftests. >> >> Removing a 'signal' endpoint will trigger the removal of all subflows >> linked to this endpoint via mptcp_pm_nl_rm_addr_or_subflow() with >> rm_type == MPTCP_MIB_RMSUBFLOW. This will decrement the >> local_addr_used >> counter, which is wrong in this case because this counter is linked >> to >> 'subflow' endpoints, and here it is a 'signal' endpoint that is being >> removed. >> >> Now, the counter is decremented, only if the ID is being used outside >> of mptcp_pm_nl_rm_addr_or_subflow(), only for 'subflow' endpoints, >> and >> if the ID is not 0 -- local_addr_used is not taking into account >> these >> ones. This marking of the ID as being available, and the decrement is >> done no matter if a subflow using this ID is currently available, >> because the subflow could have been closed before. >> >> Fixes: 06faa2271034 ("mptcp: remove multi addresses and subflows in >> PM") >> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> >> --- >> net/mptcp/pm_netlink.c | 26 +++++++++++++++++--------- >> 1 file changed, 17 insertions(+), 9 deletions(-) >> >> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c >> index 8a28fdaf3bb6..3ea417b52ff4 100644 >> --- a/net/mptcp/pm_netlink.c >> +++ b/net/mptcp/pm_netlink.c >> @@ -833,10 +833,10 @@ static void >> mptcp_pm_nl_rm_addr_or_subflow(struct mptcp_sock *msk, >> if (rm_type == MPTCP_MIB_RMSUBFLOW) >> __MPTCP_INC_STATS(sock_net(sk), >> rm_type); >> } >> - if (rm_type == MPTCP_MIB_RMSUBFLOW) >> - __set_bit(rm_id ? rm_id : msk- >>> mpc_endpoint_id, msk->pm.id_avail_bitmap); >> - else if (rm_type == MPTCP_MIB_RMADDR) >> + >> + if (rm_type == MPTCP_MIB_RMADDR) >> __MPTCP_INC_STATS(sock_net(sk), rm_type); >> + >> if (!removed) >> continue; >> >> @@ -846,8 +846,6 @@ static void mptcp_pm_nl_rm_addr_or_subflow(struct >> mptcp_sock *msk, >> if (rm_type == MPTCP_MIB_RMADDR) { >> msk->pm.add_addr_accepted--; >> WRITE_ONCE(msk->pm.accept_addr, true); >> - } else if (rm_type == MPTCP_MIB_RMSUBFLOW) { >> - msk->pm.local_addr_used--; > > > Is it necessary to call __mark_subflow_endp_available() here? I'm not > sure. No, because mptcp_pm_nl_rm_addr_or_subflow(MPTCP_MIB_RMSUBFLOW) will be called when removing an endpoint, to delete all linked subflows. That can be a 'signal' endpoint as well, while 'local_addr_used' is only linked to 'subflow' endpoint. There are more details about that in the commit message, but in short, that why I moved this to functions calling this helper, where there are more info about the endpoint type. Cheers, Matt
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c index 8a28fdaf3bb6..3ea417b52ff4 100644 --- a/net/mptcp/pm_netlink.c +++ b/net/mptcp/pm_netlink.c @@ -833,10 +833,10 @@ static void mptcp_pm_nl_rm_addr_or_subflow(struct mptcp_sock *msk, if (rm_type == MPTCP_MIB_RMSUBFLOW) __MPTCP_INC_STATS(sock_net(sk), rm_type); } - if (rm_type == MPTCP_MIB_RMSUBFLOW) - __set_bit(rm_id ? rm_id : msk->mpc_endpoint_id, msk->pm.id_avail_bitmap); - else if (rm_type == MPTCP_MIB_RMADDR) + + if (rm_type == MPTCP_MIB_RMADDR) __MPTCP_INC_STATS(sock_net(sk), rm_type); + if (!removed) continue; @@ -846,8 +846,6 @@ static void mptcp_pm_nl_rm_addr_or_subflow(struct mptcp_sock *msk, if (rm_type == MPTCP_MIB_RMADDR) { msk->pm.add_addr_accepted--; WRITE_ONCE(msk->pm.accept_addr, true); - } else if (rm_type == MPTCP_MIB_RMSUBFLOW) { - msk->pm.local_addr_used--; } } } @@ -1443,6 +1441,14 @@ static bool mptcp_pm_remove_anno_addr(struct mptcp_sock *msk, return ret; } +static void __mark_subflow_endp_available(struct mptcp_sock *msk, u8 id) +{ + /* If it was marked as used, and not ID 0, decrement local_addr_used */ + if (!__test_and_set_bit(id ? : msk->mpc_endpoint_id, msk->pm.id_avail_bitmap) && + id && !WARN_ON_ONCE(msk->pm.local_addr_used == 0)) + msk->pm.local_addr_used--; +} + static int mptcp_nl_remove_subflow_and_signal_addr(struct net *net, const struct mptcp_pm_addr_entry *entry) { @@ -1476,11 +1482,11 @@ static int mptcp_nl_remove_subflow_and_signal_addr(struct net *net, spin_lock_bh(&msk->pm.lock); mptcp_pm_nl_rm_subflow_received(msk, &list); spin_unlock_bh(&msk->pm.lock); - } else if (entry->flags & MPTCP_PM_ADDR_FLAG_SUBFLOW) { - /* If the subflow has been used, but now closed */ + } + + if (entry->flags & MPTCP_PM_ADDR_FLAG_SUBFLOW) { spin_lock_bh(&msk->pm.lock); - if (!__test_and_set_bit(entry->addr.id, msk->pm.id_avail_bitmap)) - msk->pm.local_addr_used--; + __mark_subflow_endp_available(msk, entry->addr.id); spin_unlock_bh(&msk->pm.lock); } @@ -1518,6 +1524,7 @@ static int mptcp_nl_remove_id_zero_address(struct net *net, spin_lock_bh(&msk->pm.lock); mptcp_pm_remove_addr(msk, &list); mptcp_pm_nl_rm_subflow_received(msk, &list); + __mark_subflow_endp_available(msk, 0); spin_unlock_bh(&msk->pm.lock); release_sock(sk); @@ -1919,6 +1926,7 @@ static void mptcp_pm_nl_fullmesh(struct mptcp_sock *msk, spin_lock_bh(&msk->pm.lock); mptcp_pm_nl_rm_subflow_received(msk, &list); + __mark_subflow_endp_available(msk, addr->id); mptcp_pm_create_subflow_or_signal_addr(msk); spin_unlock_bh(&msk->pm.lock); }
Adding the following warning ... WARN_ON_ONCE(msk->pm.local_addr_used == 0) ... before decrementing the local_addr_used counter helped to find a bug when running the "remove single address" subtest from the mptcp_join.sh selftests. Removing a 'signal' endpoint will trigger the removal of all subflows linked to this endpoint via mptcp_pm_nl_rm_addr_or_subflow() with rm_type == MPTCP_MIB_RMSUBFLOW. This will decrement the local_addr_used counter, which is wrong in this case because this counter is linked to 'subflow' endpoints, and here it is a 'signal' endpoint that is being removed. Now, the counter is decremented, only if the ID is being used outside of mptcp_pm_nl_rm_addr_or_subflow(), only for 'subflow' endpoints, and if the ID is not 0 -- local_addr_used is not taking into account these ones. This marking of the ID as being available, and the decrement is done no matter if a subflow using this ID is currently available, because the subflow could have been closed before. Fixes: 06faa2271034 ("mptcp: remove multi addresses and subflows in PM") Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> --- net/mptcp/pm_netlink.c | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-)