net/mptcp/protocol.c | 128 +++++++++++++++++++++---------------------- net/mptcp/protocol.h | 4 +- 2 files changed, 65 insertions(+), 67 deletions(-)
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.3On 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
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
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
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.
© 2016 - 2025 Red Hat, Inc.