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

[PATCH mptcp-net v2 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

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 v2 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 v2 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>
---
 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..4b6aa89d45a5 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;
 	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 v2 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

[PATCH mptcp-net v2 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 v2 2/4] mptcp: fix race in incoming ADD_ADDR option processing.

[Matthieu Baerts]

Hi Paolo,

On 08/02/2022 20:06, Paolo Abeni wrote:
> 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.

Thank you for looking at that!

It seems really helping for me: 500 iterations without issues! I guess
we can add:

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

(I guess we don't need Reported/Tested-by tags.)

Not related to that but do you think we could have a similar issue with
RM_ADDR (issues/225)?


But one important thing: this series introduces a regression apparently:
packetdrill ADD_ADDR tests are now failing because the port is missing
in the echo packet:

---------- 8< ----------
Our CI did some validations and here is its report:

- KVM Validation: normal:
  - Unstable: 2 failed test(s): packetdrill_add_addr selftest_mptcp_join 🔴:
  - Task: https://cirrus-ci.com/task/5025097999187968
  - Summary:
https://api.cirrus-ci.com/v1/artifact/task/5025097999187968/summary/summary.txt

- KVM Validation: debug:
  - Unstable: 1 failed test(s): packetdrill_add_addr 🔴:
  - Task: https://cirrus-ci.com/task/4765935646015488
  - Summary:
https://api.cirrus-ci.com/v1/artifact/task/4765935646015488/summary/summary.txt

Initiator: Patchew Applier
Commits:
https://github.com/multipath-tcp/mptcp_net-next/commits/34bad3e35cbc
---------- 8< ----------

This seems to be due to this patch 2/4.

> 
> Fixes: f7efc7771eac ("mptcp: drop argument port from mptcp_pm_announce_addr")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
>  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..4b6aa89d45a5 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;

I'm very surprise the compiler doesn't complain about this line because
reset_port can be used later while not having been set before. Is it
normal with 'bool'?


Setting it to false by default fixes the issue with packetdrill on my side:

---------- 8< ----------
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 4b6aa89d45a5..98b485406afa 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -660,7 +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;
+       bool reset_port = false;
        int i, nr;

        add_addr_accept_max = mptcp_pm_get_add_addr_accept_max(msk);
---------- 8< ----------

(I can fix the issue when applying the patch if it is easier)

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

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

[Mat Martineau]

On Tue, 8 Feb 2022, 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>
> ---
> 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))

There is a helper for this: mptcp_pm_should_add_signal_addr()
(which also uses READ_ONCE())


