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

[Dmytro Shytyi <dmytro@shytyi.net>]

On 9/20/2022 4:36 PM, 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...


In v9 I still modify the tcp_sendmsg_fastopen() a little bit with adding 
some locks.

>> +
>>   	/* 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...
Some locks are acquired in v9.
>>   
>> +		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 tried to add this (defer_connect) in v9, but didn't check if it works.


> Cheers,
>
> Paolo

Best,

Dmytro.