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

Dmytro Shytyi posted 7 patches 1 year, 5 months ago
Maintainers: Eric Dumazet <edumazet@google.com>, "David S. Miller" <davem@davemloft.net>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>, David Ahern <dsahern@kernel.org>, Mat Martineau <mathew.j.martineau@linux.intel.com>, Matthieu Baerts <matthieu.baerts@tessares.net>, Shuah Khan <shuah@kernel.org>
There is a newer version of this series
[RFC PATCH mptcp-next v8 3/7] reuse tcp_sendmsg_fastopen()
Posted by Dmytro Shytyi 1 year, 5 months ago
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);
+
 	/* 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);
 
+		if (ssock) {
+			int copied_syn_fastopen = 0;
+
+			ret = tcp_sendmsg_fastopen(ssock->sk, msg, &copied_syn_fastopen, len, NULL);
+			copied += copied_syn_fastopen;
+		}
+	}
 	/* silently ignore everything else */
 	msg->msg_flags &= MSG_MORE | MSG_DONTWAIT | MSG_NOSIGNAL;
 
-- 
2.25.1
Re: [RFC PATCH mptcp-next v8 3/7] reuse tcp_sendmsg_fastopen()
Posted by Paolo Abeni 1 year, 5 months ago
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
Re: [RFC PATCH mptcp-next v8 3/7] reuse tcp_sendmsg_fastopen()
Posted by Dmytro Shytyi 1 year, 5 months ago
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.
Re: [RFC PATCH mptcp-next v8 3/7] reuse tcp_sendmsg_fastopen()
Posted by Matthieu Baerts 1 year, 5 months ago
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
Re: [RFC PATCH mptcp-next v8 3/7] reuse tcp_sendmsg_fastopen()
Posted by Paolo Abeni 1 year, 5 months ago
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
Re: [RFC PATCH mptcp-next v8 3/7] reuse tcp_sendmsg_fastopen()
Posted by Dmytro Shytyi 1 year, 5 months ago
Hi Matthieu,

My previous patches had locks in this function (mptcp_sendmsg()) also 
and code was generated much earlier.

If you add locks in the mptcp _sendmsg() in current context it will lead 
to deadlock.

I temporarly avoided adding locks as I need to carefully think about this.


Best,

Dmytro.

On 9/20/2022 5:02 PM, 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/
>
> 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