[v1] mptcp: fix duplicate subflow creation

[PATCH mptcp-net 3/4] mptcp: fix data races on remote_id

Posted by Paolo Abeni 7 months ago

Similar to the previous patch, address the data race on
remote_id, adding the suitable ONCE annotations.

Fixes: bedee0b56113 ("mptcp: address lookup improvements")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/pm_netlink.c | 8 ++++----
 net/mptcp/subflow.c    | 6 +++---
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index d1b984c9dc25..066bc855365c 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -443,7 +443,7 @@ static unsigned int fill_remote_addresses_vec(struct mptcp_sock *msk,
 		mptcp_for_each_subflow(msk, subflow) {
 			ssk = mptcp_subflow_tcp_sock(subflow);
 			remote_address((struct sock_common *)ssk, &addrs[i]);
-			addrs[i].id = subflow->remote_id;
+			addrs[i].id = READ_ONCE(subflow->remote_id);
 			if (deny_id0 && !addrs[i].id)
 				continue;
 
@@ -800,17 +800,17 @@ static void mptcp_pm_nl_rm_addr_or_subflow(struct mptcp_sock *msk,
 		mptcp_for_each_subflow_safe(msk, subflow, tmp) {
 			struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
 			int how = RCV_SHUTDOWN | SEND_SHUTDOWN;
+			u8 srm_id = READ_ONCE(subflow->remote_id);
 			u8 id = subflow_get_local_id(subflow);
 
-			if (rm_type == MPTCP_MIB_RMADDR && subflow->remote_id != rm_id)
+			if (rm_type == MPTCP_MIB_RMADDR && srm_id != rm_id)
 				continue;
 			if (rm_type == MPTCP_MIB_RMSUBFLOW && !mptcp_local_id_match(msk, id, rm_id))
 				continue;
 
 			pr_debug(" -> %s rm_list_ids[%d]=%u local_id=%u remote_id=%u mpc_id=%u",
 				 rm_type == MPTCP_MIB_RMADDR ? "address" : "subflow",
-				 i, rm_id, id, subflow->remote_id,
-				 msk->mpc_endpoint_id);
+				 i, rm_id, id, srm_id, msk->mpc_endpoint_id);
 			spin_unlock_bh(&msk->pm.lock);
 			mptcp_subflow_shutdown(sk, ssk, how);
 
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 068784d3e748..6403c56f2902 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -536,7 +536,7 @@ static void subflow_finish_connect(struct sock *sk, const struct sk_buff *skb)
 		subflow->backup = mp_opt.backup;
 		subflow->thmac = mp_opt.thmac;
 		subflow->remote_nonce = mp_opt.nonce;
-		subflow->remote_id = mp_opt.join_id;
+		WRITE_ONCE(subflow->remote_id, mp_opt.join_id);
 		pr_debug("subflow=%p, thmac=%llu, remote_nonce=%u backup=%d",
 			 subflow, subflow->thmac, subflow->remote_nonce,
 			 subflow->backup);
@@ -1569,7 +1569,7 @@ int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_addr_info *loc,
 	pr_debug("msk=%p remote_token=%u local_id=%d remote_id=%d", msk,
 		 remote_token, local_id, remote_id);
 	subflow->remote_token = remote_token;
-	subflow->remote_id = remote_id;
+	WRITE_ONCE(subflow->remote_id, remote_id);
 	subflow->request_join = 1;
 	subflow->request_bkup = !!(flags & MPTCP_PM_ADDR_FLAG_BACKUP);
 	subflow->subflow_id = msk->subflow_id++;
@@ -1976,7 +1976,7 @@ static void subflow_ulp_clone(const struct request_sock *req,
 		new_ctx->fully_established = 1;
 		new_ctx->remote_key_valid = 1;
 		new_ctx->backup = subflow_req->backup;
-		new_ctx->remote_id = subflow_req->remote_id;
+		WRITE_ONCE(new_ctx->remote_id, subflow_req->remote_id);
 		new_ctx->token = subflow_req->token;
 		new_ctx->thmac = subflow_req->thmac;
 
-- 
2.43.0

Re: [PATCH mptcp-net 3/4] mptcp: fix data races on remote_id

Posted by Mat Martineau 7 months ago

On Mon, 5 Feb 2024, Paolo Abeni wrote:

> Similar to the previous patch, address the data race on
> remote_id, adding the suitable ONCE annotations.
>
> Fixes: bedee0b56113 ("mptcp: address lookup improvements")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> net/mptcp/pm_netlink.c | 8 ++++----
> net/mptcp/subflow.c    | 6 +++---
> 2 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index d1b984c9dc25..066bc855365c 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -443,7 +443,7 @@ static unsigned int fill_remote_addresses_vec(struct mptcp_sock *msk,
> 		mptcp_for_each_subflow(msk, subflow) {
> 			ssk = mptcp_subflow_tcp_sock(subflow);
> 			remote_address((struct sock_common *)ssk, &addrs[i]);
> -			addrs[i].id = subflow->remote_id;
> +			addrs[i].id = READ_ONCE(subflow->remote_id);
> 			if (deny_id0 && !addrs[i].id)
> 				continue;
>
> @@ -800,17 +800,17 @@ static void mptcp_pm_nl_rm_addr_or_subflow(struct mptcp_sock *msk,
> 		mptcp_for_each_subflow_safe(msk, subflow, tmp) {
> 			struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
> 			int how = RCV_SHUTDOWN | SEND_SHUTDOWN;
> +			u8 srm_id = READ_ONCE(subflow->remote_id);

Paolo -

"srm_id" reads oddly in statements like "srm_id != rm_id" since it creates 
some confusion between "remote" and "remove". Please rename to make the 
"remote" more obvious!

- Mat

> 			u8 id = subflow_get_local_id(subflow);
>
> -			if (rm_type == MPTCP_MIB_RMADDR && subflow->remote_id != rm_id)
> +			if (rm_type == MPTCP_MIB_RMADDR && srm_id != rm_id)
> 				continue;
> 			if (rm_type == MPTCP_MIB_RMSUBFLOW && !mptcp_local_id_match(msk, id, rm_id))
> 				continue;
>
> 			pr_debug(" -> %s rm_list_ids[%d]=%u local_id=%u remote_id=%u mpc_id=%u",
> 				 rm_type == MPTCP_MIB_RMADDR ? "address" : "subflow",
> -				 i, rm_id, id, subflow->remote_id,
> -				 msk->mpc_endpoint_id);
> +				 i, rm_id, id, srm_id, msk->mpc_endpoint_id);
> 			spin_unlock_bh(&msk->pm.lock);
> 			mptcp_subflow_shutdown(sk, ssk, how);
>
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 068784d3e748..6403c56f2902 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -536,7 +536,7 @@ static void subflow_finish_connect(struct sock *sk, const struct sk_buff *skb)
> 		subflow->backup = mp_opt.backup;
> 		subflow->thmac = mp_opt.thmac;
> 		subflow->remote_nonce = mp_opt.nonce;
> -		subflow->remote_id = mp_opt.join_id;
> +		WRITE_ONCE(subflow->remote_id, mp_opt.join_id);
> 		pr_debug("subflow=%p, thmac=%llu, remote_nonce=%u backup=%d",
> 			 subflow, subflow->thmac, subflow->remote_nonce,
> 			 subflow->backup);
> @@ -1569,7 +1569,7 @@ int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_addr_info *loc,
> 	pr_debug("msk=%p remote_token=%u local_id=%d remote_id=%d", msk,
> 		 remote_token, local_id, remote_id);
> 	subflow->remote_token = remote_token;
> -	subflow->remote_id = remote_id;
> +	WRITE_ONCE(subflow->remote_id, remote_id);
> 	subflow->request_join = 1;
> 	subflow->request_bkup = !!(flags & MPTCP_PM_ADDR_FLAG_BACKUP);
> 	subflow->subflow_id = msk->subflow_id++;
> @@ -1976,7 +1976,7 @@ static void subflow_ulp_clone(const struct request_sock *req,
> 		new_ctx->fully_established = 1;
> 		new_ctx->remote_key_valid = 1;
> 		new_ctx->backup = subflow_req->backup;
> -		new_ctx->remote_id = subflow_req->remote_id;
> +		WRITE_ONCE(new_ctx->remote_id, subflow_req->remote_id);
> 		new_ctx->token = subflow_req->token;
> 		new_ctx->thmac = subflow_req->thmac;
>
> -- 
> 2.43.0
>
>
>

[PATCH mptcp-net 1/4] mptcp: fix lockless access in subflow ULP diag
[PATCH mptcp-net 2/4] mptcp: fix data races on local_id
[PATCH mptcp-net 3/4] mptcp: fix data races on remote_id
[PATCH mptcp-net 4/4] mptcp: fix duplicate subflow creation