1. Patchew
  2. MPTCP
  3. [PATCH mptcp-next 0/9] mptcp: refactor first subflow init
  4. View patch

[PATCH mptcp-next 6/9] mptcp: move first subflow allocation at mpc access time

Paolo Abeni posted 9 patches 1 year, 7 months ago
Maintainers: Mat Martineau <mathew.j.martineau@linux.intel.com>, Matthieu Baerts <matthieu.baerts@tessares.net>, "David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>
There is a newer version of this series
[PATCH mptcp-next 6/9] mptcp: move first subflow allocation at mpc access time
Posted by Paolo Abeni 1 year, 7 months ago 
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>
---
 net/mptcp/pm_netlink.c |  4 +--
 net/mptcp/protocol.c   | 57 +++++++++++++++++++++++++++---------------
 net/mptcp/protocol.h   |  2 +-
 net/mptcp/sockopt.c    | 18 ++++++-------
 4 files changed, 49 insertions(+), 32 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index e82a112b8779..290f88fffe5f 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1014,8 +1014,8 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
 		return -EINVAL;
 
 	ssock = __mptcp_nmpc_socket(msk);
-	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..1b93624db62d 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;
 
@@ -3592,8 +3609,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 +3698,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 +3735,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 9986681aaf40..022a6cad00c1 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);
@@ -764,8 +764,8 @@ static int mptcp_setsockopt_first_sf_only(struct mptcp_sock *msk, int level, int
 
 	/* Limit to first subflow, before the connection establishment */
 	sock = __mptcp_nmpc_socket(msk);
-	if (!sock)
-		return -EINVAL;
+	if (IS_ERR(sock))
+		return PTR_ERR(sock);
 
 	return tcp_setsockopt(sock->sk, level, optname, optval, optlen);
 }
@@ -865,7 +865,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.38.1
Re: [PATCH mptcp-next 6/9] mptcp: move first subflow allocation at mpc access time
Posted by Paolo Abeni 1 year, 7 months ago 
On Fri, 2023-01-13 at 19:20 +0100, 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.
> 
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
>  net/mptcp/pm_netlink.c |  4 +--
>  net/mptcp/protocol.c   | 57 +++++++++++++++++++++++++++---------------
>  net/mptcp/protocol.h   |  2 +-
>  net/mptcp/sockopt.c    | 18 ++++++-------
>  4 files changed, 49 insertions(+), 32 deletions(-)
> 
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index e82a112b8779..290f88fffe5f 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -1014,8 +1014,8 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
>  		return -EINVAL;
>  
>  	ssock = __mptcp_nmpc_socket(msk);
> -	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..1b93624db62d 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;
>  
> @@ -3592,8 +3609,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 +3698,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 +3735,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 9986681aaf40..022a6cad00c1 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);
> @@ -764,8 +764,8 @@ static int mptcp_setsockopt_first_sf_only(struct mptcp_sock *msk, int level, int
>  
>  	/* Limit to first subflow, before the connection establishment */
>  	sock = __mptcp_nmpc_socket(msk);
> -	if (!sock)
> -		return -EINVAL;
> +	if (IS_ERR(sock))
> +		return PTR_ERR(sock);
>  
>  	return tcp_setsockopt(sock->sk, level, optname, optval, optlen);
>  }
> @@ -865,7 +865,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);

Self-nack. While rebasing the LSM/selinux patches on top of this
series, I noticed I actually forgot (ahem ...) to remove the first
subflow initialization from mptcp_init_sock(). The relative low number
of issues found while testing should have warned me...

I'm updating the series and I'll post after some more testings.

Cheers,

Paolo

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.