[PATCH mptcp-next] mptcp: factor out mptcp_connect()

[PATCH mptcp-next] mptcp: factor out mptcp_connect()

[Paolo Abeni]

The current MPTCP connect implementation duplicates a bit of inet
code and does not use nor provide a struct proto->connect callback,
which in turn will not fit the upcoming fastopen implementation.

Refactor such implementation to use the common helper, moving the
MPTCP-specific bits into mptcp_connect(). Additionally, avoid an
indirect call to the subflow connect callback.

Note that the fastopen call-path invokes mptcp_connect() while already
holding the subflow socket lock. Explicitly keep track of such path
via a new MPTCP-level flag and handle the locking accordingly.

Additionally, track the connect flags in a new msk field to allow
propagating them to the subflow inet_stream_connect call.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/protocol.c | 128 +++++++++++++++++++++----------------------
 net/mptcp/protocol.h |   4 +-
 2 files changed, 65 insertions(+), 67 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 8feb684408f7..6245248d0182 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1689,7 +1689,10 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 
 		lock_sock(ssk);
 
+		msk->connect_flags = (msg->msg_flags & MSG_DONTWAIT) ? O_NONBLOCK : 0;
+		msk->is_sendmsg = 1;
 		ret = tcp_sendmsg_fastopen(ssk, msg, &copied_syn, len, NULL);
+		msk->is_sendmsg = 0;
 		copied += copied_syn;
 		if (ret == -EINPROGRESS && copied_syn > 0) {
 			/* reflect the new state on the MPTCP socket */
@@ -3506,10 +3509,65 @@ static int mptcp_ioctl(struct sock *sk, int cmd, unsigned long arg)
 	return put_user(answ, (int __user *)arg);
 }
 
+static void mptcp_subflow_early_fallback(struct mptcp_sock *msk,
+					 struct mptcp_subflow_context *subflow)
+{
+	subflow->request_mptcp = 0;
+	__mptcp_do_fallback(msk);
+}
+
+static int mptcp_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len)
+{
+	struct mptcp_subflow_context *subflow;
+	struct mptcp_sock *msk = mptcp_sk(sk);
+	struct socket *ssock;
+	int err = -EINVAL;
+
+	ssock = __mptcp_nmpc_socket(msk);
+	if (!ssock)
+		return -EINVAL;
+
+	mptcp_token_destroy(msk);
+	inet_sk_state_store(sk, TCP_SYN_SENT);
+	subflow = mptcp_subflow_ctx(ssock->sk);
+#ifdef CONFIG_TCP_MD5SIG
+	/* no MPTCP if MD5SIG is enabled on this socket or we may run out of
+	 * TCP option space.
+	 */
+	if (rcu_access_pointer(tcp_sk(ssock->sk)->md5sig_info))
+		mptcp_subflow_early_fallback(msk, subflow);
+#endif
+	if (subflow->request_mptcp && mptcp_token_new_connect(ssock->sk)) {
+		MPTCP_INC_STATS(sock_net(ssock->sk), MPTCP_MIB_TOKENFALLBACKINIT);
+		mptcp_subflow_early_fallback(msk, subflow);
+	}
+	if (likely(!__mptcp_check_fallback(msk)))
+		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPCAPABLEACTIVE);
+
+	/* if reaching here via the fastopen/sendmsg path, the caller already
+	 * acquired the subflow socket lock, too.
+	 */
+	if (msk->is_sendmsg)
+		err = __inet_stream_connect(ssock, uaddr, addr_len, msk->connect_flags, 1);
+	else
+		err = inet_stream_connect(ssock, uaddr, addr_len, msk->connect_flags);
+	inet_sk(sk)->defer_connect = inet_sk(ssock->sk)->defer_connect;
+
+	/* on successful connect, the msk state will be moved to established by
+	 * subflow_finish_connect()
+	 */
+	if (!err || err == -EINPROGRESS)
+		mptcp_copy_inaddrs(sk, ssock->sk);
+	else
+		inet_sk_state_store(sk, inet_sk_state_load(ssock->sk));
+	return err;
+}
+
 static struct proto mptcp_prot = {
 	.name		= "MPTCP",
 	.owner		= THIS_MODULE,
 	.init		= mptcp_init_sock,
+	.connect	= mptcp_connect,
 	.disconnect	= mptcp_disconnect,
 	.close		= mptcp_close,
 	.accept		= mptcp_accept,
@@ -3561,78 +3619,16 @@ static int mptcp_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
 	return err;
 }
 
