netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net 00/14] mptcp: pm: fix IDs not being reusable
@ 2024-08-19 19:45 Matthieu Baerts (NGI0)
  2024-08-19 19:45 ` [PATCH net 01/14] mptcp: pm: re-using ID of unused removed ADD_ADDR Matthieu Baerts (NGI0)
                   ` (14 more replies)
  0 siblings, 15 replies; 16+ messages in thread
From: Matthieu Baerts (NGI0) @ 2024-08-19 19:45 UTC (permalink / raw)
  To: mptcp, Mat Martineau, Geliang Tang, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Shuah Khan
  Cc: netdev, linux-kernel, linux-kselftest, Matthieu Baerts (NGI0),
	stable

Here are more fixes for the MPTCP in-kernel path-manager. In this
series, the fixes are around the endpoint IDs not being reusable for
on-going connections when re-creating endpoints with previously used IDs.

- Patch 1 fixes this case for endpoints being used to send ADD_ADDR.
  Patch 2 validates this fix. The issue is present since v5.10.

- Patch 3 fixes this case for endpoints being used to establish new
  subflows. Patch 4 validates this fix. The issue is present since v5.10.

- Patch 5 fixes this case when all endpoints are flushed. Patch 6
  validates this fix. The issue is present since v5.13.

- Patch 7 removes a helper that is confusing, and introduced in v5.10.
  It helps simplifying the next patches.

- Patch 8 makes sure a 'subflow' counter is only decremented when
  removing a 'subflow' endpoint. Can be backported up to v5.13.

- Patch 9 is similar, but for a 'signal' counter. Can be backported up
  to v5.10.

- Patch 10 checks the last max accepted ADD_ADDR limit before accepting
  new ADD_ADDR. For v5.10 as well.

- Patch 11 removes a wrong restriction for the userspace PM, added
  during a refactoring in v6.5.

- Patch 12 makes sure the fullmesh mode sets the ID 0 when a new subflow
  using the source address of the initial subflow is created. Patch 13
  covers this case. This issue is present since v5.15.

- Patch 14 avoid possible UaF when selecting an address from the
  endpoints list.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Matthieu Baerts (NGI0) (14):
      mptcp: pm: re-using ID of unused removed ADD_ADDR
      selftests: mptcp: join: check re-using ID of unused ADD_ADDR
      mptcp: pm: re-using ID of unused removed subflows
      selftests: mptcp: join: check re-using ID of closed subflow
      mptcp: pm: re-using ID of unused flushed subflows
      selftests: mptcp: join: test for flush/re-add endpoints
      mptcp: pm: remove mptcp_pm_remove_subflow()
      mptcp: pm: only mark 'subflow' endp as available
      mptcp: pm: only decrement add_addr_accepted for MPJ req
      mptcp: pm: check add_addr_accept_max before accepting new ADD_ADDR
      mptcp: pm: only in-kernel cannot have entries with ID 0
      mptcp: pm: fullmesh: select the right ID later
      selftests: mptcp: join: validate fullmesh endp on 1st sf
      mptcp: pm: avoid possible UaF when selecting endp

 net/mptcp/pm.c                                  |  13 ---
 net/mptcp/pm_netlink.c                          | 142 ++++++++++++++++--------
 net/mptcp/protocol.h                            |   3 -
 tools/testing/selftests/net/mptcp/mptcp_join.sh |  76 +++++++++++--
 4 files changed, 160 insertions(+), 74 deletions(-)
---
base-commit: 565d121b69980637f040eb4d84289869cdaabedf
change-id: 20240819-net-mptcp-pm-reusing-id-eb08827b7be6

Best regards,
-- 
Matthieu Baerts (NGI0) <matttbe@kernel.org>


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH net 01/14] mptcp: pm: re-using ID of unused removed ADD_ADDR
  2024-08-19 19:45 [PATCH net 00/14] mptcp: pm: fix IDs not being reusable Matthieu Baerts (NGI0)
@ 2024-08-19 19:45 ` Matthieu Baerts (NGI0)
  2024-08-19 19:45 ` [PATCH net 02/14] selftests: mptcp: join: check re-using ID of unused ADD_ADDR Matthieu Baerts (NGI0)
                   ` (13 subsequent siblings)
  14 siblings, 0 replies; 16+ messages in thread
From: Matthieu Baerts (NGI0) @ 2024-08-19 19:45 UTC (permalink / raw)
  To: mptcp, Mat Martineau, Geliang Tang, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Shuah Khan
  Cc: netdev, linux-kernel, linux-kselftest, Matthieu Baerts (NGI0),
	stable

If no subflow is attached to the 'signal' endpoint that is being
removed, the addr ID will not be marked as available again.

Mark the linked ID as available when removing the address entry from the
list to cover this case.

Fixes: b6c08380860b ("mptcp: remove addr and subflow in PM netlink")
Cc: stable@vger.kernel.org
Reviewed-by: Mat Martineau <martineau@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/pm_netlink.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 4cae2aa7be5c..26f0329e16bb 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1431,7 +1431,10 @@ static bool mptcp_pm_remove_anno_addr(struct mptcp_sock *msk,
 	ret = remove_anno_list_by_saddr(msk, addr);
 	if (ret || force) {
 		spin_lock_bh(&msk->pm.lock);
-		msk->pm.add_addr_signaled -= ret;
+		if (ret) {
+			__set_bit(addr->id, msk->pm.id_avail_bitmap);
+			msk->pm.add_addr_signaled--;
+		}
 		mptcp_pm_remove_addr(msk, &list);
 		spin_unlock_bh(&msk->pm.lock);
 	}

-- 
2.45.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH net 02/14] selftests: mptcp: join: check re-using ID of unused ADD_ADDR
  2024-08-19 19:45 [PATCH net 00/14] mptcp: pm: fix IDs not being reusable Matthieu Baerts (NGI0)
  2024-08-19 19:45 ` [PATCH net 01/14] mptcp: pm: re-using ID of unused removed ADD_ADDR Matthieu Baerts (NGI0)