> +			return;
> +
> 		if (local) {
> 			if (mptcp_pm_alloc_anno_list(msk, local)) {
> 				__clear_bit(local->addr.id, msk->pm.id_avail_bitmap);
> -- 
> 2.34.1

--
Mat Martineau
Intel

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

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 3425 bytes --]

On Tue, 8 Feb 2022, Matthieu Baerts wrote:

> Hi Paolo,
>
> On 08/02/2022 20:06, Paolo Abeni wrote:
>> 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.
>
> Thank you for looking at that!
>
> It seems really helping for me: 500 iterations without issues! I guess
> we can add:
>
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/254
>
> (I guess we don't need Reported/Tested-by tags.)
>
> Not related to that but do you think we could have a similar issue with
> RM_ADDR (issues/225)?
>
>
> But one important thing: this series introduces a regression apparently:
> packetdrill ADD_ADDR tests are now failing because the port is missing
> in the echo packet:
>
> ---------- 8< ----------
> Our CI did some validations and here is its report:
>
> - KVM Validation: normal:
>  - Unstable: 2 failed test(s): packetdrill_add_addr selftest_mptcp_join 🔴:
>  - Task: https://cirrus-ci.com/task/5025097999187968
>  - Summary:
> https://api.cirrus-ci.com/v1/artifact/task/5025097999187968/summary/summary.txt
>
> - KVM Validation: debug:
>  - Unstable: 1 failed test(s): packetdrill_add_addr 🔴:
>  - Task: https://cirrus-ci.com/task/4765935646015488
>  - Summary:
> https://api.cirrus-ci.com/v1/artifact/task/4765935646015488/summary/summary.txt
>
> Initiator: Patchew Applier
> Commits:
> https://github.com/multipath-tcp/mptcp_net-next/commits/34bad3e35cbc
> ---------- 8< ----------
>
> This seems to be due to this patch 2/4.
>
>>
>> Fixes: f7efc7771eac ("mptcp: drop argument port from mptcp_pm_announce_addr")
>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>> ---
>>  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..4b6aa89d45a5 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;
>
> I'm very surprise the compiler doesn't complain about this line because
> reset_port can be used later while not having been set before. Is it
> normal with 'bool'?
>
>
> Setting it to false by default fixes the issue with packetdrill on my side:
>
> ---------- 8< ----------
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index 4b6aa89d45a5..98b485406afa 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -660,7 +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;
> +       bool reset_port = false;
>        int i, nr;
>
>        add_addr_accept_max = mptcp_pm_get_add_addr_accept_max(msk);
> ---------- 8< ----------
>
> (I can fix the issue when applying the patch if it is easier)

With Matthieu's reset_port fix, it's working well for me too.


--
Mat Martineau
Intel

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

[Paolo Abeni]

On Tue, 2022-02-08 at 16:54 -0800, Mat Martineau wrote:
> On Tue, 8 Feb 2022, Matthieu Baerts wrote:
> 
> > Hi Paolo,
> > 
> > On 08/02/2022 20:06, Paolo Abeni wrote:
> > > 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.
> > 
> > Thank you for looking at that!
> > 
> > It seems really helping for me: 500 iterations without issues! I guess
> > we can add:
> > 
> > Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/254
> > 
> > (I guess we don't need Reported/Tested-by tags.)
> > 
> > Not related to that but do you think we could have a similar issue with
> > RM_ADDR (issues/225)?
> > 
> > 
> > But one important thing: this series introduces a regression apparently:
> > packetdrill ADD_ADDR tests are now failing because the port is missing
> > in the echo packet:
> > 
> > ---------- 8< ----------
> > Our CI did some validations and here is its report:
> > 
> > - KVM Validation: normal:
> >  - Unstable: 2 failed test(s): packetdrill_add_addr selftest_mptcp_join 🔴:
> >  - Task: https://cirrus-ci.com/task/5025097999187968
> >  - Summary:
> > https://api.cirrus-ci.com/v1/artifact/task/5025097999187968/summary/summary.txt
> > 
> > - KVM Validation: debug:
> >  - Unstable: 1 failed test(s): packetdrill_add_addr 🔴:
> >  - Task: https://cirrus-ci.com/task/4765935646015488
> >  - Summary:
> > https://api.cirrus-ci.com/v1/artifact/task/4765935646015488/summary/summary.txt
> > 
> > Initiator: Patchew Applier
> > Commits:
> > https://github.com/multipath-tcp/mptcp_net-next/commits/34bad3e35cbc
> > ---------- 8< ----------
> > 
> > This seems to be due to this patch 2/4.
> > 
> > > 
> > > Fixes: f7efc7771eac ("mptcp: drop argument port from mptcp_pm_announce_addr")
> > > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > > ---
> > >  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..4b6aa89d45a5 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;
> > 
> > I'm very surprise the compiler doesn't complain about this line because
> > reset_port can be used later while not having been set before. Is it
> > normal with 'bool'?
> > 
> > 
> > Setting it to false by default fixes the issue with packetdrill on my side:
> > 
> > ---------- 8< ----------
> > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> > index 4b6aa89d45a5..98b485406afa 100644
> > --- a/net/mptcp/pm_netlink.c
> > +++ b/net/mptcp/pm_netlink.c
> > @@ -660,7 +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;
> > +       bool reset_port = false;
> >        int i, nr;
> > 
> >        add_addr_accept_max = mptcp_pm_get_add_addr_accept_max(msk);
> > ---------- 8< ----------
> > 
> > (I can fix the issue when applying the patch if it is easier)
> 
> With Matthieu's reset_port fix, it's working well for me too.

Whoops, not sure how I forgot the proper initializer, and I'm also
surprised the compiler did not warn (the latter may provide some excuse
for the first ;)

Do you prerfer a formal repost or can you mangle the patch in
patchwork?

Thanks!

Paolo

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

[Paolo Abeni]

On Tue, 2022-02-08 at 16:54 -0800, Mat Martineau wrote:
> On Tue, 8 Feb 2022, 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>
> > ---
> > 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))
> 
> There is a helper for this: mptcp_pm_should_add_signal_addr()
> (which also uses READ_ONCE())

I chose to not use it, as the condition I'm really checking is: "is the
PM already processing a signal addr?" which incidentally boils down to
the same code, but the helper name would be misleading IMHO.

I can adapt if there are strong opinion WRT the above. Please let me
know.

Thanks!

Paolo

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

[Matthieu Baerts]

Hi Paolo, Mat,

On 09/02/2022 09:54, Paolo Abeni wrote:
> On Tue, 2022-02-08 at 16:54 -0800, Mat Martineau wrote:
>> On Tue, 8 Feb 2022, 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>
>>> ---
>>> 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))
>>
>> There is a helper for this: mptcp_pm_should_add_signal_addr()
>> (which also uses READ_ONCE())
> 
> I chose to not use it, as the condition I'm really checking is: "is the
> PM already processing a signal addr?" which incidentally boils down to
> the same code, but the helper name would be misleading IMHO.

If it helps, we can also have a new helper:

    mptcp_pm_is_processing_signal_addr()

simply calling "mptcp_pm_should_add_signal_addr()", no? :)

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

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

[Matthieu Baerts]

Hi Paolo,

On 09/02/2022 09:49, Paolo Abeni wrote:
> On Tue, 2022-02-08 at 16:54 -0800, Mat Martineau wrote:
>> On Tue, 8 Feb 2022, Matthieu Baerts wrote:
>>> On 08/02/2022 20:06, Paolo Abeni wrote:

>>>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
>>>> index fb8d4bfcfbd6..4b6aa89d45a5 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;
>>>
>>> I'm very surprise the compiler doesn't complain about this line because
>>> reset_port can be used later while not having been set before. Is it
>>> normal with 'bool'?
>>>
>>>
>>> Setting it to false by default fixes the issue with packetdrill on my side:
>>>
>>> ---------- 8< ----------
>>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
>>> index 4b6aa89d45a5..98b485406afa 100644
>>> --- a/net/mptcp/pm_netlink.c
>>> +++ b/net/mptcp/pm_netlink.c
>>> @@ -660,7 +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;
>>> +       bool reset_port = false;
>>>        int i, nr;
>>>
>>>        add_addr_accept_max = mptcp_pm_get_add_addr_accept_max(msk);
>>> ---------- 8< ----------
>>>
>>> (I can fix the issue when applying the patch if it is easier)
>>
>> With Matthieu's reset_port fix, it's working well for me too.
> 
> Whoops, not sure how I forgot the proper initializer, and I'm also
> surprised the compiler did not warn (the latter may provide some excuse
> for the first ;)

The compiler not warning us about that bothers me :)

> Do you prerfer a formal repost or can you mangle the patch in
> patchwork?

I can do the modification just before applying the patch. I will wait
for a reply on the last question on patch 1/4 before applying it ;)

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