diff mbox series

[mptcp-net,2/4] mptcp: consolidate passive msk socket initialization

Message ID e20a42028b3035cc227f0cafa654977a62fb6b77.1684321532.git.pabeni@redhat.com (mailing list archive)
State Superseded, archived
Delegated to: Paolo Abeni
Headers show
Series mptcp: a bunch of data race fixes | expand

Checks

Context Check Description
matttbe/checkpatch warning total: 0 errors, 1 warnings, 0 checks, 105 lines checked
matttbe/build warning Build error with: make C=1 net/mptcp/protocol.o
matttbe/KVM_Validation__normal__except_selftest_mptcp_join_ success Success! ✅
matttbe/KVM_Validation__normal__only_selftest_mptcp_join_ success Success! ✅
matttbe/KVM_Validation__debug__only_selftest_mptcp_join_ success Success! ✅
matttbe/KVM_Validation__debug__except_selftest_mptcp_join_ warning Unstable: 3 failed test(s): packetdrill_add_addr packetdrill_fastopen selftest_diag

Commit Message

Paolo Abeni May 17, 2023, 11:35 a.m. UTC 
When the msk socket is cloned at MPC handshake time, a few
fields are initializated in a racy way outside mptcp_sk_clone()
and the msk socket lock.

The above is due historical reasons: before commit a88d0092b24b
("mptcp: simplify subflow_syn_recv_sock()") as the first subflow socket
carrying all the needed date was not available yet at msk creation
time

We can now refactor the code moving the missing initialization bit
under the socket lock, removing the init race and avoiding some
code duplication.

This will also simplify the next patch, as all msk->first write
access are now under the msk socket lock.

Fixes: 0397c6d85f9c ("mptcp: keep unaccepted MPC subflow into join list")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/protocol.c | 33 +++++++++++++++++++++++++++------
 net/mptcp/protocol.h |  8 ++++----
 net/mptcp/subflow.c  | 28 +---------------------------
 3 files changed, 32 insertions(+), 37 deletions(-)

Comments

kernel test robot May 17, 2023, 2:12 p.m. UTC | #1 
Hi Paolo,

kernel test robot noticed the following build warnings:

[auto build test WARNING on mptcp/export]
[cannot apply to mptcp/export-net linus/master v6.4-rc2 next-20230517]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Paolo-Abeni/mptcp-add-annotations-around-msk-subflow-accesses/20230517-193828
base:   https://github.com/multipath-tcp/mptcp_net-next.git export
patch link:    https://lore.kernel.org/r/e20a42028b3035cc227f0cafa654977a62fb6b77.1684321532.git.pabeni%40redhat.com
patch subject: [PATCH mptcp-net 2/4] mptcp: consolidate passive msk socket initialization
config: m68k-allyesconfig
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/4ebe807adecd18d841f30ee470aa18b74558c988
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Paolo-Abeni/mptcp-add-annotations-around-msk-subflow-accesses/20230517-193828
        git checkout 4ebe807adecd18d841f30ee470aa18b74558c988
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash net/mptcp/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202305172134.QLT02wOb-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> net/mptcp/protocol.c:3089:6: warning: no previous prototype for 'mptcp_copy_inaddrs' [-Wmissing-prototypes]
    3089 | void mptcp_copy_inaddrs(struct sock *msk, const struct sock *ssk)
         |      ^~~~~~~~~~~~~~~~~~


vim +/mptcp_copy_inaddrs +3089 net/mptcp/protocol.c