@ 2024-08-19 19:45 ` Matthieu Baerts (NGI0)
  2024-08-19 19:45 ` [PATCH net 03/14] mptcp: pm: re-using ID of unused removed subflows Matthieu Baerts (NGI0)
                   ` (12 subsequent siblings)
  14 siblings, 0 replies; 16+ messages in thread
From: Matthieu Baerts (NGI0) @ 2024-08-19 19:45 UTC (permalink / raw)
  To: mptcp, Mat Martineau, Geliang Tang, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Shuah Khan
  Cc: netdev, linux-kernel, linux-kselftest, Matthieu Baerts (NGI0),
	stable

This test extends "delete re-add signal" to validate the previous
commit. An extra address is announced by the server, but this address
cannot be used by the client. The result is that no subflow will be
established to this address.

Later, the server will delete this extra endpoint, and set a new one,
with a valid address, but re-using the same ID. Before the previous
commit, the server would not have been able to announce this new
address.

While at it, extra checks have been added to validate the expected
numbers of MPJ, ADD_ADDR and RM_ADDR.

The 'Fixes' tag here below is the same as the one from the previous
commit: this patch here is not fixing anything wrong in the selftests,
but it validates the previous fix for an issue introduced by this commit
ID.

Fixes: b6c08380860b ("mptcp: remove addr and subflow in PM netlink")
Cc: stable@vger.kernel.org
Reviewed-by: Mat Martineau <martineau@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 9ea6d698e9d3..25077ccf31d2 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -3601,9 +3601,11 @@ endpoint_tests()
 	# remove and re-add
 	if reset "delete re-add signal" &&
 	   mptcp_lib_kallsyms_has "subflow_rebuild_header$"; then
-		pm_nl_set_limits $ns1 1 1
-		pm_nl_set_limits $ns2 1 1
+		pm_nl_set_limits $ns1 0 2
+		pm_nl_set_limits $ns2 2 2
 		pm_nl_add_endpoint $ns1 10.0.2.1 id 1 flags signal
+		# broadcast IP: no packet for this address will be received on ns1
+		pm_nl_add_endpoint $ns1 224.0.0.1 id 2 flags signal
 		test_linkfail=4 speed=20 \
 			run_tests $ns1 $ns2 10.0.1.1 &
 		local tests_pid=$!
@@ -3615,15 +3617,21 @@ endpoint_tests()
 		chk_mptcp_info subflows 1 subflows 1
 
 		pm_nl_del_endpoint $ns1 1 10.0.2.1
+		pm_nl_del_endpoint $ns1 2 224.0.0.1
 		sleep 0.5
 		chk_subflow_nr "after delete" 1
 		chk_mptcp_info subflows 0 subflows 0
 
-		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal
+		pm_nl_add_endpoint $ns1 10.0.2.1 id 1 flags signal
+		pm_nl_add_endpoint $ns1 10.0.3.1 id 2 flags signal
 		wait_mpj $ns2
-		chk_subflow_nr "after re-add" 2
-		chk_mptcp_info subflows 1 subflows 1
+		chk_subflow_nr "after re-add" 3
+		chk_mptcp_info subflows 2 subflows 2
 		mptcp_lib_kill_wait $tests_pid
+
+		chk_join_nr 3 3 3
+		chk_add_nr 4 4
+		chk_rm_nr 2 1 invert
 	fi
 
 }

-- 
2.45.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH net 03/14] mptcp: pm: re-using ID of unused removed subflows
  2024-08-19 19:45 [PATCH net 00/14] mptcp: pm: fix IDs not being reusable Matthieu Baerts (NGI0)
  2024-08-19 19:45 ` [PATCH net 01/14] mptcp: pm: re-using ID of unused removed ADD_ADDR Matthieu Baerts (NGI0)
  2024-08-19 19:45 ` [PATCH net 02/14] selftests: mptcp: join: check re-using ID of unused ADD_ADDR Matthieu Baerts (NGI0)
@ 2024-08-19 19:45 ` Matthieu Baerts (NGI0)
  2024-08-19 19:45 ` [PATCH net 04/14] selftests: mptcp: join: check re-using ID of closed subflow Matthieu Baerts (NGI0)
                   ` (11 subsequent siblings)
  14 siblings, 0 replies; 16+ messages in thread
From: Matthieu Baerts (NGI0) @ 2024-08-19 19:45 UTC (permalink / raw)
  To: mptcp, Mat Martineau, Geliang Tang, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Shuah Khan
  Cc: netdev, linux-kernel, linux-kselftest, Matthieu Baerts (NGI0),
	stable

If no subflow is attached to the 'subflow' endpoint that is being
removed, the addr ID will not be marked as available again.

Mark the linked ID as available when removing the 'subflow' endpoint if
no subflow is attached to it.

While at it, the local_addr_used counter is decremented if the ID was
marked as being used to reflect the reality, but also to allow adding
new endpoints after that.

Fixes: b6c08380860b ("mptcp: remove addr and subflow in PM netlink")
Cc: stable@vger.kernel.org
Reviewed-by: Mat Martineau <martineau@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/pm_netlink.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 26f0329e16bb..8b232a210a06 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1469,8 +1469,17 @@ static int mptcp_nl_remove_subflow_and_signal_addr(struct net *net,
 		remove_subflow = lookup_subflow_by_saddr(&msk->conn_list, addr);
 		mptcp_pm_remove_anno_addr(msk, addr, remove_subflow &&
 					  !(entry->flags & MPTCP_PM_ADDR_FLAG_IMPLICIT));
-		if (remove_subflow)
+
+		if (remove_subflow) {
 			mptcp_pm_remove_subflow(msk, &list);
+		} else if (entry->flags & MPTCP_PM_ADDR_FLAG_SUBFLOW) {
+			/* If the subflow has been used, but now closed */
+			spin_lock_bh(&msk->pm.lock);
+			if (!__test_and_set_bit(entry->addr.id, msk->pm.id_avail_bitmap))
+				msk->pm.local_addr_used--;
+			spin_unlock_bh(&msk->pm.lock);
+		}
+
 		release_sock(sk);
 
 next:

-- 
2.45.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH net 04/14] selftests: mptcp: join: check re-using ID of closed subflow
  2024-08-19 19:45 [PATCH net 00/14] mptcp: pm: fix IDs not being reusable Matthieu Baerts (NGI0)
                   ` (2 preceding siblings ...)
  2024-08-19 19:45 ` [PATCH net 03/14] mptcp: pm: re-using ID of unused removed subflows Matthieu Baerts (NGI0)
