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

[Mat Martineau <mathew.j.martineau@linux.intel.com>]

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