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

[Paolo Abeni <pabeni@redhat.com>]

On Tue, 2022-09-20 at 17:02 +0200, Matthieu Baerts wrote:
> Hi Paolo, Dmytro,
> 
> On 20/09/2022 16:36, Paolo Abeni wrote:
> > 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
> 
> I think we should better discuss about that at the next meeting because
> all items you are asking here is what Benjamin did in [1]:
> 
> https://patchwork.kernel.org/project/mptcp/patch/20220908133829.3410092-4-benjamin.hesmans@tessares.net/

Almost. The above lacks the msk socket state update.

Cheers,

Paolo