[PATCH mptcp-net v3 0/4] mptcp: fix races in add_addr handling

[PATCH mptcp-net v3 0/4] mptcp: fix races in add_addr handling

[Paolo Abeni]

the patches 1 && 2 fix actual races, even if the race addressed by
2/4 is not exposed by the self-tests, afaics.

patch 3 introduces some mibs to detect critical scenarios we can't
fix without major rework

patch 4 updates the self-test to be graceful in above cases.

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/254

Note: I did not attach the above tag to a specific patch, as all
of them will be needed to fix the issue

v2 -> v3:
 - fix missing init in patch 2/4

Paolo Abeni (4):
  mptcp: fix race in overlapping signal events
  mptcp: fix race in incoming ADD_ADDR option processing.
  mptcp: add mibs counter for ignored incoming options
  selftests: mptcp: more robust signal race test

 net/mptcp/mib.c                               |  2 ++
 net/mptcp/mib.h                               |  2 ++
 net/mptcp/pm.c                                |  8 +++--
 net/mptcp/pm_netlink.c                        | 29 +++++++++++++++----
 .../testing/selftests/net/mptcp/mptcp_join.sh | 16 ++++++++--
 5 files changed, 47 insertions(+), 10 deletions(-)

-- 
2.34.1

[PATCH mptcp-net v3 1/4] mptcp: fix race in overlapping signal events

[Paolo Abeni]

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 ++++++++++
 1 file changed, 10 insertions(+)

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);
-- 
2.34.1

[PATCH mptcp-net v3 2/4] mptcp: fix race in incoming ADD_ADDR option processing.

[Paolo Abeni]

If an MPTCP endpoint received moltiple consecutive incoming
ADD_ADDR options, mptcp_pm_add_addr_received() can overwrite
the current remote address value after the PM lock is released
in mptcp_pm_nl_add_addr_received() and before such address
is echoed.

Fix the issue caching the remote address value a little earlier
and always using the cached value after releasing the PM lock.

Fixes: f7efc7771eac ("mptcp: drop argument port from mptcp_pm_announce_addr")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
v2 -> v3:
 - properly initialize reset_port - Matt
---
 net/mptcp/pm_netlink.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index fb8d4bfcfbd6..98b485406afa 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -660,6 +660,7 @@ static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
 	unsigned int add_addr_accept_max;
 	struct mptcp_addr_info remote;
 	unsigned int subflows_max;
+	bool reset_port = false;
 	int i, nr;
 
 	add_addr_accept_max = mptcp_pm_get_add_addr_accept_max(msk);
@@ -669,15 +670,19 @@ static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
 		 msk->pm.add_addr_accepted, add_addr_accept_max,
 		 msk->pm.remote.family);
 
-	if (lookup_subflow_by_daddr(&msk->conn_list, &msk->pm.remote))
+	remote = msk->pm.remote;
+	if (lookup_subflow_by_daddr(&msk->conn_list, &remote))
 		goto add_addr_echo;
 
+	/* pick id 0 port, if none is provided the remote address */
+	if (!remote.port) {
+		reset_port = true;
+		remote.port = sk->sk_dport;
+	}
+
 	/* connect to the specified remote address, using whatever
 	 * local address the routing configuration will pick.
 	 */
-	remote = msk->pm.remote;
-	if (!remote.port)
-		remote.port = sk->sk_dport;
 	nr = fill_local_addresses_vec(msk, addrs);
 
 	msk->pm.add_addr_accepted++;
@@ -690,8 +695,12 @@ static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
 		__mptcp_subflow_connect(sk, &addrs[i], &remote);
 	spin_lock_bh(&msk->pm.lock);
 
+	/* be sure to echo exactly the received address */
+	if (reset_port)
+		remote.port = 0;
+
 add_addr_echo:
-	mptcp_pm_announce_addr(msk, &msk->pm.remote, true);
+	mptcp_pm_announce_addr(msk, &remote, true);
 	mptcp_pm_nl_addr_send_ack(msk);
 }
 
-- 
2.34.1

[PATCH mptcp-net v3 3/4] mptcp: add mibs counter for ignored incoming options

[Paolo Abeni]

