[PATCH mptcp-net] mptcp: fix race in overlapping signal events

Paolo Abeni posted 1 patch 2 years, 1 month ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/multipath-tcp/mptcp_net-next tags/patchew/c5d17a37b8ad315e72dd8551e148017a66987f7f.1644266387.git.pabeni@redhat.com
Maintainers: Matthieu Baerts <matthieu.baerts@tessares.net>, Paolo Abeni <pabeni@redhat.com>, Shuah Khan <shuah@kernel.org>, "David S. Miller" <davem@davemloft.net>, Mat Martineau <mathew.j.martineau@linux.intel.com>, Jakub Kicinski <kuba@kernel.org>
net/mptcp/pm_netlink.c                          | 10 ++++++++++
tools/testing/selftests/net/mptcp/mptcp_join.sh |  2 +-
2 files changed, 11 insertions(+), 1 deletion(-)
[PATCH mptcp-net] mptcp: fix race in overlapping signal events
Posted by Paolo Abeni 2 years, 1 month ago
After commit a88c9e496937 ("mptcp: do not block subflows
creation on errors"), if a signal address races with a failing
subflow creation, the subflow creation failure control path
can trigger the selection of the next address to be announced
while the current announced is still pending.

The above will cause the unintended suppression of the ADD_ADDR
announce.

Fix the issue skipping the to-be-suppressed announce before it
will mark an endpoint as already used. The relevant announce
will be triggered again when the current one will complete.

Fixes: a88c9e496937 ("mptcp: do not block subflows creation on errors")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/pm_netlink.c                          | 10 ++++++++++
 tools/testing/selftests/net/mptcp/mptcp_join.sh |  2 +-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 93800f32fcb6..fb8d4bfcfbd6 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -546,6 +546,16 @@ static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
 	if (msk->pm.add_addr_signaled < add_addr_signal_max) {
 		local = select_signal_address(pernet, msk);
 
+		/* due to racing events on both ends we can reach here while
+		 * previous add address is still running: if we invoke now
+		 * mptcp_pm_announce_addr(), that will fail and the
+		 * corresponding id will be marked as used.
+		 * Instead let the PM machinery reschedule us when the
+		 * current address announce will be completed.
+		 */
+		if (msk->pm.addr_signal & BIT(MPTCP_ADD_ADDR_SIGNAL))
+			return;
+
 		if (local) {
 			if (mptcp_pm_alloc_anno_list(msk, local)) {
 				__clear_bit(local->addr.id, msk->pm.id_avail_bitmap);
diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 007223364718..ba32ea616206 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -1341,7 +1341,7 @@ signal_address_tests()
 	pm_nl_add_endpoint $ns2 10.0.2.2 flags signal
 	pm_nl_add_endpoint $ns2 10.0.3.2 flags signal
 	pm_nl_add_endpoint $ns2 10.0.4.2 flags signal
-	run_tests $ns1 $ns2 10.0.1.1
+	run_tests $ns1 $ns2 10.0.1.1 0 0 0 slow
 	chk_join_nr "signal addresses race test" 3 3 3
 
 	# the server will not signal the address terminating
-- 
2.34.1


Re: [PATCH mptcp-net] mptcp: fix race in overlapping signal events
Posted by Paolo Abeni 2 years, 1 month ago
On Mon, 2022-02-07 at 21:39 +0100, Paolo Abeni wrote:
> After commit a88c9e496937 ("mptcp: do not block subflows
> creation on errors"), if a signal address races with a failing
> subflow creation, the subflow creation failure control path
> can trigger the selection of the next address to be announced
> while the current announced is still pending.
> 
> The above will cause the unintended suppression of the ADD_ADDR
> announce.
> 
> Fix the issue skipping the to-be-suppressed announce before it
> will mark an endpoint as already used. The relevant announce
> will be triggered again when the current one will complete.
> 
> Fixes: a88c9e496937 ("mptcp: do not block subflows creation on errors")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

Please keep this on-hold a little, it need a little more testing,

Thanks!

Paolo


Re: [PATCH mptcp-net] mptcp: fix race in overlapping signal events
Posted by Matthieu Baerts 2 years, 1 month ago
Hi Paolo,

On 07/02/2022 21:53, Paolo Abeni wrote:
> On Mon, 2022-02-07 at 21:39 +0100, Paolo Abeni wrote:
>> After commit a88c9e496937 ("mptcp: do not block subflows
>> creation on errors"), if a signal address races with a failing
>> subflow creation, the subflow creation failure control path
>> can trigger the selection of the next address to be announced
>> while the current announced is still pending.
>>
>> The above will cause the unintended suppression of the ADD_ADDR
>> announce.
>>
>> Fix the issue skipping the to-be-suppressed announce before it
>> will mark an endpoint as already used. The relevant announce
>> will be triggered again when the current one will complete.
>>
>> Fixes: a88c9e496937 ("mptcp: do not block subflows creation on errors")
>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

Thank you for looking at that!

> Please keep this on-hold a little, it need a little more testing,

I still have the same issue with this patch. Do not hesitate if you need
me to provide you more debug info.

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