@ 2024-08-19 19:45 ` Matthieu Baerts (NGI0)
  2024-08-19 19:45 ` [PATCH net 05/14] mptcp: pm: re-using ID of unused flushed subflows Matthieu Baerts (NGI0)
                   ` (10 subsequent siblings)
  14 siblings, 0 replies; 16+ messages in thread
From: Matthieu Baerts (NGI0) @ 2024-08-19 19:45 UTC (permalink / raw)
  To: mptcp, Mat Martineau, Geliang Tang, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Shuah Khan
  Cc: netdev, linux-kernel, linux-kselftest, Matthieu Baerts (NGI0),
	stable

This test extends "delete and re-add" to validate the previous commit. A
new 'subflow' endpoint is added, but the subflow request will be
rejected. The result is that no subflow will be established from this
address.

Later, the endpoint is removed and re-added after having cleared the
firewall rule. Before the previous commit, the client would not have
been able to create this new subflow.

While at it, extra checks have been added to validate the expected
numbers of MPJ and RM_ADDR.

The 'Fixes' tag here below is the same as the one from the previous
commit: this patch here is not fixing anything wrong in the selftests,
but it validates the previous fix for an issue introduced by this commit
ID.

Fixes: b6c08380860b ("mptcp: remove addr and subflow in PM netlink")
Cc: stable@vger.kernel.org
Reviewed-by: Mat Martineau <martineau@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 27 ++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 25077ccf31d2..fbb0174145ad 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -436,9 +436,10 @@ reset_with_tcp_filter()
 	local ns="${!1}"
 	local src="${2}"
 	local target="${3}"
+	local chain="${4:-INPUT}"
 
 	if ! ip netns exec "${ns}" ${iptables} \
-			-A INPUT \
+			-A "${chain}" \
 			-s "${src}" \
 			-p tcp \
 			-j "${target}"; then
@@ -3571,10 +3572,10 @@ endpoint_tests()
 		mptcp_lib_kill_wait $tests_pid
 	fi
 
-	if reset "delete and re-add" &&
+	if reset_with_tcp_filter "delete and re-add" ns2 10.0.3.2 REJECT OUTPUT &&
 	   mptcp_lib_kallsyms_has "subflow_rebuild_header$"; then
-		pm_nl_set_limits $ns1 1 1
-		pm_nl_set_limits $ns2 1 1
+		pm_nl_set_limits $ns1 0 2
+		pm_nl_set_limits $ns2 0 2
 		pm_nl_add_endpoint $ns2 10.0.2.2 id 2 dev ns2eth2 flags subflow
 		test_linkfail=4 speed=20 \
 			run_tests $ns1 $ns2 10.0.1.1 &
@@ -3591,11 +3592,27 @@ endpoint_tests()
 		chk_subflow_nr "after delete" 1
 		chk_mptcp_info subflows 0 subflows 0
 
-		pm_nl_add_endpoint $ns2 10.0.2.2 dev ns2eth2 flags subflow
+		pm_nl_add_endpoint $ns2 10.0.2.2 id 2 dev ns2eth2 flags subflow
 		wait_mpj $ns2
 		chk_subflow_nr "after re-add" 2
 		chk_mptcp_info subflows 1 subflows 1
+
+		pm_nl_add_endpoint $ns2 10.0.3.2 id 3 flags subflow
+		wait_attempt_fail $ns2
+		chk_subflow_nr "after new reject" 2
+		chk_mptcp_info subflows 1 subflows 1
+
+		ip netns exec "${ns2}" ${iptables} -D OUTPUT -s "10.0.3.2" -p tcp -j REJECT
+		pm_nl_del_endpoint $ns2 3 10.0.3.2
+		pm_nl_add_endpoint $ns2 10.0.3.2 id 3 flags subflow
+		wait_mpj $ns2
+		chk_subflow_nr "after no reject" 3
+		chk_mptcp_info subflows 2 subflows 2
+
 		mptcp_lib_kill_wait $tests_pid
+
+		chk_join_nr 3 3 3
+		chk_rm_nr 1 1
 	fi
 
 	# remove and re-add

-- 
2.45.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH net 05/14] mptcp: pm: re-using ID of unused flushed subflows
  2024-08-19 19:45 [PATCH net 00/14] mptcp: pm: fix IDs not being reusable Matthieu Baerts (NGI0)
                   ` (3 preceding siblings ...)
  2024-08-19 19:45 ` [PATCH net 04/14] selftests: mptcp: join: check re-using ID of closed subflow Matthieu Baerts (NGI0)
@ 2024-08-19 19:45 ` Matthieu Baerts (NGI0)
  2024-08-19 19:45 ` [PATCH net 06/14] selftests: mptcp: join: test for flush/re-add endpoints Matthieu Baerts (NGI0)
                   ` (9 subsequent siblings)
  14 siblings, 0 replies; 16+ messages in thread
From: Matthieu Baerts (NGI0) @ 2024-08-19 19:45 UTC (permalink / raw)
  To: mptcp, Mat Martineau, Geliang Tang, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Shuah Khan
  Cc: netdev, linux-kernel, linux-kselftest, Matthieu Baerts (NGI0),
	stable

If no subflows are attached to the 'subflow' endpoints that are being
flushed, the corresponding addr IDs will not be marked as available
again.

Mark all ID as being available when flushing all the 'subflow'
endpoints, and reset local_addr_used counter to cover these cases.

Note that mptcp_pm_remove_addrs_and_subflows() helper is only called for
flushing operations, not to remove a specific set of addresses and
subflows.

Fixes: 06faa2271034 ("mptcp: remove multi addresses and subflows in PM")
Cc: stable@vger.kernel.org
Reviewed-by: Mat Martineau <martineau@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/pm_netlink.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 8b232a210a06..2c26696b820e 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1623,8 +1623,15 @@ static void mptcp_pm_remove_addrs_and_subflows(struct mptcp_sock *msk,
 		mptcp_pm_remove_addr(msk, &alist);
 		spin_unlock_bh(&msk->pm.lock);
 	}
+
 	if (slist.nr)
 		mptcp_pm_remove_subflow(msk, &slist);
+
+	/* Reset counters: maybe some subflows have been removed before */
+	spin_lock_bh(&msk->pm.lock);
+	bitmap_fill(msk->pm.id_avail_bitmap, MPTCP_PM_MAX_ADDR_ID + 1);
+	msk->pm.local_addr_used = 0;
+	spin_unlock_bh(&msk->pm.lock);
 }
 
 static void mptcp_nl_remove_addrs_list(struct net *net,

-- 
2.45.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH net 06/14] selftests: mptcp: join: test for flush/re-add endpoints
  2024-08-19 19:45 [PATCH net 00/14] mptcp: pm: fix IDs not being reusable Matthieu Baerts (NGI0)
                   ` (4 preceding siblings ...)
  2024-08-19 19:45 ` [PATCH net 05/14] mptcp: pm: re-using ID of unused flushed subflows Matthieu Baerts (NGI0)
@ 2024-08-19 19:45 ` Matthieu Baerts (NGI0)
  2024-08-19 19:45 ` [PATCH net 07/14] mptcp: pm: remove mptcp_pm_remove_subflow() Matthieu Baerts (NGI0)
                   ` (8 subsequent siblings)
  14 siblings, 0 replies; 16+ messages in thread
From: Matthieu Baerts (NGI0) @ 2024-08-19 19:45 UTC (permalink / raw)
  To: mptcp, Mat Martineau, Geliang Tang, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Shuah Khan
  Cc: netdev, linux-kernel, linux-kselftest, Matthieu Baerts (NGI0),
	stable

After having flushed endpoints that didn't cause the creation of new
subflows, it is important to check endpoints can be re-created, re-using
previously used IDs.

Before the previous commit, the client would not have been able to
re-create the subflow that was previously rejected.

The 'Fixes' tag here below is the same as the one from the previous
commit: this patch here is not fixing anything wrong in the selftests,
but it validates the previous fix for an issue introduced by this commit
ID.

Fixes: 06faa2271034 ("mptcp: remove multi addresses and subflows in PM")
Cc: stable@vger.kernel.org
Reviewed-by: Mat Martineau <martineau@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 30 +++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index fbb0174145ad..f609c02c6123 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -3651,6 +3651,36 @@ endpoint_tests()
 		chk_rm_nr 2 1 invert
 	fi
 
+	# flush and re-add
+	if reset_with_tcp_filter "flush re-add" ns2 10.0.3.2 REJECT OUTPUT &&
+	   mptcp_lib_kallsyms_has "subflow_rebuild_header$"; then
+		pm_nl_set_limits $ns1 0 2
+		pm_nl_set_limits $ns2 1 2
+		# broadcast IP: no packet for this address will be received on ns1
+		pm_nl_add_endpoint $ns1 224.0.0.1 id 2 flags signal
+		pm_nl_add_endpoint $ns2 10.0.3.2 id 3 flags subflow
+		test_linkfail=4 speed=20 \
+			run_tests $ns1 $ns2 10.0.1.1 &
+		local tests_pid=$!
+
+		wait_attempt_fail $ns2
+		chk_subflow_nr "before flush" 1
+		chk_mptcp_info subflows 0 subflows 0
+
+		pm_nl_flush_endpoint $ns2
+		pm_nl_flush_endpoint $ns1
+		wait_rm_addr $ns2 0
+		ip netns exec "${ns2}" ${iptables} -D OUTPUT -s "10.0.3.2" -p tcp -j REJECT
+		pm_nl_add_endpoint $ns2 10.0.3.2 id 3 flags subflow
+		wait_mpj $ns2
+		pm_nl_add_endpoint $ns1 10.0.3.1 id 2 flags signal
+		wait_mpj $ns2
+		mptcp_lib_kill_wait $tests_pid
+
+		chk_join_nr 2 2 2
+		chk_add_nr 2 2
+		chk_rm_nr 1 0 invert
+	fi
 }
 
 # [$1: error message]

-- 
2.45.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH net 07/14] mptcp: pm: remove mptcp_pm_remove_subflow()
  2024-08-19 19:45 [PATCH net 00/14] mptcp: pm: fix IDs not being reusable Matthieu Baerts (NGI0)
                   ` (5 preceding siblings ...)
  2024-08-19 19:45 ` [PATCH net 06/14] selftests: mptcp: join: test for flush/re-add endpoints Matthieu Baerts (NGI0)
@ 2024-08-19 19:45 ` Matthieu Baerts (NGI0)
  2024-08-19 19:45 ` [PATCH net 08/14] mptcp: pm: only mark 'subflow' endp as available Matthieu Baerts (NGI0)
                   ` (7 subsequent siblings)
  14 siblings, 0 replies; 16+ messages in thread
