In the long run this will simplify the mptcp code and will
allow for more consistent behavior. Move the first subflow
allocation out of the sock->init ops into the __mptcp_nmpc_socket()
helper.

Since the first subflow creation can now happen after the first
setsockopt() we additionally need to invoke mptcp_sockopt_sync()
on it.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---

v1 -> v2:
 - really remove subflow creation from init() (!!!)
---
 net/mptcp/pm_netlink.c |  4 +--
 net/mptcp/protocol.c   | 61 +++++++++++++++++++++++++-----------------
 net/mptcp/protocol.h   |  2 +-
 net/mptcp/sockopt.c    | 20 +++++++-------
 4 files changed, 51 insertions(+), 36 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 155916174841..d1d859517d91 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1016,8 +1016,8 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
 	lock_sock(newsk);
 	ssock = __mptcp_nmpc_socket(mptcp_sk(newsk));
 	release_sock(newsk);
-	if (!ssock)
-		return -EINVAL;
+	if (IS_ERR(ssock))
+		return PTR_ERR(ssock);
 
 	mptcp_info2sockaddr(&entry->addr, &addr, entry->addr.family);
 #if IS_ENABLED(CONFIG_MPTCP_IPV6)
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 9c4c729bf271..4f7a71561efd 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -49,18 +49,6 @@ static void __mptcp_check_send_data_fin(struct sock *sk);
 DEFINE_PER_CPU(struct mptcp_delegated_action, mptcp_delegated_actions);
 static struct net_device mptcp_napi_dev;
 
-/* If msk has an initial subflow socket, and the MP_CAPABLE handshake has not
- * completed yet or has failed, return the subflow socket.
- * Otherwise return NULL.
- */
-struct socket *__mptcp_nmpc_socket(const struct mptcp_sock *msk)
-{
-	if (!msk->subflow || READ_ONCE(msk->can_ack))
-		return NULL;
-
-	return msk->subflow;
-}
-
 /* Returns end sequence number of the receiver's advertised window */
 static u64 mptcp_wnd_end(const struct mptcp_sock *msk)
 {
@@ -116,6 +104,31 @@ static int __mptcp_socket_create(struct mptcp_sock *msk)
 	return 0;
 }
 
