Re: [RFC PATCH mptcp-next v8 3/7] reuse tcp_sendmsg_fastopen()

[Paolo Abeni <pabeni@redhat.com>]

On Tue, 2022-09-20 at 14:52 +0200, Dmytro Shytyi wrote:
> In the following patches we will reuse modified tcp_sendmsg_fastopen().
> We call it from mptcp_sendmsg().
> 
> Signed-off-by: Dmytro Shytyi <dmytro@shytyi.net>
> ---
>  include/net/tcp.h    |  3 +++
>  net/ipv4/tcp.c       | 18 +++++++++++++-----
>  net/mptcp/protocol.c | 11 +++++++++--
>  3 files changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 735e957f7f4b..a7d49e42470a 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -1754,6 +1754,9 @@ struct sock *tcp_try_fastopen(struct sock *sk, struct sk_buff *skb,
>  			      struct request_sock *req,
>  			      struct tcp_fastopen_cookie *foc,
>  			      const struct dst_entry *dst);
> +int tcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg,
> +			 int *copied, size_t size,
> +			 struct ubuf_info *uarg);
>  void tcp_fastopen_init_key_once(struct net *net);
>  bool tcp_fastopen_cookie_check(struct sock *sk, u16 *mss,
>  			     struct tcp_fastopen_cookie *cookie);
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 9251c99d3cfd..d10a3cdae220 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -280,6 +280,9 @@
>  #include <asm/ioctls.h>
>  #include <net/busy_poll.h>
>  
> +#include <net/mptcp.h>
> +#include "../mptcp/protocol.h"
> +
>  /* Track pending CMSGs. */
>  enum {
>  	TCP_CMSG_INQ = 1,
> @@ -1162,9 +1165,9 @@ void tcp_free_fastopen_req(struct tcp_sock *tp)
>  	}
>  }
>  
> -static int tcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg,
> -				int *copied, size_t size,
> -				struct ubuf_info *uarg)
> +int tcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg,
> +			 int *copied, size_t size,
> +			 struct ubuf_info *uarg)
>  {
>  	struct tcp_sock *tp = tcp_sk(sk);
>  	struct inet_sock *inet = inet_sk(sk);
> @@ -1197,8 +1200,13 @@ static int tcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg,
>  		}
>  	}
>  	flags = (msg->msg_flags & MSG_DONTWAIT) ? O_NONBLOCK : 0;
> -	err = __inet_stream_connect(sk->sk_socket, uaddr,
> -				    msg->msg_namelen, flags, 1);
> +	if (!sk_is_mptcp(sk))
> +		err = __inet_stream_connect(sk->sk_socket, uaddr,
> +					    msg->msg_namelen, flags, 1);
> +	else
> +		err = mptcp_stream_connect(sk->sk_socket, uaddr,
> +					   msg->msg_namelen, msg->msg_flags);


I guess the goal of the above change is let mptcp_stream_connect()
update the msk socket status, is that correct?

However there are a few problems with lock: you must acquite the
subflow socket lock before calling tcp_sendmsg_fastopen() - or will see
subflow state corruption - but that in turn will cause a deadlock as 
 mptcp_stream_connect() acquires the msk socket lock and then the
subflow socket lock.

I think it's better leave the tcp_sendmsg_fastopen() body unchanged...

> +
>  	/* fastopen_req could already be freed in __inet_stream_connect
>  	 * if the connection times out or gets rst
>  	 */
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 470045793181..8cf307e4e59c 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -1673,9 +1673,16 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
>  	long timeo;
>  
>  	/* we don't support FASTOPEN yet */
> -	if (msg->msg_flags & MSG_FASTOPEN)
> -		return -EOPNOTSUPP;
> +	if (msg->msg_flags & MSG_FASTOPEN) {
> +		struct socket *ssock = __mptcp_nmpc_socket(msk);

... acquire the subflow socket lock here...

>  
> +		if (ssock) {
> +			int copied_syn_fastopen = 0;
> +
> +			ret = tcp_sendmsg_fastopen(ssock->sk, msg, &copied_syn_fastopen, len, NULL);
> +			copied += copied_syn_fastopen;
> +		}

... and additionally handle the sock state update here. Possibly you
can encapsulate all the fastopen code in a new function - say
__mptcp_sendmsg_fastopen(), as it will be called under the msk socket
lock.


Side note: you should enter the fastopen branch even when 
inet_sk(ssock->sk)->defer_connect is set

Cheers,

Paolo