f870fa0b576884 Mat Martineau 2020-01-21  3088  
e72e4032637f46 Paolo Abeni   2022-10-21 @3089  void mptcp_copy_inaddrs(struct sock *msk, const struct sock *ssk)
cf7da0d66cc1a2 Peter Krystad 2020-01-21  3090  {
cf7da0d66cc1a2 Peter Krystad 2020-01-21  3091  #if IS_ENABLED(CONFIG_MPTCP_IPV6)
cf7da0d66cc1a2 Peter Krystad 2020-01-21  3092  	const struct ipv6_pinfo *ssk6 = inet6_sk(ssk);
cf7da0d66cc1a2 Peter Krystad 2020-01-21  3093  	struct ipv6_pinfo *msk6 = inet6_sk(msk);
cf7da0d66cc1a2 Peter Krystad 2020-01-21  3094  
cf7da0d66cc1a2 Peter Krystad 2020-01-21  3095  	msk->sk_v6_daddr = ssk->sk_v6_daddr;
cf7da0d66cc1a2 Peter Krystad 2020-01-21  3096  	msk->sk_v6_rcv_saddr = ssk->sk_v6_rcv_saddr;
cf7da0d66cc1a2 Peter Krystad 2020-01-21  3097  
cf7da0d66cc1a2 Peter Krystad 2020-01-21  3098  	if (msk6 && ssk6) {
cf7da0d66cc1a2 Peter Krystad 2020-01-21  3099  		msk6->saddr = ssk6->saddr;
cf7da0d66cc1a2 Peter Krystad 2020-01-21  3100  		msk6->flow_label = ssk6->flow_label;
cf7da0d66cc1a2 Peter Krystad 2020-01-21  3101  	}
cf7da0d66cc1a2 Peter Krystad 2020-01-21  3102  #endif
cf7da0d66cc1a2 Peter Krystad 2020-01-21  3103  
cf7da0d66cc1a2 Peter Krystad 2020-01-21  3104  	inet_sk(msk)->inet_num = inet_sk(ssk)->inet_num;
cf7da0d66cc1a2 Peter Krystad 2020-01-21  3105  	inet_sk(msk)->inet_dport = inet_sk(ssk)->inet_dport;
cf7da0d66cc1a2 Peter Krystad 2020-01-21  3106  	inet_sk(msk)->inet_sport = inet_sk(ssk)->inet_sport;
cf7da0d66cc1a2 Peter Krystad 2020-01-21  3107  	inet_sk(msk)->inet_daddr = inet_sk(ssk)->inet_daddr;
cf7da0d66cc1a2 Peter Krystad 2020-01-21  3108  	inet_sk(msk)->inet_saddr = inet_sk(ssk)->inet_saddr;
cf7da0d66cc1a2 Peter Krystad 2020-01-21  3109  	inet_sk(msk)->inet_rcv_saddr = inet_sk(ssk)->inet_rcv_saddr;
cf7da0d66cc1a2 Peter Krystad 2020-01-21  3110  }
cf7da0d66cc1a2 Peter Krystad 2020-01-21  3111
kernel test robot May 17, 2023, 6:26 p.m. UTC | #2 
Hi Paolo,

kernel test robot noticed the following build warnings:

[auto build test WARNING on mptcp/export]
[cannot apply to mptcp/export-net linus/master v6.4-rc2 next-20230517]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Paolo-Abeni/mptcp-add-annotations-around-msk-subflow-accesses/20230517-193828
base:   https://github.com/multipath-tcp/mptcp_net-next.git export
patch link:    https://lore.kernel.org/r/e20a42028b3035cc227f0cafa654977a62fb6b77.1684321532.git.pabeni%40redhat.com
patch subject: [PATCH mptcp-net 2/4] mptcp: consolidate passive msk socket initialization
config: x86_64-randconfig-a001
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/4ebe807adecd18d841f30ee470aa18b74558c988
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Paolo-Abeni/mptcp-add-annotations-around-msk-subflow-accesses/20230517-193828
        git checkout 4ebe807adecd18d841f30ee470aa18b74558c988
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash net/mptcp/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202305180106.ndNxhLqT-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> net/mptcp/protocol.c:3089:6: warning: no previous prototype for function 'mptcp_copy_inaddrs' [-Wmissing-prototypes]
   void mptcp_copy_inaddrs(struct sock *msk, const struct sock *ssk)
        ^
   net/mptcp/protocol.c:3089:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   void mptcp_copy_inaddrs(struct sock *msk, const struct sock *ssk)
   ^
   static 
   1 warning generated.


vim +/mptcp_copy_inaddrs +3089 net/mptcp/protocol.c