+/* Returns the first subflow socket if available and the MPC
+ * handshake is not started yet.
+ */
+struct socket *__mptcp_nmpc_socket(struct mptcp_sock *msk)
+{
+	struct sock *sk = (struct sock *)msk;
+	int ret;
+
+	if (!((1 << sk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN)))
+		return ERR_PTR(-EINVAL);
+
+	if (!msk->subflow) {
+		if (msk->first)
+			return ERR_PTR(-EINVAL);
+
+		ret = __mptcp_socket_create(msk);
+		if (ret)
+			return ERR_PTR(ret);
+
+		mptcp_sockopt_sync(msk, msk->first);
+	}
+
+	return msk->subflow;
+}
+
 static void mptcp_drop(struct sock *sk, struct sk_buff *skb)
 {
 	sk_drops_add(sk, skb);
@@ -1699,6 +1712,7 @@ static int mptcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg,
 {
 	unsigned int saved_flags = msg->msg_flags;
 	struct mptcp_sock *msk = mptcp_sk(sk);
+	struct socket *ssock;
 	struct sock *ssk;
 	int ret;
 
@@ -1708,8 +1722,11 @@ static int mptcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg,
 	 * Since the defer_connect flag is cleared after the first succsful
 	 * fastopen attempt, no need to check for additional subflow status.
 	 */
-	if (msg->msg_flags & MSG_FASTOPEN && !__mptcp_nmpc_socket(msk))
-		return -EINVAL;
+	if (msg->msg_flags & MSG_FASTOPEN) {
+		ssock = __mptcp_nmpc_socket(msk);
+		if (IS_ERR(ssock))
+			return PTR_ERR(ssock);
+	}
 	if (!msk->first)
 		return -EINVAL;
 
@@ -2768,10 +2785,6 @@ static int mptcp_init_sock(struct sock *sk)
 	if (unlikely(!net->mib.mptcp_statistics) && !mptcp_mib_alloc(net))
 		return -ENOMEM;
 
-	ret = __mptcp_socket_create(mptcp_sk(sk));
-	if (ret)
-		return ret;
-
 	ret = mptcp_init_sched(mptcp_sk(sk),
 			       mptcp_sched_find(mptcp_get_scheduler(net)));
 	if (ret)
@@ -3592,8 +3605,8 @@ static int mptcp_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len)
 	int err = -EINVAL;
 
 	ssock = __mptcp_nmpc_socket(msk);
-	if (!ssock)
-		return -EINVAL;
+	if (IS_ERR(ssock))
+		return PTR_ERR(ssock);
 
 	mptcp_token_destroy(msk);
 	inet_sk_state_store(sk, TCP_SYN_SENT);
@@ -3681,8 +3694,8 @@ static int mptcp_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
 
 	lock_sock(sock->sk);
 	ssock = __mptcp_nmpc_socket(msk);
-	if (!ssock) {
-		err = -EINVAL;
+	if (IS_ERR(ssock)) {
+		err = PTR_ERR(ssock);
 		goto unlock;
 	}
 
@@ -3718,8 +3731,8 @@ static int mptcp_listen(struct socket *sock, int backlog)
 
 	lock_sock(sk);
 	ssock = __mptcp_nmpc_socket(msk);
-	if (!ssock) {
-		err = -EINVAL;
+	if (IS_ERR(ssock)) {
+		err = PTR_ERR(ssock);
 		goto unlock;
 	}
 
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 5f1a30959b5c..3a055438c65e 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -632,7 +632,7 @@ void __mptcp_subflow_send_ack(struct sock *ssk);
 void mptcp_subflow_reset(struct sock *ssk);
 void mptcp_subflow_queue_clean(struct sock *sk, struct sock *ssk);
 void mptcp_sock_graft(struct sock *sk, struct socket *parent);
-struct socket *__mptcp_nmpc_socket(const struct mptcp_sock *msk);
+struct socket *__mptcp_nmpc_socket(struct mptcp_sock *msk);
 bool __mptcp_close(struct sock *sk, long timeout);
 void mptcp_cancel_work(struct sock *sk);
 void mptcp_set_owner_r(struct sk_buff *skb, struct sock *sk);
diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
index 8a9656248b0f..9bddae9c5c58 100644
--- a/net/mptcp/sockopt.c
+++ b/net/mptcp/sockopt.c
@@ -301,9 +301,9 @@ static int mptcp_setsockopt_sol_socket(struct mptcp_sock *msk, int optname,
 	case SO_BINDTOIFINDEX:
 		lock_sock(sk);
 		ssock = __mptcp_nmpc_socket(msk);
-		if (!ssock) {
+		if (IS_ERR(ssock)) {
 			release_sock(sk);
-			return -EINVAL;
+			return PTR_ERR(ssock);
 		}
 
 		ret = sock_setsockopt(ssock, SOL_SOCKET, optname, optval, optlen);
@@ -396,9 +396,9 @@ static int mptcp_setsockopt_v6(struct mptcp_sock *msk, int optname,
 	case IPV6_FREEBIND:
 		lock_sock(sk);
 		ssock = __mptcp_nmpc_socket(msk);
-		if (!ssock) {
+		if (IS_ERR(ssock)) {
 			release_sock(sk);
-			return -EINVAL;
+			return PTR_ERR(ssock);
 		}
 
 		ret = tcp_setsockopt(ssock->sk, SOL_IPV6, optname, optval, optlen);
@@ -693,9 +693,9 @@ static int mptcp_setsockopt_sol_ip_set_transparent(struct mptcp_sock *msk, int o
 	lock_sock(sk);
 
 	ssock = __mptcp_nmpc_socket(msk);
-	if (!ssock) {
+	if (IS_ERR(ssock)) {
 		release_sock(sk);
-		return -EINVAL;
+		return PTR_ERR(ssock);
 	}
 
 	issk = inet_sk(ssock->sk);
@@ -762,13 +762,15 @@ static int mptcp_setsockopt_first_sf_only(struct mptcp_sock *msk, int level, int
 {
 	struct sock *sk = (struct sock *)msk;
 	struct socket *sock;
-	int ret = -EINVAL;
+	int ret;
 
 	/* Limit to first subflow, before the connection establishment */
 	lock_sock(sk);
 	sock = __mptcp_nmpc_socket(msk);
-	if (!sock)
+	if (IS_ERR(sock)) {
+		ret = PTR_ERR(sock);
 		goto unlock;
+	}
 
 	ret = tcp_setsockopt(sock->sk, level, optname, optval, optlen);
 
@@ -872,7 +874,7 @@ static int mptcp_getsockopt_first_sf_only(struct mptcp_sock *msk, int level, int
 	}
 
 	ssock = __mptcp_nmpc_socket(msk);
-	if (!ssock)
+	if (IS_ERR(ssock))
 		goto out;
 
 	ret = tcp_getsockopt(ssock->sk, level, optname, optval, optlen);
-- 
2.39.0

Hi Paolo,

On 17/01/2023 08:36, Paolo Abeni wrote:
> In the long run this will simplify the mptcp code and will
> allow for more consistent behavior. Move the first subflow
> allocation out of the sock->init ops into the __mptcp_nmpc_socket()
> helper.
> 
> Since the first subflow creation can now happen after the first
> setsockopt() we additionally need to invoke mptcp_sockopt_sync()
> on it.

Thank you for this improvement! I have some questions below if you don't
mind :)

(...)

> @@ -116,6 +104,31 @@ static int __mptcp_socket_create(struct mptcp_sock *msk)
>  	return 0;
>  }
>  
> +/* Returns the first subflow socket if available and the MPC
> + * handshake is not started yet.
> + */
> +struct socket *__mptcp_nmpc_socket(struct mptcp_sock *msk)
> +{
> +	struct sock *sk = (struct sock *)msk;
> +	int ret;
> +
> +	if (!((1 << sk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN)))
> +		return ERR_PTR(-EINVAL);
> +
> +	if (!msk->subflow) {
> +		if (msk->first)
> +			return ERR_PTR(-EINVAL);
> +
> +		ret = __mptcp_socket_create(msk);
> +		if (ret)
> +			return ERR_PTR(ret);
> +
> +		mptcp_sockopt_sync(msk, msk->first);

Do you think the function should be renamed? Maybe not but it looks
strange to me to create the socket and do the sync here with this name.
Maybe it is just me but with this name, I would expect it just to
returns the first subflow or NULL, but not create a socket if not, no?

Mainly to make it clear a "[gs]etsockopt()" before the MPC will create a
new socket.

> +	}
> +
> +	return msk->subflow;
> +}
> +
>  static void mptcp_drop(struct sock *sk, struct sk_buff *skb)
>  {
>  	sk_drops_add(sk, skb);
> @@ -1699,6 +1712,7 @@ static int mptcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg,
>  {
>  	unsigned int saved_flags = msg->msg_flags;
>  	struct mptcp_sock *msk = mptcp_sk(sk);
> +	struct socket *ssock;
>  	struct sock *ssk;
>  	int ret;
>  
> @@ -1708,8 +1722,11 @@ static int mptcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg,
>  	 * Since the defer_connect flag is cleared after the first succsful
>  	 * fastopen attempt, no need to check for additional subflow status.
>  	 */
> -	if (msg->msg_flags & MSG_FASTOPEN && !__mptcp_nmpc_socket(msk))
> -		return -EINVAL;
> +	if (msg->msg_flags & MSG_FASTOPEN) {
> +		ssock = __mptcp_nmpc_socket(msk);
> +		if (IS_ERR(ssock))
> +			return PTR_ERR(ssock);> +	}

Here 'ssock' could be declared in this 'if' section: I guess it doesn't
really matter but out of curiosity, what's the code style to apply here?
Fine where it is or moved only where needed?

>  	if (!msk->first)
>  		return -EINVAL;
>  
> @@ -2768,10 +2785,6 @@ static int mptcp_init_sock(struct sock *sk)
>  	if (unlikely(!net->mib.mptcp_statistics) && !mptcp_mib_alloc(net))
>  		return -ENOMEM;
>  
> -	ret = __mptcp_socket_create(mptcp_sk(sk));
> -	if (ret)
> -		return ret;
> -
>  	ret = mptcp_init_sched(mptcp_sk(sk),
>  			       mptcp_sched_find(mptcp_get_scheduler(net)));
>  	if (ret)
> @@ -3592,8 +3605,8 @@ static int mptcp_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len)
>  	int err = -EINVAL;
>  
>  	ssock = __mptcp_nmpc_socket(msk);
> -	if (!ssock)
> -		return -EINVAL;
> +	if (IS_ERR(ssock))
> +		return PTR_ERR(ssock);
>  
>  	mptcp_token_destroy(msk);
>  	inet_sk_state_store(sk, TCP_SYN_SENT);
> @@ -3681,8 +3694,8 @@ static int mptcp_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
>  
>  	lock_sock(sock->sk);
>  	ssock = __mptcp_nmpc_socket(msk);
> -	if (!ssock) {
> -		err = -EINVAL;
> +	if (IS_ERR(ssock)) {
> +		err = PTR_ERR(ssock);
>  		goto unlock;
>  	}
>  
> @@ -3718,8 +3731,8 @@ static int mptcp_listen(struct socket *sock, int backlog)
>  
>  	lock_sock(sk);
>  	ssock = __mptcp_nmpc_socket(msk);
> -	if (!ssock) {
> -		err = -EINVAL;
> +	if (IS_ERR(ssock)) {
> +		err = PTR_ERR(ssock);
>  		goto unlock;
>  	}
>  
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 5f1a30959b5c..3a055438c65e 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -632,7 +632,7 @@ void __mptcp_subflow_send_ack(struct sock *ssk);
>  void mptcp_subflow_reset(struct sock *ssk);
>  void mptcp_subflow_queue_clean(struct sock *sk, struct sock *ssk);
>  void mptcp_sock_graft(struct sock *sk, struct socket *parent);
> -struct socket *__mptcp_nmpc_socket(const struct mptcp_sock *msk);
> +struct socket *__mptcp_nmpc_socket(struct mptcp_sock *msk);
>  bool __mptcp_close(struct sock *sk, long timeout);
>  void mptcp_cancel_work(struct sock *sk);
>  void mptcp_set_owner_r(struct sk_buff *skb, struct sock *sk);
> diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
> index 8a9656248b0f..9bddae9c5c58 100644
> --- a/net/mptcp/sockopt.c
> +++ b/net/mptcp/sockopt.c
> @@ -301,9 +301,9 @@ static int mptcp_setsockopt_sol_socket(struct mptcp_sock *msk, int optname,
>  	case SO_BINDTOIFINDEX:
>  		lock_sock(sk);
>  		ssock = __mptcp_nmpc_socket(msk);
> -		if (!ssock) {
> +		if (IS_ERR(ssock)) {
>  			release_sock(sk);
> -			return -EINVAL;
> +			return PTR_ERR(ssock);
>  		}
>  
>  		ret = sock_setsockopt(ssock, SOL_SOCKET, optname, optval, optlen);
> @@ -396,9 +396,9 @@ static int mptcp_setsockopt_v6(struct mptcp_sock *msk, int optname,
>  	case IPV6_FREEBIND:
>  		lock_sock(sk);
>  		ssock = __mptcp_nmpc_socket(msk);
> -		if (!ssock) {
> +		if (IS_ERR(ssock)) {
>  			release_sock(sk);
> -			return -EINVAL;
> +			return PTR_ERR(ssock);
>  		}
>  
>  		ret = tcp_setsockopt(ssock->sk, SOL_IPV6, optname, optval, optlen);
> @@ -693,9 +693,9 @@ static int mptcp_setsockopt_sol_ip_set_transparent(struct mptcp_sock *msk, int o
>  	lock_sock(sk);
>  
>  	ssock = __mptcp_nmpc_socket(msk);
> -	if (!ssock) {
> +	if (IS_ERR(ssock)) {
>  		release_sock(sk);
> -		return -EINVAL;
> +		return PTR_ERR(ssock);
>  	}
>  
>  	issk = inet_sk(ssock->sk);
> @@ -762,13 +762,15 @@ static int mptcp_setsockopt_first_sf_only(struct mptcp_sock *msk, int level, int
>  {
>  	struct sock *sk = (struct sock *)msk;
>  	struct socket *sock;
> -	int ret = -EINVAL;
> +	int ret;
>  
>  	/* Limit to first subflow, before the connection establishment */
>  	lock_sock(sk);
>  	sock = __mptcp_nmpc_socket(msk);
> -	if (!sock)
> +	if (IS_ERR(sock)) {
> +		ret = PTR_ERR(sock);
>  		goto unlock;
> +	}

Just to be sure: here and above the error with __mptcp_nmpc_socket()
should not happen most of the time: only when we reach some limits, e.g.
mem, etc., right?

So it is fine to potentially return a different error than before (and
potentially different than what TCP what doing), is it correct?

>  
>  	ret = tcp_setsockopt(sock->sk, level, optname, optval, optlen);
>  
> @@ -872,7 +874,7 @@ static int mptcp_getsockopt_first_sf_only(struct mptcp_sock *msk, int level, int
>  	}
>  
>  	ssock = __mptcp_nmpc_socket(msk);
> -	if (!ssock)
> +	if (IS_ERR(ssock))
>  		goto out;
>  
>  	ret = tcp_getsockopt(ssock->sk, level, optname, optval, optlen);

Why here you don't assign the error that is returned? (Maybe a comment
should be added?)

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

On Tue, 2023-01-24 at 17:31 +0100, Matthieu Baerts wrote:
> On 17/01/2023 08:36, Paolo Abeni wrote:
> 
> > @@ -116,6 +104,31 @@ static int __mptcp_socket_create(struct mptcp_sock *msk)
> >  	return 0;
> >  }
> >  
> > +/* Returns the first subflow socket if available and the MPC
> > + * handshake is not started yet.
> > + */
> > +struct socket *__mptcp_nmpc_socket(struct mptcp_sock *msk)
> > +{
> > +	struct sock *sk = (struct sock *)msk;
> > +	int ret;
> > +
> > +	if (!((1 << sk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN)))
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	if (!msk->subflow) {
> > +		if (msk->first)
> > +			return ERR_PTR(-EINVAL);
> > +
> > +		ret = __mptcp_socket_create(msk);
> > +		if (ret)
> > +			return ERR_PTR(ret);
> > +
> > +		mptcp_sockopt_sync(msk, msk->first);
> 
> Do you think the function should be renamed? Maybe not but it looks
> strange to me to create the socket and do the sync here with this name.
> Maybe it is just me but with this name, I would expect it just to
> returns the first subflow or NULL, but not create a socket if not, no?
> 
> Mainly to make it clear a "[gs]etsockopt()" before the MPC will create a
> new socket.

I don't have a good alternative name handy. You have good suggestions,
they are more then welcome!

IMHO the function name is not bad: it does not imply allocation, nor
lack of such thing.

> > @@ -1708,8 +1722,11 @@ static int mptcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg,
> >  	 * Since the defer_connect flag is cleared after the first succsful
> >  	 * fastopen attempt, no need to check for additional subflow status.
> >  	 */
> > -	if (msg->msg_flags & MSG_FASTOPEN && !__mptcp_nmpc_socket(msk))
> > -		return -EINVAL;
> > +	if (msg->msg_flags & MSG_FASTOPEN) {
> > +		ssock = __mptcp_nmpc_socket(msk);
> > +		if (IS_ERR(ssock))
> > +			return PTR_ERR(ssock);> +	}
> 
> Here 'ssock' could be declared in this 'if' section: I guess it doesn't
> really matter but out of curiosity, what's the code style to apply here?
> Fine where it is or moved only where needed?

AFAIK there is no strict codying style rule (even if the preference
should go for the smaller scope). I opted for this way just to reduce
the number of added line - otherwise an additional empty line would
have been mandatory.

> > @@ -762,13 +762,15 @@ static int mptcp_setsockopt_first_sf_only(struct mptcp_sock *msk, int level, int
> >  {
> >  	struct sock *sk = (struct sock *)msk;
> >  	struct socket *sock;
> > -	int ret = -EINVAL;
> > +	int ret;
> >  
> >  	/* Limit to first subflow, before the connection establishment */
> >  	lock_sock(sk);
> >  	sock = __mptcp_nmpc_socket(msk);
> > -	if (!sock)
> > +	if (IS_ERR(sock)) {
> > +		ret = PTR_ERR(sock);
> >  		goto unlock;
> > +	}
> 
> Just to be sure: here and above the error with __mptcp_nmpc_socket()
> should not happen most of the time: only when we reach some limits, e.g.
> mem, etc., right?
> 
> So it is fine to potentially return a different error than before (and
> potentially different than what TCP what doing), is it correct?

Well the situation is a bit fuzzy. __mptcp_nmpc_socket() can now return
an additional error code (ENOMEM) compared to the code prior to this
patch. But plain tcp could already return such error for
TCP_FASTOPEN_KEY. For the other sockopt addressed with
mptcp_setsockopt_first_sf_only(), that will introduce a new possible
error code. I think that is fine, the user-space application must not
restrict the set of accepted error code for a given syscall.
 
> >  	ret = tcp_setsockopt(sock->sk, level, optname, optval, optlen);
> >  
> > @@ -872,7 +874,7 @@ static int mptcp_getsockopt_first_sf_only(struct mptcp_sock *msk, int level, int
> >  	}
> >  
> >  	ssock = __mptcp_nmpc_socket(msk);
> > -	if (!ssock)
> > +	if (IS_ERR(ssock))
> >  		goto out;
> >  
> >  	ret = tcp_getsockopt(ssock->sk, level, optname, optval, optlen);
> 
> Why here you don't assign the error that is returned? (Maybe a comment
> should be added?)

I guess it felt strange returning ENOMEM for a get operation. I can
switch to full error reporting if you prefer.

Thanks!

Paolo

Hi Paolo,

Thank you for your reply!

On 26/01/2023 18:49, Paolo Abeni wrote:
> On Tue, 2023-01-24 at 17:31 +0100, Matthieu Baerts wrote:
>> On 17/01/2023 08:36, Paolo Abeni wrote:
>>
>>> @@ -116,6 +104,31 @@ static int __mptcp_socket_create(struct mptcp_sock *msk)
>>>  	return 0;
>>>  }
>>>  
>>> +/* Returns the first subflow socket if available and the MPC
>>> + * handshake is not started yet.
>>> + */
>>> +struct socket *__mptcp_nmpc_socket(struct mptcp_sock *msk)
>>> +{
>>> +	struct sock *sk = (struct sock *)msk;
>>> +	int ret;
>>> +
>>> +	if (!((1 << sk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN)))
>>> +		return ERR_PTR(-EINVAL);
>>> +
>>> +	if (!msk->subflow) {
>>> +		if (msk->first)
>>> +			return ERR_PTR(-EINVAL);
>>> +
>>> +		ret = __mptcp_socket_create(msk);
>>> +		if (ret)
>>> +			return ERR_PTR(ret);
>>> +
>>> +		mptcp_sockopt_sync(msk, msk->first);
>>
>> Do you think the function should be renamed? Maybe not but it looks
>> strange to me to create the socket and do the sync here with this name.
>> Maybe it is just me but with this name, I would expect it just to
>> returns the first subflow or NULL, but not create a socket if not, no?
>>
>> Mainly to make it clear a "[gs]etsockopt()" before the MPC will create a
>> new socket.
> 
> I don't have a good alternative name handy. You have good suggestions,
> they are more then welcome!

Indeed, not easy to find an alternative.

  __mptcp_initial_or_new_sf_socket_before_estab()

might be too long :þ

(but quite explicit :) )

> IMHO the function name is not bad: it does not imply allocation, nor
> lack of such thing.

You are right. If there is no better names, we can keep it like that.

If you want, I can add this line in the comment when applying the patch
(if there is no v3):

  Be careful that a new socket might be created if needed.

>>> @@ -1708,8 +1722,11 @@ static int mptcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg,
>>>  	 * Since the defer_connect flag is cleared after the first succsful
>>>  	 * fastopen attempt, no need to check for additional subflow status.
>>>  	 */
>>> -	if (msg->msg_flags & MSG_FASTOPEN && !__mptcp_nmpc_socket(msk))
>>> -		return -EINVAL;
>>> +	if (msg->msg_flags & MSG_FASTOPEN) {
>>> +		ssock = __mptcp_nmpc_socket(msk);
>>> +		if (IS_ERR(ssock))
>>> +			return PTR_ERR(ssock);> +	}
>>
>> Here 'ssock' could be declared in this 'if' section: I guess it doesn't
>> really matter but out of curiosity, what's the code style to apply here?
>> Fine where it is or moved only where needed?
> 
> AFAIK there is no strict codying style rule (even if the preference
> should go for the smaller scope). I opted for this way just to reduce
> the number of added line - otherwise an additional empty line would
> have been mandatory.

Thank you for the explanation. OK to keep it like that!

>>> @@ -762,13 +762,15 @@ static int mptcp_setsockopt_first_sf_only(struct mptcp_sock *msk, int level, int
>>>  {
>>>  	struct sock *sk = (struct sock *)msk;
>>>  	struct socket *sock;
>>> -	int ret = -EINVAL;
>>> +	int ret;
>>>  
>>>  	/* Limit to first subflow, before the connection establishment */
>>>  	lock_sock(sk);
>>>  	sock = __mptcp_nmpc_socket(msk);
>>> -	if (!sock)
>>> +	if (IS_ERR(sock)) {
>>> +		ret = PTR_ERR(sock);
>>>  		goto unlock;
>>> +	}
>>
>> Just to be sure: here and above the error with __mptcp_nmpc_socket()
>> should not happen most of the time: only when we reach some limits, e.g.
>> mem, etc., right?
>>
>> So it is fine to potentially return a different error than before (and
>> potentially different than what TCP what doing), is it correct?
> 
> Well the situation is a bit fuzzy. __mptcp_nmpc_socket() can now return
> an additional error code (ENOMEM) compared to the code prior to this
> patch. But plain tcp could already return such error for
> TCP_FASTOPEN_KEY. For the other sockopt addressed with
> mptcp_setsockopt_first_sf_only(), that will introduce a new possible
> error code. I think that is fine, the user-space application must not
> restrict the set of accepted error code for a given syscall.

OK so additional error codes should then be rare. Then it is fine. If
they get the ENOMEM now, they would have got it later. If they ignore
it, that's fine. If they consider it as a big issue resulting on not
doing the connect/bind/listen, that's fine as well.

Thank you for having looked!

>>>  	ret = tcp_setsockopt(sock->sk, level, optname, optval, optlen);
>>>  
>>> @@ -872,7 +874,7 @@ static int mptcp_getsockopt_first_sf_only(struct mptcp_sock *msk, int level, int
>>>  	}
>>>  
>>>  	ssock = __mptcp_nmpc_socket(msk);
>>> -	if (!ssock)
>>> +	if (IS_ERR(ssock))
>>>  		goto out;
>>>  
>>>  	ret = tcp_getsockopt(ssock->sk, level, optname, optval, optlen);
>>
>> Why here you don't assign the error that is returned? (Maybe a comment
>> should be added?)
> 
> I guess it felt strange returning ENOMEM for a get operation. I can
> switch to full error reporting if you prefer.

Indeed, you are right.

Do you mind if I add this when applying the patch (if there is no v3):

  /* don't return err linked to the socket creation for a getsockopt */

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net