The MPTCP in kernel path manager has some constraints on incoming
addresses annunces processing, so that in edge scenarios it can
end-up dropping (ignoring) some of such announces.

The above is not very limiting in practice since such scenarios are
very uncommon and MPTCP will recover due to ADD_ADDR retransmissions.

This patch adds a few MIBs counter to account for such drop events
to allow easier introspection of the critical scenarios.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/mib.c | 2 ++
 net/mptcp/mib.h | 2 ++
 net/mptcp/pm.c  | 8 ++++++--
 3 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/mib.c b/net/mptcp/mib.c
index c12251cb0d44..d544b5304596 100644
--- a/net/mptcp/mib.c
+++ b/net/mptcp/mib.c
@@ -36,12 +36,14 @@ static const struct snmp_mib mptcp_snmp_list[] = {
 	SNMP_MIB_ITEM("AddAddr", MPTCP_MIB_ADDADDR),
 	SNMP_MIB_ITEM("EchoAdd", MPTCP_MIB_ECHOADD),
 	SNMP_MIB_ITEM("PortAdd", MPTCP_MIB_PORTADD),
+	SNMP_MIB_ITEM("AddAddrDropped", MPTCP_MIB_ADDADDRDROP),
 	SNMP_MIB_ITEM("MPJoinPortSynRx", MPTCP_MIB_JOINPORTSYNRX),
 	SNMP_MIB_ITEM("MPJoinPortSynAckRx", MPTCP_MIB_JOINPORTSYNACKRX),
 	SNMP_MIB_ITEM("MPJoinPortAckRx", MPTCP_MIB_JOINPORTACKRX),
 	SNMP_MIB_ITEM("MismatchPortSynRx", MPTCP_MIB_MISMATCHPORTSYNRX),
 	SNMP_MIB_ITEM("MismatchPortAckRx", MPTCP_MIB_MISMATCHPORTACKRX),
 	SNMP_MIB_ITEM("RmAddr", MPTCP_MIB_RMADDR),
+	SNMP_MIB_ITEM("RmAddrDrop", MPTCP_MIB_RMADDRDROP),
 	SNMP_MIB_ITEM("RmSubflow", MPTCP_MIB_RMSUBFLOW),
 	SNMP_MIB_ITEM("MPPrioTx", MPTCP_MIB_MPPRIOTX),
 	SNMP_MIB_ITEM("MPPrioRx", MPTCP_MIB_MPPRIORX),
diff --git a/net/mptcp/mib.h b/net/mptcp/mib.h
index 7901f1338d15..785cf751e5c0 100644
--- a/net/mptcp/mib.h
+++ b/net/mptcp/mib.h
@@ -29,12 +29,14 @@ enum linux_mptcp_mib_field {
 	MPTCP_MIB_ADDADDR,		/* Received ADD_ADDR with echo-flag=0 */
 	MPTCP_MIB_ECHOADD,		/* Received ADD_ADDR with echo-flag=1 */
 	MPTCP_MIB_PORTADD,		/* Received ADD_ADDR with a port-number */
+	MPTCP_MIB_ADDADDRDROP,		/* Dropped incoming ADD_ADDR */
 	MPTCP_MIB_JOINPORTSYNRX,	/* Received a SYN MP_JOIN with a different port-number */
 	MPTCP_MIB_JOINPORTSYNACKRX,	/* Received a SYNACK MP_JOIN with a different port-number */
 	MPTCP_MIB_JOINPORTACKRX,	/* Received an ACK MP_JOIN with a different port-number */
 	MPTCP_MIB_MISMATCHPORTSYNRX,	/* Received a SYN MP_JOIN with a mismatched port-number */
 	MPTCP_MIB_MISMATCHPORTACKRX,	/* Received an ACK MP_JOIN with a mismatched port-number */
 	MPTCP_MIB_RMADDR,		/* Received RM_ADDR */
+	MPTCP_MIB_RMADDRDROP,		/* Dropped incoming RM_ADDR */
 	MPTCP_MIB_RMSUBFLOW,		/* Remove a subflow */
 	MPTCP_MIB_MPPRIOTX,		/* Transmit a MP_PRIO */
 	MPTCP_MIB_MPPRIORX,		/* Received a MP_PRIO */
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 1f8878cc29e3..c28dd5c21cbe 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -213,6 +213,8 @@ void mptcp_pm_add_addr_received(struct mptcp_sock *msk,
 		mptcp_pm_add_addr_send_ack(msk);
 	} else if (mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_RECEIVED)) {
 		pm->remote = *addr;
+	} else {
+		__MPTCP_INC_STATS(sock_net((struct sock*)msk), MPTCP_MIB_ADDADDRDROP);
 	}
 
 	spin_unlock_bh(&pm->lock);
@@ -253,8 +255,10 @@ void mptcp_pm_rm_addr_received(struct mptcp_sock *msk,
 		mptcp_event_addr_removed(msk, rm_list->ids[i]);
 
 	spin_lock_bh(&pm->lock);
-	mptcp_pm_schedule_work(msk, MPTCP_PM_RM_ADDR_RECEIVED);
-	pm->rm_list_rx = *rm_list;
+	if (mptcp_pm_schedule_work(msk, MPTCP_PM_RM_ADDR_RECEIVED))
+		pm->rm_list_rx = *rm_list;
+	else
+		__MPTCP_INC_STATS(sock_net((struct sock*)msk), MPTCP_MIB_RMADDRDROP);
 	spin_unlock_bh(&pm->lock);
 }
 
-- 
2.34.1

Re: [PATCH mptcp-net v3 3/4] mptcp: add mibs counter for ignored incoming options

[Matthieu Baerts]

Hi Paolo,

On 09/02/2022 10:47, Paolo Abeni wrote:
> The MPTCP in kernel path manager has some constraints on incoming
> addresses annunces processing, so that in edge scenarios it can
> end-up dropping (ignoring) some of such announces.
> 
> The above is not very limiting in practice since such scenarios are
> very uncommon and MPTCP will recover due to ADD_ADDR retransmissions.
> 
> This patch adds a few MIBs counter to account for such drop events
> to allow easier introspection of the critical scenarios.
> 
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
>  net/mptcp/mib.c | 2 ++
>  net/mptcp/mib.h | 2 ++
>  net/mptcp/pm.c  | 8 ++++++--
>  3 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/net/mptcp/mib.c b/net/mptcp/mib.c
> index c12251cb0d44..d544b5304596 100644
> --- a/net/mptcp/mib.c
> +++ b/net/mptcp/mib.c
> @@ -36,12 +36,14 @@ static const struct snmp_mib mptcp_snmp_list[] = {
>  	SNMP_MIB_ITEM("AddAddr", MPTCP_MIB_ADDADDR),
>  	SNMP_MIB_ITEM("EchoAdd", MPTCP_MIB_ECHOADD),
>  	SNMP_MIB_ITEM("PortAdd", MPTCP_MIB_PORTADD),
> +	SNMP_MIB_ITEM("AddAddrDropped", MPTCP_MIB_ADDADDRDROP),

Do you mind if I rename "AddAddrDropped" to "AddAddrDrop", similar to
the enum name and the title for the RmAddr?

>  	SNMP_MIB_ITEM("MPJoinPortSynRx", MPTCP_MIB_JOINPORTSYNRX),
>  	SNMP_MIB_ITEM("MPJoinPortSynAckRx", MPTCP_MIB_JOINPORTSYNACKRX),
>  	SNMP_MIB_ITEM("MPJoinPortAckRx", MPTCP_MIB_JOINPORTACKRX),
>  	SNMP_MIB_ITEM("MismatchPortSynRx", MPTCP_MIB_MISMATCHPORTSYNRX),
>  	SNMP_MIB_ITEM("MismatchPortAckRx", MPTCP_MIB_MISMATCHPORTACKRX),
>  	SNMP_MIB_ITEM("RmAddr", MPTCP_MIB_RMADDR),
> +	SNMP_MIB_ITEM("RmAddrDrop", MPTCP_MIB_RMADDRDROP),

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

[PATCH mptcp-net v3 4/4] selftests: mptcp: more robust signal race test

[Paolo Abeni]

The in kernel MPTCP PM implementation can process a single
incoming add address option at any given time. In the
mentioned test the server can surpas such limit. Let the
setup cope with that allowing a faster add_addr retransmission.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 90a6adc36490..66ac990415e6 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -915,11 +915,17 @@ chk_add_nr()
 	local mis_ack_nr=${8:-0}
 	local count
 	local dump_stats
+	local timeout
+
+	timeout=`ip netns exec $ns1 sysctl -n net.mptcp.add_addr_timeout`
 
 	printf "%-${nr_blank}s %s" " " "add"
-	count=`ip netns exec $ns2 nstat -as | grep MPTcpExtAddAddr | awk '{print $2}'`
+	count=`ip netns exec $ns2 nstat -as MPTcpExtAddAddr | grep MPTcpExtAddAddr | awk '{print $2}'`
 	[ -z "$count" ] && count=0
-	if [ "$count" != "$add_nr" ]; then
+
+	# if the test configured a short timeout tolerate greater then expected
+	# add addrs options, due to retransmissions
+	if [ "$count" != "$add_nr" ] && [ "$timeout" -gt 1 -o "$count" -lt "$add_nr" ]; then
 		echo "[fail] got $count ADD_ADDR[s] expected $add_nr"
 		ret=1
 		dump_stats=1
@@ -1311,6 +1317,7 @@ signal_address_tests()
 
 	# signal addresses race test
 	reset
+
 	pm_nl_set_limits $ns1 4 4
 	pm_nl_set_limits $ns2 4 4
 	pm_nl_add_endpoint $ns1 10.0.1.1 flags signal
@@ -1321,7 +1328,10 @@ 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
+
+	# the peer could possibly miss some addr notification, allow retransmission
+	ip netns exec $ns1 sysctl -q net.mptcp.add_addr_timeout=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 v3 0/4] mptcp: fix races in add_addr handling

[Matthieu Baerts]

Hi Paolo,

On 09/02/2022 10:47, Paolo Abeni wrote:
> the patches 1 && 2 fix actual races, even if the race addressed by
> 2/4 is not exposed by the self-tests, afaics.
> 
> patch 3 introduces some mibs to detect critical scenarios we can't
> fix without major rework
> 
> patch 4 updates the self-test to be graceful in above cases.

Thank you for the new version!

I suggest to apply it now to have more feedback from the CIs. We can
always end Squash-to patches later if needed.


> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/254
> 
> Note: I did not attach the above tag to a specific patch, as all
> of them will be needed to fix the issue

Do not hesitate to put it on the last one so the ticket will be
automatically closed and tracked when applying the patches on the
different trees :)
We can also put it on all of them or use 'Link' on the first ones and
'Closes' on the last one.