f870fa0b576884 Mat Martineau 2020-01-21  3088  
e72e4032637f46 Paolo Abeni   2022-10-21 @3089  void mptcp_copy_inaddrs(struct sock *msk, const struct sock *ssk)
cf7da0d66cc1a2 Peter Krystad 2020-01-21  3090  {
cf7da0d66cc1a2 Peter Krystad 2020-01-21  3091  #if IS_ENABLED(CONFIG_MPTCP_IPV6)
cf7da0d66cc1a2 Peter Krystad 2020-01-21  3092  	const struct ipv6_pinfo *ssk6 = inet6_sk(ssk);
cf7da0d66cc1a2 Peter Krystad 2020-01-21  3093  	struct ipv6_pinfo *msk6 = inet6_sk(msk);
cf7da0d66cc1a2 Peter Krystad 2020-01-21  3094  
cf7da0d66cc1a2 Peter Krystad 2020-01-21  3095  	msk->sk_v6_daddr = ssk->sk_v6_daddr;
cf7da0d66cc1a2 Peter Krystad 2020-01-21  3096  	msk->sk_v6_rcv_saddr = ssk->sk_v6_rcv_saddr;
cf7da0d66cc1a2 Peter Krystad 2020-01-21  3097  
cf7da0d66cc1a2 Peter Krystad 2020-01-21  3098  	if (msk6 && ssk6) {
cf7da0d66cc1a2 Peter Krystad 2020-01-21  3099  		msk6->saddr = ssk6->saddr;
cf7da0d66cc1a2 Peter Krystad 2020-01-21  3100  		msk6->flow_label = ssk6->flow_label;
cf7da0d66cc1a2 Peter Krystad 2020-01-21  3101  	}
cf7da0d66cc1a2 Peter Krystad 2020-01-21  3102  #endif
cf7da0d66cc1a2 Peter Krystad 2020-01-21  3103  
cf7da0d66cc1a2 Peter Krystad 2020-01-21  3104  	inet_sk(msk)->inet_num = inet_sk(ssk)->inet_num;
cf7da0d66cc1a2 Peter Krystad 2020-01-21  3105  	inet_sk(msk)->inet_dport = inet_sk(ssk)->inet_dport;
cf7da0d66cc1a2 Peter Krystad 2020-01-21  3106  	inet_sk(msk)->inet_sport = inet_sk(ssk)->inet_sport;
cf7da0d66cc1a2 Peter Krystad 2020-01-21  3107  	inet_sk(msk)->inet_daddr = inet_sk(ssk)->inet_daddr;
cf7da0d66cc1a2 Peter Krystad 2020-01-21  3108  	inet_sk(msk)->inet_saddr = inet_sk(ssk)->inet_saddr;
cf7da0d66cc1a2 Peter Krystad 2020-01-21  3109  	inet_sk(msk)->inet_rcv_saddr = inet_sk(ssk)->inet_rcv_saddr;
cf7da0d66cc1a2 Peter Krystad 2020-01-21  3110  }
cf7da0d66cc1a2 Peter Krystad 2020-01-21  3111
diff mbox series

Patch

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index b96b1191763a..55db12cf7ccb 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -3163,9 +3163,10 @@  static struct ipv6_pinfo *mptcp_inet6_sk(const struct sock *sk)
 }
 #endif
 
