| Message ID | 1392a4ef6a8a45ca3a920f933a12766283a828c6.1707144963.git.pabeni@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Commit | 22e3b19337f7e6d7bad6bf2be412560588684d4d |
| Delegated to: | Mat Martineau |
| Headers | show |
| Series | mptcp: fix duplicate subflow creation | expand |
| Context | Check | Description |
|---|---|---|
| matttbe/checkpatch | warning | total: 0 errors, 2 warnings, 0 checks, 54 lines checked |
| matttbe/build | success | Build and static analysis OK |
| matttbe/KVM_Validation__normal | success | Success! ✅ |
| matttbe/KVM_Validation__debug__except_selftest_mptcp_join_ | warning | Unstable: 1 failed test(s): packetdrill_regressions |
| matttbe/KVM_Validation__debug__only_selftest_mptcp_join_ | success | Success! ✅ |
Hi Paolo,
Thank you for your modifications, that's great!
Our CI (GitHub Action) did some validations and here is its report:
- KVM Validation: normal:
- Success! ✅:
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/7787400213
Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/75f455f724d8
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-normal
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 (NGI0 Core)
Hi Paolo, Thank you for your modifications, that's great! Our CI (Cirrus) did some validations with a debug kernel and here is its report: - KVM Validation: debug (except selftest_mptcp_join): - Unstable: 1 failed test(s): packetdrill_regressions
On Mon, 5 Feb 2024, Paolo Abeni wrote: > Fullmesh endpoints could end-up unexpectedly generating duplicate > subflows - same local and remote addresses - when multiple incoming > ADD_ADDR are processed before the PM creates the subflow for the local > endpoints. > > Address the issue explicitly checking for duplicates at subflow > creation time. > > To avoid a quadratic computational complexity, track the unavailable > remote address ids in a temporary bitmap and initialize such bitmap > with the remote ids of all the existing subflows matching the local > address currently processed. > > The above allows additionally replacing the existing code checking > for duplicate entry in the current set with a simple bit test > operation. > > Fixes: 2843ff6f36db ("mptcp: remote addresses fullmesh") > Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/435 > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > --- > Note that there is no problem for the opposite event sequence. > --- > net/mptcp/pm_netlink.c | 33 ++++++++++++++++++--------------- > 1 file changed, 18 insertions(+), 15 deletions(-) > > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c > index 066bc855365c..617aad83eb87 100644 > --- a/net/mptcp/pm_netlink.c > +++ b/net/mptcp/pm_netlink.c > @@ -396,19 +396,6 @@ void mptcp_pm_free_anno_list(struct mptcp_sock *msk) > } > } > > -static bool lookup_address_in_vec(const struct mptcp_addr_info *addrs, unsigned int nr, > - const struct mptcp_addr_info *addr) > -{ > - int i; > - > - for (i = 0; i < nr; i++) { > - if (addrs[i].id == addr->id) > - return true; > - } > - > - return false; > -} > - > /* Fill all the remote addresses into the array addrs[], > * and return the array size. > */ > @@ -440,6 +427,16 @@ static unsigned int fill_remote_addresses_vec(struct mptcp_sock *msk, > msk->pm.subflows++; > addrs[i++] = remote; > } else { > + DECLARE_BITMAP(unavail_id, MPTCP_PM_MAX_ADDR_ID + 1); > + > + /* Forbit creation of new subflows matching existing Looks ok code-wise, but s/forbit/forbid/ here and... > + * ones, possibly already created by incoming ADD_ADDR > + */ > + bitmap_zero(unavail_id, MPTCP_PM_MAX_ADDR_ID + 1); > + mptcp_for_each_subflow(msk, subflow) > + if (READ_ONCE(subflow->local_id) == local->id) > + __set_bit(subflow->remote_id, unavail_id); > + > mptcp_for_each_subflow(msk, subflow) { > ssk = mptcp_subflow_tcp_sock(subflow); > remote_address((struct sock_common *)ssk, &addrs[i]); > @@ -447,11 +444,17 @@ static unsigned int fill_remote_addresses_vec(struct mptcp_sock *msk, > if (deny_id0 && !addrs[i].id) > continue; > > + if (test_bit(addrs[i].id, unavail_id)) > + continue; > + > if (!mptcp_pm_addr_families_match(sk, local, &addrs[i])) > continue; > > - if (!lookup_address_in_vec(addrs, i, &addrs[i]) && > - msk->pm.subflows < subflows_max) { > + if (msk->pm.subflows < subflows_max) { > + /* forbit creating multiple address towards ...here too. Or "Prevent" works too :) - Mat > + * this id > + */ > + __set_bit(addrs[i].id, unavail_id); > msk->pm.subflows++; > i++; > } > -- > 2.43.0 > > >
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c index 066bc855365c..617aad83eb87 100644 --- a/net/mptcp/pm_netlink.c +++ b/net/mptcp/pm_netlink.c @@ -396,19 +396,6 @@ void mptcp_pm_free_anno_list(struct mptcp_sock *msk) } } -static bool lookup_address_in_vec(const struct mptcp_addr_info *addrs, unsigned int nr, - const struct mptcp_addr_info *addr) -{ - int i; - - for (i = 0; i < nr; i++) { - if (addrs[i].id == addr->id) - return true; - } - - return false; -} - /* Fill all the remote addresses into the array addrs[], * and return the array size. */ @@ -440,6 +427,16 @@ static unsigned int fill_remote_addresses_vec(struct mptcp_sock *msk, msk->pm.subflows++; addrs[i++] = remote; } else { + DECLARE_BITMAP(unavail_id, MPTCP_PM_MAX_ADDR_ID + 1); + + /* Forbit creation of new subflows matching existing + * ones, possibly already created by incoming ADD_ADDR + */ + bitmap_zero(unavail_id, MPTCP_PM_MAX_ADDR_ID + 1); + mptcp_for_each_subflow(msk, subflow) + if (READ_ONCE(subflow->local_id) == local->id) + __set_bit(subflow->remote_id, unavail_id); + mptcp_for_each_subflow(msk, subflow) { ssk = mptcp_subflow_tcp_sock(subflow); remote_address((struct sock_common *)ssk, &addrs[i]); @@ -447,11 +444,17 @@ static unsigned int fill_remote_addresses_vec(struct mptcp_sock *msk, if (deny_id0 && !addrs[i].id) continue; + if (test_bit(addrs[i].id, unavail_id)) + continue; + if (!mptcp_pm_addr_families_match(sk, local, &addrs[i])) continue; - if (!lookup_address_in_vec(addrs, i, &addrs[i]) && - msk->pm.subflows < subflows_max) { + if (msk->pm.subflows < subflows_max) { + /* forbit creating multiple address towards + * this id + */ + __set_bit(addrs[i].id, unavail_id); msk->pm.subflows++; i++; }
Fullmesh endpoints could end-up unexpectedly generating duplicate subflows - same local and remote addresses - when multiple incoming ADD_ADDR are processed before the PM creates the subflow for the local endpoints. Address the issue explicitly checking for duplicates at subflow creation time. To avoid a quadratic computational complexity, track the unavailable remote address ids in a temporary bitmap and initialize such bitmap with the remote ids of all the existing subflows matching the local address currently processed. The above allows additionally replacing the existing code checking for duplicate entry in the current set with a simple bit test operation. Fixes: 2843ff6f36db ("mptcp: remote addresses fullmesh") Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/435 Signed-off-by: Paolo Abeni <pabeni@redhat.com> --- Note that there is no problem for the opposite event sequence. --- net/mptcp/pm_netlink.c | 33 ++++++++++++++++++--------------- 1 file changed, 18 insertions(+), 15 deletions(-)