-static void mptcp_subflow_early_fallback(struct mptcp_sock *msk,
-					 struct mptcp_subflow_context *subflow)
-{
-	subflow->request_mptcp = 0;
-	__mptcp_do_fallback(msk);
-}
-
 static int mptcp_stream_connect(struct socket *sock, struct sockaddr *uaddr,
 				int addr_len, int flags)
 {
-	struct mptcp_sock *msk = mptcp_sk(sock->sk);
-	struct mptcp_subflow_context *subflow;
-	struct socket *ssock;
-	int err = -EINVAL;
+	int ret;
 
 	lock_sock(sock->sk);
-	if (uaddr) {
-		if (addr_len < sizeof(uaddr->sa_family))
-			goto unlock;
-
-		if (uaddr->sa_family == AF_UNSPEC) {
-			err = mptcp_disconnect(sock->sk, flags);
-			sock->state = err ? SS_DISCONNECTING : SS_UNCONNECTED;
-			goto unlock;
-		}
-	}
-
-	if (sock->state != SS_UNCONNECTED && msk->subflow) {
-		/* pending connection or invalid state, let existing subflow
-		 * cope with that
-		 */
-		ssock = msk->subflow;
-		goto do_connect;
-	}
-
-	ssock = __mptcp_nmpc_socket(msk);
-	if (!ssock)
-		goto unlock;
-
-	mptcp_token_destroy(msk);
-	inet_sk_state_store(sock->sk, TCP_SYN_SENT);
-	subflow = mptcp_subflow_ctx(ssock->sk);
-#ifdef CONFIG_TCP_MD5SIG
-	/* no MPTCP if MD5SIG is enabled on this socket or we may run out of
-	 * TCP option space.
-	 */
-	if (rcu_access_pointer(tcp_sk(ssock->sk)->md5sig_info))
-		mptcp_subflow_early_fallback(msk, subflow);
-#endif
-	if (subflow->request_mptcp && mptcp_token_new_connect(ssock->sk)) {
-		MPTCP_INC_STATS(sock_net(ssock->sk), MPTCP_MIB_TOKENFALLBACKINIT);
-		mptcp_subflow_early_fallback(msk, subflow);
-	}
-	if (likely(!__mptcp_check_fallback(msk)))
-		MPTCP_INC_STATS(sock_net(sock->sk), MPTCP_MIB_MPCAPABLEACTIVE);
-
-do_connect:
-	err = ssock->ops->connect(ssock, uaddr, addr_len, flags);
-	inet_sk(sock->sk)->defer_connect = inet_sk(ssock->sk)->defer_connect;
-	sock->state = ssock->state;
-
-	/* on successful connect, the msk state will be moved to established by
-	 * subflow_finish_connect()
-	 */
-	if (!err || err == -EINPROGRESS)
-		mptcp_copy_inaddrs(sock->sk, ssock->sk);
-	else
-		inet_sk_state_store(sock->sk, inet_sk_state_load(ssock->sk));
-
-unlock:
+	mptcp_sk(sock->sk)->connect_flags = flags;
+	ret = __inet_stream_connect(sock, uaddr, addr_len, flags, 0);
 	release_sock(sock->sk);
-	return err;
+	return ret;
 }
 
 static int mptcp_listen(struct socket *sock, int backlog)
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 93c535440a5c..18f866b1afda 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -285,7 +285,9 @@ struct mptcp_sock {
 	u8		mpc_endpoint_id;
 	u8		recvmsg_inq:1,
 			cork:1,
-			nodelay:1;
+			nodelay:1,
+			is_sendmsg:1;
+	int		connect_flags;
 	struct work_struct work;
 	struct sk_buff  *ooo_last_skb;
 	struct rb_root  out_of_order_queue;
-- 
2.37.3

Re: [PATCH mptcp-next] mptcp: factor out mptcp_connect()

[Dmytro Shytyi]

Thank you, Paolo!


I will rebase Fast Open Mechanism patches, after these patches will be 
merged to the tree.


Best,

Dmytro.

On 10/3/2022 5:59 PM, Paolo Abeni wrote:
> not fit the upcoming fastopen implementation.
>
> Refactor such implementation to use the common helper, moving the
> MPTCP-specific bits into mptcp_connect(). Additionally, avoid an
> indirect call to the subflow connect callback.
>
> Note that the fastopen call-path invokes mptcp_connect() while already
> holding the subflow socket lock. Explicitly keep track of such path
> via a new MPTCP-level flag and handle the locking accordingly.

Re: [PATCH mptcp-next] mptcp: factor out mptcp_connect()

[Mat Martineau]

On Mon, 3 Oct 2022, Paolo Abeni wrote:

> The current MPTCP connect implementation duplicates a bit of inet
> code and does not use nor provide a struct proto->connect callback,
> which in turn will not fit the upcoming fastopen implementation.
>
> Refactor such implementation to use the common helper, moving the
> MPTCP-specific bits into mptcp_connect(). Additionally, avoid an
> indirect call to the subflow connect callback.
>
> Note that the fastopen call-path invokes mptcp_connect() while already
> holding the subflow socket lock. Explicitly keep track of such path
> via a new MPTCP-level flag and handle the locking accordingly.
>
> Additionally, track the connect flags in a new msk field to allow
> propagating them to the subflow inet_stream_connect call.
>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> net/mptcp/protocol.c | 128 +++++++++++++++++++++----------------------
> net/mptcp/protocol.h |   4 +-
> 2 files changed, 65 insertions(+), 67 deletions(-)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 8feb684408f7..6245248d0182 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -1689,7 +1689,10 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
>
> 		lock_sock(ssk);
>
> +		msk->connect_flags = (msg->msg_flags & MSG_DONTWAIT) ? O_NONBLOCK : 0;
> +		msk->is_sendmsg = 1;
> 		ret = tcp_sendmsg_fastopen(ssk, msg, &copied_syn, len, NULL);
> +		msk->is_sendmsg = 0;

I'm not excited about passing the flags this way (new msk members), but it 
seems better than changing the parameters of (struct proto_ops)->connect 
(and every connect function it's used with). So, I'm ok with the above 
change.

> 		copied += copied_syn;
> 		if (ret == -EINPROGRESS && copied_syn > 0) {
> 			/* reflect the new state on the MPTCP socket */
> @@ -3506,10 +3509,65 @@ static int mptcp_ioctl(struct sock *sk, int cmd, unsigned long arg)
> 	return put_user(answ, (int __user *)arg);
> }
>
> +static void mptcp_subflow_early_fallback(struct mptcp_sock *msk,
> +					 struct mptcp_subflow_context *subflow)
> +{
> +	subflow->request_mptcp = 0;
> +	__mptcp_do_fallback(msk);
> +}
> +
> +static int mptcp_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len)
> +{
> +	struct mptcp_subflow_context *subflow;
> +	struct mptcp_sock *msk = mptcp_sk(sk);
> +	struct socket *ssock;
> +	int err = -EINVAL;
> +
> +	ssock = __mptcp_nmpc_socket(msk);
> +	if (!ssock)
> +		return -EINVAL;
> +
> +	mptcp_token_destroy(msk);
> +	inet_sk_state_store(sk, TCP_SYN_SENT);
> +	subflow = mptcp_subflow_ctx(ssock->sk);
> +#ifdef CONFIG_TCP_MD5SIG
> +	/* no MPTCP if MD5SIG is enabled on this socket or we may run out of
> +	 * TCP option space.
> +	 */
> +	if (rcu_access_pointer(tcp_sk(ssock->sk)->md5sig_info))
> +		mptcp_subflow_early_fallback(msk, subflow);
> +#endif
> +	if (subflow->request_mptcp && mptcp_token_new_connect(ssock->sk)) {
> +		MPTCP_INC_STATS(sock_net(ssock->sk), MPTCP_MIB_TOKENFALLBACKINIT);
> +		mptcp_subflow_early_fallback(msk, subflow);
> +	}
> +	if (likely(!__mptcp_check_fallback(msk)))
> +		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPCAPABLEACTIVE);
> +
> +	/* if reaching here via the fastopen/sendmsg path, the caller already
> +	 * acquired the subflow socket lock, too.
> +	 */
> +	if (msk->is_sendmsg)
> +		err = __inet_stream_connect(ssock, uaddr, addr_len, msk->connect_flags, 1);
> +	else
> +		err = inet_stream_connect(ssock, uaddr, addr_len, msk->connect_flags);
> +	inet_sk(sk)->defer_connect = inet_sk(ssock->sk)->defer_connect;
> +
> +	/* on successful connect, the msk state will be moved to established by
> +	 * subflow_finish_connect()
> +	 */
> +	if (!err || err == -EINPROGRESS)
> +		mptcp_copy_inaddrs(sk, ssock->sk);
> +	else
> +		inet_sk_state_store(sk, inet_sk_state_load(ssock->sk));
> +	return err;
> +}
> +
> static struct proto mptcp_prot = {
> 	.name		= "MPTCP",
> 	.owner		= THIS_MODULE,
> 	.init		= mptcp_init_sock,
> +	.connect	= mptcp_connect,
> 	.disconnect	= mptcp_disconnect,
> 	.close		= mptcp_close,
> 	.accept		= mptcp_accept,
> @@ -3561,78 +3619,16 @@ static int mptcp_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
> 	return err;
> }
>
> -static void mptcp_subflow_early_fallback(struct mptcp_sock *msk,
> -					 struct mptcp_subflow_context *subflow)
> -{
> -	subflow->request_mptcp = 0;
> -	__mptcp_do_fallback(msk);
> -}
> -
> static int mptcp_stream_connect(struct socket *sock, struct sockaddr *uaddr,
> 				int addr_len, int flags)
> {
> -	struct mptcp_sock *msk = mptcp_sk(sock->sk);
> -	struct mptcp_subflow_context *subflow;
> -	struct socket *ssock;
> -	int err = -EINVAL;
> +	int ret;
>
> 	lock_sock(sock->sk);
> -	if (uaddr) {
> -		if (addr_len < sizeof(uaddr->sa_family))
> -			goto unlock;
> -
> -		if (uaddr->sa_family == AF_UNSPEC) {
> -			err = mptcp_disconnect(sock->sk, flags);
> -			sock->state = err ? SS_DISCONNECTING : SS_UNCONNECTED;
> -			goto unlock;
> -		}
> -	}
> -
> -	if (sock->state != SS_UNCONNECTED && msk->subflow) {
> -		/* pending connection or invalid state, let existing subflow
> -		 * cope with that
> -		 */
> -		ssock = msk->subflow;
> -		goto do_connect;
> -	}

I don't see a similar chunk of code added back in elsewhere in this patch.

After looking at the commit message for 41be81a8d3d0 ("mptcp: fix 
unblocking connect()"), I'm not sure the nonblocking connect is still 
handled? I see where __inet_stream_connect() will not call mptcp_connect() 
with SS_UNCONNECTED, but it's not obvious why it's no longer important to 
handle this scenario any more. Could you clarify the reasoning for this in 
the commit message if the nonblocking connect is handled some other way 
now?

- Mat

> -
> -	ssock = __mptcp_nmpc_socket(msk);
> -	if (!ssock)
> -		goto unlock;
> -
> -	mptcp_token_destroy(msk);
> -	inet_sk_state_store(sock->sk, TCP_SYN_SENT);
> -	subflow = mptcp_subflow_ctx(ssock->sk);
> -#ifdef CONFIG_TCP_MD5SIG
> -	/* no MPTCP if MD5SIG is enabled on this socket or we may run out of
> -	 * TCP option space.
> -	 */
> -	if (rcu_access_pointer(tcp_sk(ssock->sk)->md5sig_info))
> -		mptcp_subflow_early_fallback(msk, subflow);
> -#endif
> -	if (subflow->request_mptcp && mptcp_token_new_connect(ssock->sk)) {
> -		MPTCP_INC_STATS(sock_net(ssock->sk), MPTCP_MIB_TOKENFALLBACKINIT);
> -		mptcp_subflow_early_fallback(msk, subflow);
> -	}
> -	if (likely(!__mptcp_check_fallback(msk)))
> -		MPTCP_INC_STATS(sock_net(sock->sk), MPTCP_MIB_MPCAPABLEACTIVE);
> -
> -do_connect:
> -	err = ssock->ops->connect(ssock, uaddr, addr_len, flags);
> -	inet_sk(sock->sk)->defer_connect = inet_sk(ssock->sk)->defer_connect;
> -	sock->state = ssock->state;
> -
> -	/* on successful connect, the msk state will be moved to established by
> -	 * subflow_finish_connect()
> -	 */
> -	if (!err || err == -EINPROGRESS)
> -		mptcp_copy_inaddrs(sock->sk, ssock->sk);
> -	else
> -		inet_sk_state_store(sock->sk, inet_sk_state_load(ssock->sk));
> -
> -unlock:
> +	mptcp_sk(sock->sk)->connect_flags = flags;
> +	ret = __inet_stream_connect(sock, uaddr, addr_len, flags, 0);
> 	release_sock(sock->sk);
> -	return err;
> +	return ret;
> }
>
> static int mptcp_listen(struct socket *sock, int backlog)
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 93c535440a5c..18f866b1afda 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -285,7 +285,9 @@ struct mptcp_sock {
> 	u8		mpc_endpoint_id;
> 	u8		recvmsg_inq:1,
> 			cork:1,
> -			nodelay:1;
> +			nodelay:1,
> +			is_sendmsg:1;
> +	int		connect_flags;
> 	struct work_struct work;
> 	struct sk_buff  *ooo_last_skb;
> 	struct rb_root  out_of_order_queue;
> -- 
> 2.37.3
>
>
>

--
Mat Martineau
Intel

Re: [PATCH mptcp-next] mptcp: factor out mptcp_connect()

[Paolo Abeni]

On Mon, 2022-10-03 at 15:30 -0700, Mat Martineau wrote:
> On Mon, 3 Oct 2022, Paolo Abeni wrote:
> 
> > The current MPTCP connect implementation duplicates a bit of inet
> > code and does not use nor provide a struct proto->connect callback,
> > which in turn will not fit the upcoming fastopen implementation.
> > 
> > Refactor such implementation to use the common helper, moving the
> > MPTCP-specific bits into mptcp_connect(). Additionally, avoid an
> > indirect call to the subflow connect callback.
> > 
> > Note that the fastopen call-path invokes mptcp_connect() while already
> > holding the subflow socket lock. Explicitly keep track of such path
> > via a new MPTCP-level flag and handle the locking accordingly.
> > 
> > Additionally, track the connect flags in a new msk field to allow
> > propagating them to the subflow inet_stream_connect call.
> > 
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > ---
> > net/mptcp/protocol.c | 128 +++++++++++++++++++++----------------------
> > net/mptcp/protocol.h |   4 +-
> > 2 files changed, 65 insertions(+), 67 deletions(-)
> > 
> > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > index 8feb684408f7..6245248d0182 100644
> > --- a/net/mptcp/protocol.c
> > +++ b/net/mptcp/protocol.c
> > @@ -1689,7 +1689,10 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
> > 
> > 		lock_sock(ssk);
> > 
> > +		msk->connect_flags = (msg->msg_flags & MSG_DONTWAIT) ? O_NONBLOCK : 0;
> > +		msk->is_sendmsg = 1;
> > 		ret = tcp_sendmsg_fastopen(ssk, msg, &copied_syn, len, NULL);
> > +		msk->is_sendmsg = 0;
> 
> I'm not excited about passing the flags this way (new msk members), but it 
> seems better than changing the parameters of (struct proto_ops)->connect 
> (and every connect function it's used with). So, I'm ok with the above 
> change.

I agree it's not very nice. Perhpas it can be made less ugly moving the

		msk->is_sendmsg = 0;

in mptcp_stream_connect()?

> > 		copied += copied_syn;
> > 		if (ret == -EINPROGRESS && copied_syn > 0) {
> > 			/* reflect the new state on the MPTCP socket */
> > @@ -3506,10 +3509,65 @@ static int mptcp_ioctl(struct sock *sk, int cmd, unsigned long arg)
> > 	return put_user(answ, (int __user *)arg);
> > }
> > 
> > +static void mptcp_subflow_early_fallback(struct mptcp_sock *msk,
> > +					 struct mptcp_subflow_context *subflow)
> > +{
> > +	subflow->request_mptcp = 0;
> > +	__mptcp_do_fallback(msk);
> > +}
> > +
> > +static int mptcp_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len)
> > +{
> > +	struct mptcp_subflow_context *subflow;
> > +	struct mptcp_sock *msk = mptcp_sk(sk);
> > +	struct socket *ssock;
> > +	int err = -EINVAL;
> > +
> > +	ssock = __mptcp_nmpc_socket(msk);
> > +	if (!ssock)
> > +		return -EINVAL;
> > +
> > +	mptcp_token_destroy(msk);
> > +	inet_sk_state_store(sk, TCP_SYN_SENT);
> > +	subflow = mptcp_subflow_ctx(ssock->sk);
> > +#ifdef CONFIG_TCP_MD5SIG
> > +	/* no MPTCP if MD5SIG is enabled on this socket or we may run out of
> > +	 * TCP option space.
> > +	 */
> > +	if (rcu_access_pointer(tcp_sk(ssock->sk)->md5sig_info))
> > +		mptcp_subflow_early_fallback(msk, subflow);
> > +#endif
> > +	if (subflow->request_mptcp && mptcp_token_new_connect(ssock->sk)) {
> > +		MPTCP_INC_STATS(sock_net(ssock->sk), MPTCP_MIB_TOKENFALLBACKINIT);
> > +		mptcp_subflow_early_fallback(msk, subflow);
> > +	}
> > +	if (likely(!__mptcp_check_fallback(msk)))
> > +		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPCAPABLEACTIVE);
> > +
> > +	/* if reaching here via the fastopen/sendmsg path, the caller already
> > +	 * acquired the subflow socket lock, too.
> > +	 */
> > +	if (msk->is_sendmsg)
> > +		err = __inet_stream_connect(ssock, uaddr, addr_len, msk->connect_flags, 1);
> > +	else
> > +		err = inet_stream_connect(ssock, uaddr, addr_len, msk->connect_flags);
> > +	inet_sk(sk)->defer_connect = inet_sk(ssock->sk)->defer_connect;
> > +
> > +	/* on successful connect, the msk state will be moved to established by
> > +	 * subflow_finish_connect()
> > +	 */
> > +	if (!err || err == -EINPROGRESS)
> > +		mptcp_copy_inaddrs(sk, ssock->sk);
> > +	else
> > +		inet_sk_state_store(sk, inet_sk_state_load(ssock->sk));
> > +	return err;
> > +}
> > +
> > static struct proto mptcp_prot = {
> > 	.name		= "MPTCP",
> > 	.owner		= THIS_MODULE,
> > 	.init		= mptcp_init_sock,
> > +	.connect	= mptcp_connect,
> > 	.disconnect	= mptcp_disconnect,
> > 	.close		= mptcp_close,
> > 	.accept		= mptcp_accept,
> > @@ -3561,78 +3619,16 @@ static int mptcp_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
> > 	return err;
> > }
> > 
> > -static void mptcp_subflow_early_fallback(struct mptcp_sock *msk,
> > -					 struct mptcp_subflow_context *subflow)
> > -{
> > -	subflow->request_mptcp = 0;
> > -	__mptcp_do_fallback(msk);
> > -}
> > -
> > static int mptcp_stream_connect(struct socket *sock, struct sockaddr *uaddr,
> > 				int addr_len, int flags)
> > {
> > -	struct mptcp_sock *msk = mptcp_sk(sock->sk);
> > -	struct mptcp_subflow_context *subflow;
> > -	struct socket *ssock;
> > -	int err = -EINVAL;
> > +	int ret;
> > 
> > 	lock_sock(sock->sk);
> > -	if (uaddr) {
> > -		if (addr_len < sizeof(uaddr->sa_family))
> > -			goto unlock;
> > -
> > -		if (uaddr->sa_family == AF_UNSPEC) {
> > -			err = mptcp_disconnect(sock->sk, flags);
> > -			sock->state = err ? SS_DISCONNECTING : SS_UNCONNECTED;
> > -			goto unlock;
> > -		}
> > -	}
> > -
> > -	if (sock->state != SS_UNCONNECTED && msk->subflow) {
> > -		/* pending connection or invalid state, let existing subflow
> > -		 * cope with that
> > -		 */
> > -		ssock = msk->subflow;
> > -		goto do_connect;
> > -	}
> 
> I don't see a similar chunk of code added back in elsewhere in this patch.
> 
> After looking at the commit message for 41be81a8d3d0 ("mptcp: fix 
> unblocking connect()"), I'm not sure the nonblocking connect is still 
> handled? I see where __inet_stream_connect() will not call mptcp_connect() 
> with SS_UNCONNECTED, but it's not obvious why it's no longer important to 
> handle this scenario any more. Could you clarify the reasoning for this in 
> the commit message if the nonblocking connect is handled some other way 
> now?

Thanks for noticing this, I underlooked this chunk of code. It looks
unblocking connect still works, as the relevant pktdrill tests pass,
and I'm adding a new one adding more coverage of the relevant code
path.

The main point is that before this commit we called (indirectly) into
inet_stream_connect() to update the 'struct socket' state on both the
subflow and the msk. That did _not_ cause a call to the subflow
tcp_v{4,6}_connect() callback.

Now the msk' struct socket is update directly by
mptcp_stream_connect/inet_stream_connect. We possibly miss/lack the
state update on the ssk struct socket, but it looks harmless.

Anyhow I'll send a v2 with more comments and the change mentioned
above.

Thank,

Paolo

Re: [PATCH mptcp-next] mptcp: factor out mptcp_connect()

[Mat Martineau]

On Tue, 4 Oct 2022, Paolo Abeni wrote:

> On Mon, 2022-10-03 at 15:30 -0700, Mat Martineau wrote:
>> On Mon, 3 Oct 2022, Paolo Abeni wrote:
>>
>>> The current MPTCP connect implementation duplicates a bit of inet
>>> code and does not use nor provide a struct proto->connect callback,
>>> which in turn will not fit the upcoming fastopen implementation.
>>>
>>> Refactor such implementation to use the common helper, moving the
>>> MPTCP-specific bits into mptcp_connect(). Additionally, avoid an
>>> indirect call to the subflow connect callback.
>>>
>>> Note that the fastopen call-path invokes mptcp_connect() while already
>>> holding the subflow socket lock. Explicitly keep track of such path
>>> via a new MPTCP-level flag and handle the locking accordingly.
>>>
>>> Additionally, track the connect flags in a new msk field to allow
>>> propagating them to the subflow inet_stream_connect call.
>>>
>>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>>> ---
>>> net/mptcp/protocol.c | 128 +++++++++++++++++++++----------------------
>>> net/mptcp/protocol.h |   4 +-
>>> 2 files changed, 65 insertions(+), 67 deletions(-)
>>>
>>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>>> index 8feb684408f7..6245248d0182 100644
>>> --- a/net/mptcp/protocol.c
>>> +++ b/net/mptcp/protocol.c
>>> @@ -1689,7 +1689,10 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
>>>
>>> 		lock_sock(ssk);
>>>
>>> +		msk->connect_flags = (msg->msg_flags & MSG_DONTWAIT) ? O_NONBLOCK : 0;
>>> +		msk->is_sendmsg = 1;
>>> 		ret = tcp_sendmsg_fastopen(ssk, msg, &copied_syn, len, NULL);
>>> +		msk->is_sendmsg = 0;
>>
>> I'm not excited about passing the flags this way (new msk members), but it
>> seems better than changing the parameters of (struct proto_ops)->connect
>> (and every connect function it's used with). So, I'm ok with the above
>> change.
>
> I agree it's not very nice. Perhpas it can be made less ugly moving the
>
> 		msk->is_sendmsg = 0;
>
> in mptcp_stream_connect()?
>

I hadn't thought to look at that - but after some consideration, I think 
it fits here (in mptcp_sendmsg()) better, since fastopen is the "unusual" 
case.

>>> 		copied += copied_syn;
>>> 		if (ret == -EINPROGRESS && copied_syn > 0) {
>>> 			/* reflect the new state on the MPTCP socket */
>>> @@ -3506,10 +3509,65 @@ static int mptcp_ioctl(struct sock *sk, int cmd, unsigned long arg)
>>> 	return put_user(answ, (int __user *)arg);
>>> }
>>>
>>> +static void mptcp_subflow_early_fallback(struct mptcp_sock *msk,
>>> +					 struct mptcp_subflow_context *subflow)
>>> +{
>>> +	subflow->request_mptcp = 0;
>>> +	__mptcp_do_fallback(msk);
>>> +}
>>> +
>>> +static int mptcp_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len)
>>> +{
>>> +	struct mptcp_subflow_context *subflow;
>>> +	struct mptcp_sock *msk = mptcp_sk(sk);
>>> +	struct socket *ssock;
>>> +	int err = -EINVAL;
>>> +
>>> +	ssock = __mptcp_nmpc_socket(msk);
>>> +	if (!ssock)
>>> +		return -EINVAL;
>>> +
>>> +	mptcp_token_destroy(msk);
>>> +	inet_sk_state_store(sk, TCP_SYN_SENT);
>>> +	subflow = mptcp_subflow_ctx(ssock->sk);
>>> +#ifdef CONFIG_TCP_MD5SIG
>>> +	/* no MPTCP if MD5SIG is enabled on this socket or we may run out of
>>> +	 * TCP option space.
>>> +	 */
>>> +	if (rcu_access_pointer(tcp_sk(ssock->sk)->md5sig_info))
>>> +		mptcp_subflow_early_fallback(msk, subflow);
>>> +#endif
>>> +	if (subflow->request_mptcp && mptcp_token_new_connect(ssock->sk)) {
>>> +		MPTCP_INC_STATS(sock_net(ssock->sk), MPTCP_MIB_TOKENFALLBACKINIT);
>>> +		mptcp_subflow_early_fallback(msk, subflow);
>>> +	}
>>> +	if (likely(!__mptcp_check_fallback(msk)))
>>> +		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPCAPABLEACTIVE);
>>> +
>>> +	/* if reaching here via the fastopen/sendmsg path, the caller already
>>> +	 * acquired the subflow socket lock, too.
>>> +	 */
>>> +	if (msk->is_sendmsg)
>>> +		err = __inet_stream_connect(ssock, uaddr, addr_len, msk->connect_flags, 1);
>>> +	else
>>> +		err = inet_stream_connect(ssock, uaddr, addr_len, msk->connect_flags);
>>> +	inet_sk(sk)->defer_connect = inet_sk(ssock->sk)->defer_connect;
>>> +
>>> +	/* on successful connect, the msk state will be moved to established by
>>> +	 * subflow_finish_connect()
>>> +	 */
>>> +	if (!err || err == -EINPROGRESS)
>>> +		mptcp_copy_inaddrs(sk, ssock->sk);
>>> +	else
>>> +		inet_sk_state_store(sk, inet_sk_state_load(ssock->sk));
>>> +	return err;
>>> +}
>>> +
>>> static struct proto mptcp_prot = {
>>> 	.name		= "MPTCP",
>>> 	.owner		= THIS_MODULE,
>>> 	.init		= mptcp_init_sock,
>>> +	.connect	= mptcp_connect,
>>> 	.disconnect	= mptcp_disconnect,
>>> 	.close		= mptcp_close,
>>> 	.accept		= mptcp_accept,
>>> @@ -3561,78 +3619,16 @@ static int mptcp_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
>>> 	return err;
>>> }
>>>
>>> -static void mptcp_subflow_early_fallback(struct mptcp_sock *msk,
>>> -					 struct mptcp_subflow_context *subflow)
>>> -{
>>> -	subflow->request_mptcp = 0;
>>> -	__mptcp_do_fallback(msk);
>>> -}
>>> -
>>> static int mptcp_stream_connect(struct socket *sock, struct sockaddr *uaddr,
>>> 				int addr_len, int flags)
>>> {
>>> -	struct mptcp_sock *msk = mptcp_sk(sock->sk);
>>> -	struct mptcp_subflow_context *subflow;
>>> -	struct socket *ssock;
>>> -	int err = -EINVAL;
>>> +	int ret;
>>>
>>> 	lock_sock(sock->sk);
>>> -	if (uaddr) {
>>> -		if (addr_len < sizeof(uaddr->sa_family))
>>> -			goto unlock;
>>> -
>>> -		if (uaddr->sa_family == AF_UNSPEC) {
>>> -			err = mptcp_disconnect(sock->sk, flags);
>>> -			sock->state = err ? SS_DISCONNECTING : SS_UNCONNECTED;
>>> -			goto unlock;
>>> -		}
>>> -	}
>>> -
>>> -	if (sock->state != SS_UNCONNECTED && msk->subflow) {
>>> -		/* pending connection or invalid state, let existing subflow
>>> -		 * cope with that
>>> -		 */
>>> -		ssock = msk->subflow;
>>> -		goto do_connect;
>>> -	}
>>
>> I don't see a similar chunk of code added back in elsewhere in this patch.
>>
>> After looking at the commit message for 41be81a8d3d0 ("mptcp: fix
>> unblocking connect()"), I'm not sure the nonblocking connect is still
>> handled? I see where __inet_stream_connect() will not call mptcp_connect()
>> with SS_UNCONNECTED, but it's not obvious why it's no longer important to
>> handle this scenario any more. Could you clarify the reasoning for this in
>> the commit message if the nonblocking connect is handled some other way
>> now?
>
> Thanks for noticing this, I underlooked this chunk of code. It looks
> unblocking connect still works, as the relevant pktdrill tests pass,
> and I'm adding a new one adding more coverage of the relevant code
> path.
>
> The main point is that before this commit we called (indirectly) into
> inet_stream_connect() to update the 'struct socket' state on both the
> subflow and the msk. That did _not_ cause a call to the subflow
> tcp_v{4,6}_connect() callback.
>
> Now the msk' struct socket is update directly by
> mptcp_stream_connect/inet_stream_connect. We possibly miss/lack the
> state update on the ssk struct socket, but it looks harmless.
>

That explanation helps, thank you!


--
Mat Martineau
Intel

Re: mptcp: factor out mptcp_connect(): Tests Results

[MPTCP CI]

Hi Paolo,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

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

- KVM Validation: debug:
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/6206662674481152
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6206662674481152/summary/summary.txt

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/56fd8c59c652


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-debug

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (Tessares)

[PATCH mptcp-next v2] mptcp: factor out mptcp_connect()

[Paolo Abeni]

The current MPTCP connect implementation duplicates a bit of inet
code and does not use nor provide a struct proto->connect callback,
which in turn will not fit the upcoming fastopen implementation.

Refactor such implementation to use the common helper, moving the
MPTCP-specific bits into mptcp_connect(). Additionally, avoid an
indirect call to the subflow connect callback.

Note that the fastopen call-path invokes mptcp_connect() while already
holding the subflow socket lock. Explicitly keep track of such path
via a new MPTCP-level flag and handle the locking accordingly.

Additionally, track the connect flags in a new msk field to allow
propagating them to the subflow inet_stream_connect call.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
v1 -> v2:
 - properly update msk state on unblocking connect
---
 net/mptcp/protocol.c | 136 ++++++++++++++++++++++---------------------
 net/mptcp/protocol.h |   4 +-
 2 files changed, 73 insertions(+), 67 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 8feb684408f7..1aa940928b4f 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1689,7 +1689,10 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 
 		lock_sock(ssk);
 
+		msk->connect_flags = (msg->msg_flags & MSG_DONTWAIT) ? O_NONBLOCK : 0;
+		msk->is_sendmsg = 1;
 		ret = tcp_sendmsg_fastopen(ssk, msg, &copied_syn, len, NULL);
+		msk->is_sendmsg = 0;
 		copied += copied_syn;
 		if (ret == -EINPROGRESS && copied_syn > 0) {
 			/* reflect the new state on the MPTCP socket */
@@ -3506,10 +3509,73 @@ static int mptcp_ioctl(struct sock *sk, int cmd, unsigned long arg)
 	return put_user(answ, (int __user *)arg);
 }
 
+static void mptcp_subflow_early_fallback(struct mptcp_sock *msk,
+					 struct mptcp_subflow_context *subflow)
+{
+	subflow->request_mptcp = 0;
+	__mptcp_do_fallback(msk);
+}
+
+static int mptcp_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len)
+{
+	struct mptcp_subflow_context *subflow;
+	struct mptcp_sock *msk = mptcp_sk(sk);
+	struct socket *ssock;
+	int err = -EINVAL;
+
+	ssock = __mptcp_nmpc_socket(msk);
+	if (!ssock)
+		return -EINVAL;
+
+	mptcp_token_destroy(msk);
+	inet_sk_state_store(sk, TCP_SYN_SENT);
+	subflow = mptcp_subflow_ctx(ssock->sk);
+#ifdef CONFIG_TCP_MD5SIG
+	/* no MPTCP if MD5SIG is enabled on this socket or we may run out of
+	 * TCP option space.
+	 */
+	if (rcu_access_pointer(tcp_sk(ssock->sk)->md5sig_info))
+		mptcp_subflow_early_fallback(msk, subflow);
+#endif
+	if (subflow->request_mptcp && mptcp_token_new_connect(ssock->sk)) {
+		MPTCP_INC_STATS(sock_net(ssock->sk), MPTCP_MIB_TOKENFALLBACKINIT);
+		mptcp_subflow_early_fallback(msk, subflow);
+	}
+	if (likely(!__mptcp_check_fallback(msk)))
+		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPCAPABLEACTIVE);
+
+	/* if reaching here via the fastopen/sendmsg path, the caller already
+	 * acquired the subflow socket lock, too.
+	 */
+	if (msk->is_sendmsg)
+		err = __inet_stream_connect(ssock, uaddr, addr_len, msk->connect_flags, 1);
+	else
+		err = inet_stream_connect(ssock, uaddr, addr_len, msk->connect_flags);
+	inet_sk(sk)->defer_connect = inet_sk(ssock->sk)->defer_connect;
+
+	/* on successful connect, the msk state will be moved to established by
+	 * subflow_finish_connect()
+	 */
+	if (unlikely(err && err != -EINPROGRESS)) {
+		inet_sk_state_store(sk, inet_sk_state_load(ssock->sk));
+		return err;
+	}
+
+	mptcp_copy_inaddrs(sk, ssock->sk);
+
+	/* unblocking connect, mptcp-level inet_stream_connect will error out
+	 * without changing the socket state, update it here.
+	 */
+	if (err == -EINPROGRESS)
+		sk->sk_socket->state = ssock->state;
+	return err;
+}
+
 static struct proto mptcp_prot = {
 	.name		= "MPTCP",
 	.owner		= THIS_MODULE,
 	.init		= mptcp_init_sock,
+	.connect	= mptcp_connect,
 	.disconnect	= mptcp_disconnect,
 	.close		= mptcp_close,
 	.accept		= mptcp_accept,
@@ -3561,78 +3627,16 @@ static int mptcp_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
 	return err;
 }
 
-static void mptcp_subflow_early_fallback(struct mptcp_sock *msk,
-					 struct mptcp_subflow_context *subflow)
-{
-	subflow->request_mptcp = 0;
-	__mptcp_do_fallback(msk);
-}
-
 static int mptcp_stream_connect(struct socket *sock, struct sockaddr *uaddr,
 				int addr_len, int flags)
 {
-	struct mptcp_sock *msk = mptcp_sk(sock->sk);
-	struct mptcp_subflow_context *subflow;
-	struct socket *ssock;
-	int err = -EINVAL;
+	int ret;
 
 	lock_sock(sock->sk);
-	if (uaddr) {
-		if (addr_len < sizeof(uaddr->sa_family))
-			goto unlock;
-
-		if (uaddr->sa_family == AF_UNSPEC) {
-			err = mptcp_disconnect(sock->sk, flags);
-			sock->state = err ? SS_DISCONNECTING : SS_UNCONNECTED;
-			goto unlock;
-		}
-	}
-
-	if (sock->state != SS_UNCONNECTED && msk->subflow) {
-		/* pending connection or invalid state, let existing subflow
-		 * cope with that
-		 */
-		ssock = msk->subflow;
-		goto do_connect;
-	}
-
-	ssock = __mptcp_nmpc_socket(msk);
-	if (!ssock)
-		goto unlock;
-
-	mptcp_token_destroy(msk);
-	inet_sk_state_store(sock->sk, TCP_SYN_SENT);
-	subflow = mptcp_subflow_ctx(ssock->sk);
-#ifdef CONFIG_TCP_MD5SIG
-	/* no MPTCP if MD5SIG is enabled on this socket or we may run out of
-	 * TCP option space.
-	 */
-	if (rcu_access_pointer(tcp_sk(ssock->sk)->md5sig_info))
-		mptcp_subflow_early_fallback(msk, subflow);
-#endif
-	if (subflow->request_mptcp && mptcp_token_new_connect(ssock->sk)) {
-		MPTCP_INC_STATS(sock_net(ssock->sk), MPTCP_MIB_TOKENFALLBACKINIT);
-		mptcp_subflow_early_fallback(msk, subflow);
-	}
-	if (likely(!__mptcp_check_fallback(msk)))
-		MPTCP_INC_STATS(sock_net(sock->sk), MPTCP_MIB_MPCAPABLEACTIVE);
-
-do_connect:
-	err = ssock->ops->connect(ssock, uaddr, addr_len, flags);
-	inet_sk(sock->sk)->defer_connect = inet_sk(ssock->sk)->defer_connect;
-	sock->state = ssock->state;
-
-	/* on successful connect, the msk state will be moved to established by
-	 * subflow_finish_connect()
-	 */
-	if (!err || err == -EINPROGRESS)
-		mptcp_copy_inaddrs(sock->sk, ssock->sk);
-	else
-		inet_sk_state_store(sock->sk, inet_sk_state_load(ssock->sk));
-
-unlock:
+	mptcp_sk(sock->sk)->connect_flags = flags;
+	ret = __inet_stream_connect(sock, uaddr, addr_len, flags, 0);
 	release_sock(sock->sk);
-	return err;
+	return ret;
 }
 
 static int mptcp_listen(struct socket *sock, int backlog)
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 93c535440a5c..18f866b1afda 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -285,7 +285,9 @@ struct mptcp_sock {
 	u8		mpc_endpoint_id;
 	u8		recvmsg_inq:1,
 			cork:1,
-			nodelay:1;
+			nodelay:1,
+			is_sendmsg:1;
+	int		connect_flags;
 	struct work_struct work;
 	struct sk_buff  *ooo_last_skb;
 	struct rb_root  out_of_order_queue;
-- 
2.37.3

Re: mptcp: factor out mptcp_connect(): Tests Results

[MPTCP CI]

Hi Paolo,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

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

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

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/9d11d43b8e21


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-debug

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (Tessares)