-struct sock *mptcp_sk_clone(const struct sock *sk,
-			    const struct mptcp_options_received *mp_opt,
-			    struct request_sock *req)
+struct sock *mptcp_sk_clone_init(const struct sock *sk,
+				 const struct mptcp_options_received *mp_opt,
+				 struct sock *ssk,
+				 struct request_sock *req)
 {
 	struct mptcp_subflow_request_sock *subflow_req = mptcp_subflow_rsk(req);
 	struct sock *nsk = sk_clone_lock(sk, GFP_ATOMIC);
@@ -3198,10 +3199,30 @@  struct sock *mptcp_sk_clone(const struct sock *sk,
 	mptcp_init_sched(msk, mptcp_sk(sk)->sched);
 
 	sock_reset_flag(nsk, SOCK_RCU_FREE);
-	/* will be fully established after successful MPC subflow creation */
-	inet_sk_state_store(nsk, TCP_SYN_RECV);
-
 	security_inet_csk_clone(nsk, req);
+
+	/* this can't race with mptcp_close(), as the msk is
+	 * not yet exposted to user-space
+	 */
+	inet_sk_state_store(nsk, TCP_ESTABLISHED);
+
+	/* The msk maintain a referece to each subflow in the connections list */
+	WRITE_ONCE(msk->first, ssk);
+	list_add(&mptcp_subflow_ctx(ssk)->node, &msk->conn_list);
+	sock_hold(ssk);
+
+	/* new mpc subflow takes ownership of the newly
+	 * created mptcp socket
+	 */
+	mptcp_token_accept(subflow_req, msk);
+
+	/* set msk addresses early to ensure mptcp_pm_get_local_id()
+	 * uses the correct data
+	 */
+	mptcp_copy_inaddrs(nsk, ssk);
+	mptcp_propagate_sndbuf(nsk, ssk);
+
+	mptcp_rcv_space_init(msk, ssk);
 	bh_unlock_sock(nsk);
 
 	/* note: the newly allocated socket refcount is 2 now */
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 552d7b06aaa9..de94c01746dc 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -618,7 +618,6 @@  int mptcp_allow_join_id0(const struct net *net);
 unsigned int mptcp_stale_loss_cnt(const struct net *net);
 int mptcp_get_pm_type(const struct net *net);
 const char *mptcp_get_scheduler(const struct net *net);
-void mptcp_copy_inaddrs(struct sock *msk, const struct sock *ssk);
 void mptcp_subflow_fully_established(struct mptcp_subflow_context *subflow,
 				     const struct mptcp_options_received *mp_opt);
 bool __mptcp_retransmit_pending_data(struct sock *sk);
@@ -702,9 +701,10 @@  void __init mptcp_proto_init(void);
 int __init mptcp_proto_v6_init(void);
 #endif
 
-struct sock *mptcp_sk_clone(const struct sock *sk,
-			    const struct mptcp_options_received *mp_opt,
-			    struct request_sock *req);
+struct sock *mptcp_sk_clone_init(const struct sock *sk,
+				 const struct mptcp_options_received *mp_opt,
+				 struct sock *ssk,
+				 struct request_sock *req);
 void mptcp_get_options(const struct sk_buff *skb,
 		       struct mptcp_options_received *mp_opt);
 
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 76952cf74fc0..63ac4dc621d4 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -815,38 +815,12 @@  static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 		ctx->setsockopt_seq = listener->setsockopt_seq;
 
 		if (ctx->mp_capable) {
-			ctx->conn = mptcp_sk_clone(listener->conn, &mp_opt, req);
+			ctx->conn = mptcp_sk_clone_init(listener->conn, &mp_opt, child, req);
 			if (!ctx->conn)
 				goto fallback;
 
 			owner = mptcp_sk(ctx->conn);
-
-			/* this can't race with mptcp_close(), as the msk is
-			 * not yet exposted to user-space
-			 */
-			inet_sk_state_store(ctx->conn, TCP_ESTABLISHED);
-
-			/* record the newly created socket as the first msk
-			 * subflow, but don't link it yet into conn_list
-			 */
-			WRITE_ONCE(owner->first, child);
-
-			/* new mpc subflow takes ownership of the newly
-			 * created mptcp socket
-			 */
-			owner->setsockopt_seq = ctx->setsockopt_seq;
 			mptcp_pm_new_connection(owner, child, 1);
-			mptcp_token_accept(subflow_req, owner);
-
-			/* set msk addresses early to ensure mptcp_pm_get_local_id()
-			 * uses the correct data
-			 */
-			mptcp_copy_inaddrs(ctx->conn, child);
-			mptcp_propagate_sndbuf(ctx->conn, child);
-
-			mptcp_rcv_space_init(owner, child);
-			list_add(&ctx->node, &owner->conn_list);
-			sock_hold(child);
 
 			/* with OoO packets we can reach here without ingress
 			 * mpc option