Anyway, I can also close it manually if I don't forget ;-)

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

Re: [PATCH mptcp-net v3 0/4] mptcp: fix races in add_addr handling

[Matthieu Baerts]

Hi Paolo,

On 09/02/2022 10:59, Matthieu Baerts wrote:
> Hi Paolo,
> 
> On 09/02/2022 10:47, Paolo Abeni wrote:
>> the patches 1 && 2 fix actual races, even if the race addressed by
>> 2/4 is not exposed by the self-tests, afaics.
>>
>> patch 3 introduces some mibs to detect critical scenarios we can't
>> fix without major rework
>>
>> patch 4 updates the self-test to be graceful in above cases.
> 
> Thank you for the new version!
> 
> I suggest to apply it now to have more feedback from the CIs. We can
> always end Squash-to patches later if needed.

Just applied in our tree (fixes for -net) with my RvB tags (+ Closes,
Reported/Tested and Fixes on the patch 4/4),
s/AddAddrDropped/AddAddrDrop/ for the MIB item without two small issues
reported by CheckPatch ("(foo*)" should be "(foo *)"):

- 83013c9c30e5: mptcp: fix race in overlapping signal events
- 6a9d5a9d5ec0: mptcp: fix race in incoming ADD_ADDR option processing
- 31a634cf1d69: mptcp: add mibs counter for ignored incoming options
- be58c0804397: selftests: mptcp: more robust signal race test
- 94af849fb4a6: tg: fix wrong merge resolution
- Results: 3484d41ff4f2..be63b701c373

Builds and tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20220209T132143
https://github.com/multipath-tcp/mptcp_net-next/actions/workflows/build-validation.yml?query=branch:export

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