From: Matthieu Baerts (NGI0) @ 2024-08-19 19:45 UTC (permalink / raw)
  To: mptcp, Mat Martineau, Geliang Tang, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Shuah Khan
  Cc: netdev, linux-kernel, linux-kselftest, Matthieu Baerts (NGI0),
	stable

This helper is confusing. It is in pm.c, but it is specific to the
in-kernel PM and it cannot be used by the userspace one. Also, it simply
calls one in-kernel specific function with the PM lock, while the
similar mptcp_pm_remove_addr() helper requires the PM lock.

What's left is the pr_debug(), which is not that useful, because a
similar one is present in the only function called by this helper:

  mptcp_pm_nl_rm_subflow_received()

After these modifications, this helper can be marked as 'static', and
the lock can be taken only once in mptcp_pm_flush_addrs_and_subflows().

Note that it is not a bug fix, but it will help backporting the
following commits.

Fixes: 0ee4261a3681 ("mptcp: implement mptcp_pm_remove_subflow")
Cc: stable@vger.kernel.org
Reviewed-by: Mat Martineau <martineau@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/pm.c         | 10 ----------
 net/mptcp/pm_netlink.c | 16 +++++++---------
 net/mptcp/protocol.h   |  3 ---
 3 files changed, 7 insertions(+), 22 deletions(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 23bb89c94e90..925123e99889 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -60,16 +60,6 @@ int mptcp_pm_remove_addr(struct mptcp_sock *msk, const struct mptcp_rm_list *rm_
 	return 0;
 }
 
-int mptcp_pm_remove_subflow(struct mptcp_sock *msk, const struct mptcp_rm_list *rm_list)
-{
-	pr_debug("msk=%p, rm_list_nr=%d", msk, rm_list->nr);
-
-	spin_lock_bh(&msk->pm.lock);
-	mptcp_pm_nl_rm_subflow_received(msk, rm_list);
-	spin_unlock_bh(&msk->pm.lock);
-	return 0;
-}
-
 /* path manager event handlers */
 
 void mptcp_pm_new_connection(struct mptcp_sock *msk, const struct sock *ssk, int server_side)
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 2c26696b820e..44fc1c5959ac 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -857,8 +857,8 @@ static void mptcp_pm_nl_rm_addr_received(struct mptcp_sock *msk)
 	mptcp_pm_nl_rm_addr_or_subflow(msk, &msk->pm.rm_list_rx, MPTCP_MIB_RMADDR);
 }
 
-void mptcp_pm_nl_rm_subflow_received(struct mptcp_sock *msk,
-				     const struct mptcp_rm_list *rm_list)
+static void mptcp_pm_nl_rm_subflow_received(struct mptcp_sock *msk,
+					    const struct mptcp_rm_list *rm_list)
 {
 	mptcp_pm_nl_rm_addr_or_subflow(msk, rm_list, MPTCP_MIB_RMSUBFLOW);
 }
