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

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

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