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

[Matthieu Baerts <matthieu.baerts@tessares.net>]

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/

The work from Dmytro helped Benjamin to start looking at that and
propose another approach. Before the meeting, we can look at creating a
series focussed on the sending part taking patches from both Benjamin
and Dmytro using Benjamin's approach if that's OK. Of course both Dmytro
and Benjamin will be credited to have worked on that.

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