@@ -1471,7 +1471,9 @@ static int mptcp_nl_remove_subflow_and_signal_addr(struct net *net,
 					  !(entry->flags & MPTCP_PM_ADDR_FLAG_IMPLICIT));
 
 		if (remove_subflow) {
-			mptcp_pm_remove_subflow(msk, &list);
+			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 */
 			spin_lock_bh(&msk->pm.lock);
@@ -1617,18 +1619,14 @@ static void mptcp_pm_remove_addrs_and_subflows(struct mptcp_sock *msk,
 			alist.ids[alist.nr++] = entry->addr.id;
 	}
 
+	spin_lock_bh(&msk->pm.lock);
 	if (alist.nr) {
-		spin_lock_bh(&msk->pm.lock);
 		msk->pm.add_addr_signaled -= alist.nr;
 		mptcp_pm_remove_addr(msk, &alist);
-		spin_unlock_bh(&msk->pm.lock);
 	}
-
 	if (slist.nr)
-		mptcp_pm_remove_subflow(msk, &slist);
-
+		mptcp_pm_nl_rm_subflow_received(msk, &slist);
 	/* Reset counters: maybe some subflows have been removed before */
-	spin_lock_bh(&msk->pm.lock);
 	bitmap_fill(msk->pm.id_avail_bitmap, MPTCP_PM_MAX_ADDR_ID + 1);
 	msk->pm.local_addr_used = 0;
 	spin_unlock_bh(&msk->pm.lock);
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 60c6b073d65f..a1c1b0ff1ce1 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -1026,7 +1026,6 @@ int mptcp_pm_announce_addr(struct mptcp_sock *msk,
 			   const struct mptcp_addr_info *addr,
 			   bool echo);
 int mptcp_pm_remove_addr(struct mptcp_sock *msk, const struct mptcp_rm_list *rm_list);
-int mptcp_pm_remove_subflow(struct mptcp_sock *msk, const struct mptcp_rm_list *rm_list);
 void mptcp_pm_remove_addrs(struct mptcp_sock *msk, struct list_head *rm_list);
 
 void mptcp_free_local_addr_list(struct mptcp_sock *msk);
@@ -1133,8 +1132,6 @@ static inline u8 subflow_get_local_id(const struct mptcp_subflow_context *subflo
 
 void __init mptcp_pm_nl_init(void);
 void mptcp_pm_nl_work(struct mptcp_sock *msk);
-void mptcp_pm_nl_rm_subflow_received(struct mptcp_sock *msk,
-				     const struct mptcp_rm_list *rm_list);
 unsigned int mptcp_pm_get_add_addr_signal_max(const struct mptcp_sock *msk);
 unsigned int mptcp_pm_get_add_addr_accept_max(const struct mptcp_sock *msk);
 unsigned int mptcp_pm_get_subflows_max(const struct mptcp_sock *msk);

-- 
2.45.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH net 08/14] mptcp: pm: only mark 'subflow' endp as available
  2024-08-19 19:45 [PATCH net 00/14] mptcp: pm: fix IDs not being reusable Matthieu Baerts (NGI0)
                   ` (6 preceding siblings ...)
  2024-08-19 19:45 ` [PATCH net 07/14] mptcp: pm: remove mptcp_pm_remove_subflow() Matthieu Baerts (NGI0)
@ 2024-08-19 19:45 ` Matthieu Baerts (NGI0)
  2024-08-19 19:45 ` [PATCH net 09/14] mptcp: pm: only decrement add_addr_accepted for MPJ req Matthieu Baerts (NGI0)
                   ` (6 subsequent siblings)
  14 siblings, 0 replies; 16+ messages in thread
From: Matthieu Baerts (NGI0) @ 2024-08-19 19:45 UTC (permalink / raw)
  To: mptcp, Mat Martineau, Geliang Tang, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Shuah Khan
  Cc: netdev, linux-kernel, linux-kselftest, Matthieu Baerts (NGI0),
	stable

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")
Cc: stable@vger.kernel.org
Reviewed-by: Mat Martineau <martineau@kernel.org>
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 44fc1c5959ac..4cf7cc851f80 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--;
 		}
 	}
 }
@@ -1441,6 +1439,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)
 {
@@ -1474,11 +1480,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, list.ids[0]);
 			spin_unlock_bh(&msk->pm.lock);
 		}
 
@@ -1516,6 +1522,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);
 
@@ -1917,6 +1924,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, list.ids[0]);
 	mptcp_pm_create_subflow_or_signal_addr(msk);
 	spin_unlock_bh(&msk->pm.lock);
 }

-- 
2.45.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH net 09/14] mptcp: pm: only decrement add_addr_accepted for MPJ req
  2024-08-19 19:45 [PATCH net 00/14] mptcp: pm: fix IDs not being reusable Matthieu Baerts (NGI0)
                   ` (7 preceding siblings ...)
  2024-08-19 19:45 ` [PATCH net 08/14] mptcp: pm: only mark 'subflow' endp as available Matthieu Baerts (NGI0)
@ 2024-08-19 19:45 ` Matthieu Baerts (NGI0)
  2024-08-19 19:45 ` [PATCH net 10/14] mptcp: pm: check add_addr_accept_max before accepting new ADD_ADDR Matthieu Baerts (NGI0)
                   ` (5 subsequent siblings)
  14 siblings, 0 replies; 16+ messages in thread
From: Matthieu Baerts (NGI0) @ 2024-08-19 19:45 UTC (permalink / raw)
  To: mptcp, Mat Martineau, Geliang Tang, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Shuah Khan
  Cc: netdev, linux-kernel, linux-kselftest, Matthieu Baerts (NGI0),
	stable

Adding the following warning ...

  WARN_ON_ONCE(msk->pm.add_addr_accepted == 0)

... before decrementing the add_addr_accepted counter helped to find a
bug when running the "remove single subflow" subtest from the
mptcp_join.sh selftest.

Removing a 'subflow' endpoint will first trigger a RM_ADDR, then the
subflow closure. Before this patch, and upon the reception of the
RM_ADDR, the other peer will then try to decrement this
add_addr_accepted. That's not correct because the attached subflows have
not been created upon the reception of an ADD_ADDR.

A way to solve that is to decrement the counter only if the attached
subflow was an MP_JOIN to a remote id that was not 0, and initiated by
the host receiving the RM_ADDR.

Fixes: d0876b2284cf ("mptcp: add the incoming RM_ADDR support")
Cc: stable@vger.kernel.org
Reviewed-by: Mat Martineau <martineau@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/pm_netlink.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 4cf7cc851f80..882781571c7b 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -829,7 +829,7 @@ static void mptcp_pm_nl_rm_addr_or_subflow(struct mptcp_sock *msk,
 			mptcp_close_ssk(sk, ssk, subflow);
 			spin_lock_bh(&msk->pm.lock);
 
-			removed = true;
+			removed |= subflow->request_join;
 			if (rm_type == MPTCP_MIB_RMSUBFLOW)
 				__MPTCP_INC_STATS(sock_net(sk), rm_type);
 		}
@@ -843,7 +843,11 @@ static void mptcp_pm_nl_rm_addr_or_subflow(struct mptcp_sock *msk,
 		if (!mptcp_pm_is_kernel(msk))
 			continue;
 
-		if (rm_type == MPTCP_MIB_RMADDR) {
+		if (rm_type == MPTCP_MIB_RMADDR && rm_id &&
+		    !WARN_ON_ONCE(msk->pm.add_addr_accepted == 0)) {
+			/* Note: if the subflow has been closed before, this
+			 * add_addr_accepted counter will not be decremented.
+			 */
 			msk->pm.add_addr_accepted--;
 			WRITE_ONCE(msk->pm.accept_addr, true);
 		}

-- 
2.45.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH net 10/14] mptcp: pm: check add_addr_accept_max before accepting new ADD_ADDR
  2024-08-19 19:45 [PATCH net 00/14] mptcp: pm: fix IDs not being reusable Matthieu Baerts (NGI0)
                   ` (8 preceding siblings ...)
  2024-08-19 19:45 ` [PATCH net 09/14] mptcp: pm: only decrement add_addr_accepted for MPJ req Matthieu Baerts (NGI0)
@ 2024-08-19 19:45 ` Matthieu Baerts (NGI0)
  2024-08-19 19:45 ` [PATCH net 11/14] mptcp: pm: only in-kernel cannot have entries with ID 0 Matthieu Baerts (NGI0)
                   ` (4 subsequent siblings)
  14 siblings, 0 replies; 16+ messages in thread
From: Matthieu Baerts (NGI0) @ 2024-08-19 19:45 UTC (permalink / raw)
  To: mptcp, Mat Martineau, Geliang Tang, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Shuah Khan
  Cc: netdev, linux-kernel, linux-kselftest, Matthieu Baerts (NGI0),
	stable

The limits might have changed in between, it is best to check them
before accepting new ADD_ADDR.

Fixes: d0876b2284cf ("mptcp: add the incoming RM_ADDR support")
Cc: stable@vger.kernel.org
Reviewed-by: Mat Martineau <martineau@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/pm_netlink.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 882781571c7b..28a9a3726146 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -848,8 +848,8 @@ static void mptcp_pm_nl_rm_addr_or_subflow(struct mptcp_sock *msk,
 			/* Note: if the subflow has been closed before, this
 			 * add_addr_accepted counter will not be decremented.
 			 */
-			msk->pm.add_addr_accepted--;
-			WRITE_ONCE(msk->pm.accept_addr, true);
+			if (--msk->pm.add_addr_accepted < mptcp_pm_get_add_addr_accept_max(msk))
+				WRITE_ONCE(msk->pm.accept_addr, true);
 		}
 	}
 }

-- 
2.45.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH net 11/14] mptcp: pm: only in-kernel cannot have entries with ID 0
  2024-08-19 19:45 [PATCH net 00/14] mptcp: pm: fix IDs not being reusable Matthieu Baerts (NGI0)
                   ` (9 preceding siblings ...)
  2024-08-19 19:45 ` [PATCH net 10/14] mptcp: pm: check add_addr_accept_max before accepting new ADD_ADDR Matthieu Baerts (NGI0)
@ 2024-08-19 19:45 ` Matthieu Baerts (NGI0)
  2024-08-19 19:45 ` [PATCH net 12/14] mptcp: pm: fullmesh: select the right ID later Matthieu Baerts (NGI0)
                   ` (3 subsequent siblings)
  14 siblings, 0 replies; 16+ messages in thread
From: Matthieu Baerts (NGI0) @ 2024-08-19 19:45 UTC (permalink / raw)
  To: mptcp, Mat Martineau, Geliang Tang, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Shuah Khan
  Cc: netdev, linux-kernel, linux-kselftest, Matthieu Baerts (NGI0),
	stable

The ID 0 is specific per MPTCP connections. The per netns entries cannot
have this special ID 0 then.

But that's different for the userspace PM where the entries are per
connection, they can then use this special ID 0.

Fixes: f40be0db0b76 ("mptcp: unify pm get_flags_and_ifindex_by_id")
Cc: stable@vger.kernel.org
Acked-by: Geliang Tang <geliang@kernel.org>
Reviewed-by: Mat Martineau <martineau@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/pm.c         | 3 ---
 net/mptcp/pm_netlink.c | 4 ++++
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 925123e99889..3e6e0f5510bb 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -434,9 +434,6 @@ int mptcp_pm_get_flags_and_ifindex_by_id(struct mptcp_sock *msk, unsigned int id
 	*flags = 0;
 	*ifindex = 0;
 
-	if (!id)
-		return 0;
-
 	if (mptcp_pm_is_userspace(msk))
 		return mptcp_userspace_pm_get_flags_and_ifindex_by_id(msk, id, flags, ifindex);
 	return mptcp_pm_nl_get_flags_and_ifindex_by_id(msk, id, flags, ifindex);
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 28a9a3726146..d0a80f537fc3 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1395,6 +1395,10 @@ int mptcp_pm_nl_get_flags_and_ifindex_by_id(struct mptcp_sock *msk, unsigned int
 	struct sock *sk = (struct sock *)msk;
 	struct net *net = sock_net(sk);
 
+	/* No entries with ID 0 */
+	if (id == 0)
+		return 0;
+
 	rcu_read_lock();
 	entry = __lookup_addr_by_id(pm_nl_get_pernet(net), id);
 	if (entry) {

-- 
2.45.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH net 12/14] mptcp: pm: fullmesh: select the right ID later
  2024-08-19 19:45 [PATCH net 00/14] mptcp: pm: fix IDs not being reusable Matthieu Baerts (NGI0)
                   ` (10 preceding siblings ...)
  2024-08-19 19:45 ` [PATCH net 11/14] mptcp: pm: only in-kernel cannot have entries with ID 0 Matthieu Baerts (NGI0)
@ 2024-08-19 19:45 ` Matthieu Baerts (NGI0)
  2024-08-19 19:45 ` [PATCH net 13/14] selftests: mptcp: join: validate fullmesh endp on 1st sf Matthieu Baerts (NGI0)
                   ` (2 subsequent siblings)
  14 siblings, 0 replies; 16+ messages in thread
From: Matthieu Baerts (NGI0) @ 2024-08-19 19:45 UTC (permalink / raw)
  To: mptcp, Mat Martineau, Geliang Tang, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Shuah Khan
  Cc: netdev, linux-kernel, linux-kselftest, Matthieu Baerts (NGI0),
	stable

When reacting upon the reception of an ADD_ADDR, the in-kernel PM first
looks for fullmesh endpoints. If there are some, it will pick them,
using their entry ID.

It should set the ID 0 when using the endpoint corresponding to the
initial subflow, it is a special case imposed by the MPTCP specs.

Note that msk->mpc_endpoint_id might not be set when receiving the first
ADD_ADDR from the server. So better to compare the addresses.

Fixes: 1a0d6136c5f0 ("mptcp: local addresses fullmesh")
Cc: stable@vger.kernel.org
Reviewed-by: Mat Martineau <martineau@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/pm_netlink.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index d0a80f537fc3..a2e37ab1c40f 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -636,6 +636,7 @@ static unsigned int fill_local_addresses_vec(struct mptcp_sock *msk,
 {
 	struct sock *sk = (struct sock *)msk;
 	struct mptcp_pm_addr_entry *entry;
+	struct mptcp_addr_info mpc_addr;
 	struct pm_nl_pernet *pernet;
 	unsigned int subflows_max;
 	int i = 0;
@@ -643,6 +644,8 @@ static unsigned int fill_local_addresses_vec(struct mptcp_sock *msk,
 	pernet = pm_nl_get_pernet_from_msk(msk);
 	subflows_max = mptcp_pm_get_subflows_max(msk);
 
+	mptcp_local_address((struct sock_common *)msk, &mpc_addr);
+
 	rcu_read_lock();
 	list_for_each_entry_rcu(entry, &pernet->local_addr_list, list) {
 		if (!(entry->flags & MPTCP_PM_ADDR_FLAG_FULLMESH))
@@ -653,7 +656,13 @@ static unsigned int fill_local_addresses_vec(struct mptcp_sock *msk,
 
 		if (msk->pm.subflows < subflows_max) {
 			msk->pm.subflows++;
-			addrs[i++] = entry->addr;
+			addrs[i] = entry->addr;
+
+			/* Special case for ID0: set the correct ID */
+			if (mptcp_addresses_equal(&entry->addr, &mpc_addr, entry->addr.port))
+				addrs[i].id = 0;
+
+			i++;
 		}
 	}
 	rcu_read_unlock();

-- 
2.45.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH net 13/14] selftests: mptcp: join: validate fullmesh endp on 1st sf
  2024-08-19 19:45 [PATCH net 00/14] mptcp: pm: fix IDs not being reusable Matthieu Baerts (NGI0)
                   ` (11 preceding siblings ...)
  2024-08-19 19:45 ` [PATCH net 12/14] mptcp: pm: fullmesh: select the right ID later Matthieu Baerts (NGI0)
@ 2024-08-19 19:45 ` Matthieu Baerts (NGI0)
  2024-08-19 19:45 ` [PATCH net 14/14] mptcp: pm: avoid possible UaF when selecting endp Matthieu Baerts (NGI0)
  2024-08-21  1:30 ` [PATCH net 00/14] mptcp: pm: fix IDs not being reusable patchwork-bot+netdevbpf
  14 siblings, 0 replies; 16+ messages in thread
From: Matthieu Baerts (NGI0) @ 2024-08-19 19:45 UTC (permalink / raw)
  To: mptcp, Mat Martineau, Geliang Tang, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Shuah Khan
  Cc: netdev, linux-kernel, linux-kselftest, Matthieu Baerts (NGI0),
	stable

This case was not covered, and the wrong ID was set before the previous
commit.

The rest is not modified, it is just that it will increase the code
coverage.

The right address ID can be verified by looking at the packet traces. We
could automate that using Netfilter with some cBPF code for example, but
that's always a bit cryptic. Packetdrill seems better fitted for that.

Fixes: 4f49d63352da ("selftests: mptcp: add fullmesh testcases")
Cc: stable@vger.kernel.org
Reviewed-by: Mat Martineau <martineau@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index f609c02c6123..89e553e0e0c2 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -3059,6 +3059,7 @@ fullmesh_tests()
 		pm_nl_set_limits $ns1 1 3
 		pm_nl_set_limits $ns2 1 3
 		pm_nl_add_endpoint $ns1 10.0.2.1 flags signal
+		pm_nl_add_endpoint $ns2 10.0.1.2 flags subflow,fullmesh
 		fullmesh=1 speed=slow \
 			run_tests $ns1 $ns2 10.0.1.1
 		chk_join_nr 3 3 3

-- 
2.45.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH net 14/14] mptcp: pm: avoid possible UaF when selecting endp
  2024-08-19 19:45 [PATCH net 00/14] mptcp: pm: fix IDs not being reusable Matthieu Baerts (NGI0)
                   ` (12 preceding siblings ...)
  2024-08-19 19:45 ` [PATCH net 13/14] selftests: mptcp: join: validate fullmesh endp on 1st sf Matthieu Baerts (NGI0)
@ 2024-08-19 19:45 ` Matthieu Baerts (NGI0)
  2024-08-21  1:30 ` [PATCH net 00/14] mptcp: pm: fix IDs not being reusable patchwork-bot+netdevbpf
  14 siblings, 0 replies; 16+ messages in thread
From: Matthieu Baerts (NGI0) @ 2024-08-19 19:45 UTC (permalink / raw)
  To: mptcp, Mat Martineau, Geliang Tang, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Shuah Khan
  Cc: netdev, linux-kernel, linux-kselftest, Matthieu Baerts (NGI0),
	stable

select_local_address() and select_signal_address() both select an
endpoint entry from the list inside an RCU protected section, but return
a reference to it, to be read later on. If the entry is dereferenced
after the RCU unlock, reading info could cause a Use-after-Free.

A simple solution is to copy the required info while inside the RCU
protected section to avoid any risk of UaF later. The address ID might
need to be modified later to handle the ID0 case later, so a copy seems
OK to deal with.

Reported-by: Paolo Abeni <pabeni@redhat.com>
Closes: https://lore.kernel.org/45cd30d3-7710-491c-ae4d-a1368c00beb1@redhat.com
Fixes: 01cacb00b35c ("mptcp: add netlink-based PM")
Cc: stable@vger.kernel.org
Reviewed-by: Mat Martineau <martineau@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/pm_netlink.c | 64 +++++++++++++++++++++++++++-----------------------
 1 file changed, 34 insertions(+), 30 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index a2e37ab1c40f..3e4ad801786f 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -143,11 +143,13 @@ static bool lookup_subflow_by_daddr(const struct list_head *list,
 	return false;
 }
 
-static struct mptcp_pm_addr_entry *
+static bool
 select_local_address(const struct pm_nl_pernet *pernet,
-		     const struct mptcp_sock *msk)
+		     const struct mptcp_sock *msk,
+		     struct mptcp_pm_addr_entry *new_entry)
 {
-	struct mptcp_pm_addr_entry *entry, *ret = NULL;
+	struct mptcp_pm_addr_entry *entry;
+	bool found = false;
 
 	msk_owned_by_me(msk);
 
@@ -159,17 +161,21 @@ select_local_address(const struct pm_nl_pernet *pernet,
 		if (!test_bit(entry->addr.id, msk->pm.id_avail_bitmap))
 			continue;
 
-		ret = entry;
+		*new_entry = *entry;
+		found = true;
 		break;
 	}
 	rcu_read_unlock();
-	return ret;
+
+	return found;
 }
 
-static struct mptcp_pm_addr_entry *
-select_signal_address(struct pm_nl_pernet *pernet, const struct mptcp_sock *msk)
+static bool
+select_signal_address(struct pm_nl_pernet *pernet, const struct mptcp_sock *msk,
+		      struct mptcp_pm_addr_entry *new_entry)
 {
-	struct mptcp_pm_addr_entry *entry, *ret = NULL;
+	struct mptcp_pm_addr_entry *entry;
+	bool found = false;
 
 	rcu_read_lock();
 	/* do not keep any additional per socket state, just signal
@@ -184,11 +190,13 @@ select_signal_address(struct pm_nl_pernet *pernet, const struct mptcp_sock *msk)
 		if (!(entry->flags & MPTCP_PM_ADDR_FLAG_SIGNAL))
 			continue;
 
-		ret = entry;
+		*new_entry = *entry;
+		found = true;
 		break;
 	}
 	rcu_read_unlock();
-	return ret;
+
+	return found;
 }
 
 unsigned int mptcp_pm_get_add_addr_signal_max(const struct mptcp_sock *msk)
@@ -512,9 +520,10 @@ __lookup_addr(struct pm_nl_pernet *pernet, const struct mptcp_addr_info *info)
 
 static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
 {
-	struct mptcp_pm_addr_entry *local, *signal_and_subflow = NULL;
 	struct sock *sk = (struct sock *)msk;
+	struct mptcp_pm_addr_entry local;
 	unsigned int add_addr_signal_max;
+	bool signal_and_subflow = false;
 	unsigned int local_addr_max;
 	struct pm_nl_pernet *pernet;
 	unsigned int subflows_max;
@@ -565,23 +574,22 @@ static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
 		if (msk->pm.addr_signal & BIT(MPTCP_ADD_ADDR_SIGNAL))
 			return;
 
-		local = select_signal_address(pernet, msk);
-		if (!local)
+		if (!select_signal_address(pernet, msk, &local))
 			goto subflow;
 
 		/* If the alloc fails, we are on memory pressure, not worth
 		 * continuing, and trying to create subflows.
 		 */
-		if (!mptcp_pm_alloc_anno_list(msk, &local->addr))
+		if (!mptcp_pm_alloc_anno_list(msk, &local.addr))
 			return;
 
-		__clear_bit(local->addr.id, msk->pm.id_avail_bitmap);
+		__clear_bit(local.addr.id, msk->pm.id_avail_bitmap);
 		msk->pm.add_addr_signaled++;
-		mptcp_pm_announce_addr(msk, &local->addr, false);
+		mptcp_pm_announce_addr(msk, &local.addr, false);
 		mptcp_pm_nl_addr_send_ack(msk);
 
-		if (local->flags & MPTCP_PM_ADDR_FLAG_SUBFLOW)
-			signal_and_subflow = local;
+		if (local.flags & MPTCP_PM_ADDR_FLAG_SUBFLOW)
+			signal_and_subflow = true;
 	}
 
 subflow:
@@ -592,26 +600,22 @@ static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
 		bool fullmesh;
 		int i, nr;
 
-		if (signal_and_subflow) {
-			local = signal_and_subflow;
-			signal_and_subflow = NULL;
-		} else {
-			local = select_local_address(pernet, msk);
-			if (!local)
-				break;
-		}
+		if (signal_and_subflow)
+			signal_and_subflow = false;
+		else if (!select_local_address(pernet, msk, &local))
+			break;
 
-		fullmesh = !!(local->flags & MPTCP_PM_ADDR_FLAG_FULLMESH);
+		fullmesh = !!(local.flags & MPTCP_PM_ADDR_FLAG_FULLMESH);
 
 		msk->pm.local_addr_used++;
-		__clear_bit(local->addr.id, msk->pm.id_avail_bitmap);
-		nr = fill_remote_addresses_vec(msk, &local->addr, fullmesh, addrs);
+		__clear_bit(local.addr.id, msk->pm.id_avail_bitmap);
+		nr = fill_remote_addresses_vec(msk, &local.addr, fullmesh, addrs);
 		if (nr == 0)
 			continue;
 
 		spin_unlock_bh(&msk->pm.lock);
 		for (i = 0; i < nr; i++)
-			__mptcp_subflow_connect(sk, &local->addr, &addrs[i]);
+			__mptcp_subflow_connect(sk, &local.addr, &addrs[i]);
 		spin_lock_bh(&msk->pm.lock);
 	}
 	mptcp_pm_nl_check_work_pending(msk);

-- 
2.45.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH net 00/14] mptcp: pm: fix IDs not being reusable
  2024-08-19 19:45 [PATCH net 00/14] mptcp: pm: fix IDs not being reusable Matthieu Baerts (NGI0)
                   ` (13 preceding siblings ...)
  2024-08-19 19:45 ` [PATCH net 14/14] mptcp: pm: avoid possible UaF when selecting endp Matthieu Baerts (NGI0)
@ 2024-08-21  1:30 ` patchwork-bot+netdevbpf
  14 siblings, 0 replies; 16+ messages in thread
From: patchwork-bot+netdevbpf @ 2024-08-21  1:30 UTC (permalink / raw)
  To: Matthieu Baerts
  Cc: mptcp, martineau, geliang, davem, edumazet, kuba, pabeni, shuah,
	netdev, linux-kernel, linux-kselftest, stable

Hello:

This series was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 19 Aug 2024 21:45:18 +0200 you wrote:
> Here are more fixes for the MPTCP in-kernel path-manager. In this
> series, the fixes are around the endpoint IDs not being reusable for
> on-going connections when re-creating endpoints with previously used IDs.
> 
> - Patch 1 fixes this case for endpoints being used to send ADD_ADDR.
>   Patch 2 validates this fix. The issue is present since v5.10.
> 
> [...]

Here is the summary with links:
  - [net,01/14] mptcp: pm: re-using ID of unused removed ADD_ADDR
    https://git.kernel.org/netdev/net/c/e255683c06df
  - [net,02/14] selftests: mptcp: join: check re-using ID of unused ADD_ADDR
    https://git.kernel.org/netdev/net/c/a13d5aad4dd9
  - [net,03/14] mptcp: pm: re-using ID of unused removed subflows
    https://git.kernel.org/netdev/net/c/edd8b5d868a4
  - [net,04/14] selftests: mptcp: join: check re-using ID of closed subflow
    https://git.kernel.org/netdev/net/c/65fb58afa341
  - [net,05/14] mptcp: pm: re-using ID of unused flushed subflows
    https://git.kernel.org/netdev/net/c/ef34a6ea0cab
  - [net,06/14] selftests: mptcp: join: test for flush/re-add endpoints
    https://git.kernel.org/netdev/net/c/e06959e9eebd
  - [net,07/14] mptcp: pm: remove mptcp_pm_remove_subflow()
    https://git.kernel.org/netdev/net/c/f448451aa62d
  - [net,08/14] mptcp: pm: only mark 'subflow' endp as available
    https://git.kernel.org/netdev/net/c/322ea3778965
  - [net,09/14] mptcp: pm: only decrement add_addr_accepted for MPJ req
    https://git.kernel.org/netdev/net/c/1c1f72137598
  - [net,10/14] mptcp: pm: check add_addr_accept_max before accepting new ADD_ADDR
    https://git.kernel.org/netdev/net/c/0137a3c7c2ea
  - [net,11/14] mptcp: pm: only in-kernel cannot have entries with ID 0
    https://git.kernel.org/netdev/net/c/ca6e55a703ca
  - [net,12/14] mptcp: pm: fullmesh: select the right ID later
    https://git.kernel.org/netdev/net/c/09355f7abb9f
  - [net,13/14] selftests: mptcp: join: validate fullmesh endp on 1st sf
    https://git.kernel.org/netdev/net/c/4878f9f8421f
  - [net,14/14] mptcp: pm: avoid possible UaF when selecting endp
    https://git.kernel.org/netdev/net/c/48e50dcbcbaa

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2024-08-21  1:30 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-08-19 19:45 [PATCH net 00/14] mptcp: pm: fix IDs not being reusable Matthieu Baerts (NGI0)
2024-08-19 19:45 ` [PATCH net 01/14] mptcp: pm: re-using ID of unused removed ADD_ADDR Matthieu Baerts (NGI0)
2024-08-19 19:45 ` [PATCH net 02/14] selftests: mptcp: join: check re-using ID of unused ADD_ADDR Matthieu Baerts (NGI0)
2024-08-19 19:45 ` [PATCH net 03/14] mptcp: pm: re-using ID of unused removed subflows Matthieu Baerts (NGI0)
2024-08-19 19:45 ` [PATCH net 04/14] selftests: mptcp: join: check re-using ID of closed subflow Matthieu Baerts (NGI0)
2024-08-19 19:45 ` [PATCH net 05/14] mptcp: pm: re-using ID of unused flushed subflows Matthieu Baerts (NGI0)
2024-08-19 19:45 ` [PATCH net 06/14] selftests: mptcp: join: test for flush/re-add endpoints Matthieu Baerts (NGI0)
2024-08-19 19:45 ` [PATCH net 07/14] mptcp: pm: remove mptcp_pm_remove_subflow() Matthieu Baerts (NGI0)
2024-08-19 19:45 ` [PATCH net 08/14] mptcp: pm: only mark 'subflow' endp as available Matthieu Baerts (NGI0)
2024-08-19 19:45 ` [PATCH net 09/14] mptcp: pm: only decrement add_addr_accepted for MPJ req Matthieu Baerts (NGI0)
2024-08-19 19:45 ` [PATCH net 10/14] mptcp: pm: check add_addr_accept_max before accepting new ADD_ADDR Matthieu Baerts (NGI0)
2024-08-19 19:45 ` [PATCH net 11/14] mptcp: pm: only in-kernel cannot have entries with ID 0 Matthieu Baerts (NGI0)
2024-08-19 19:45 ` [PATCH net 12/14] mptcp: pm: fullmesh: select the right ID later Matthieu Baerts (NGI0)
2024-08-19 19:45 ` [PATCH net 13/14] selftests: mptcp: join: validate fullmesh endp on 1st sf Matthieu Baerts (NGI0)
2024-08-19 19:45 ` [PATCH net 14/14] mptcp: pm: avoid possible UaF when selecting endp Matthieu Baerts (NGI0)
2024-08-21  1:30 ` [PATCH net 00/14] mptcp: pm: fix IDs not being reusable patchwork-bot+netdevbpf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).