Our CI reported lockdep splat in the fastopen code:
 ======================================================
 WARNING: possible circular locking dependency detected
 6.0.0.mptcp_f5e8bfe9878d+ #1558 Not tainted
 ------------------------------------------------------
 packetdrill/1071 is trying to acquire lock:
 ffff8881bd198140 (sk_lock-AF_INET){+.+.}-{0:0}, at: inet_wait_for_connect+0x19c/0x310

 but task is already holding lock:
 ffff8881b8346540 (k-sk_lock-AF_INET){+.+.}-{0:0}, at: mptcp_sendmsg+0xfdf/0x1740

 which lock already depends on the new lock.

 the existing dependency chain (in reverse order) is:

 -> #1 (k-sk_lock-AF_INET){+.+.}-{0:0}:
        __lock_acquire+0xb6d/0x1860
        lock_acquire+0x1d8/0x620
        lock_sock_nested+0x37/0xd0
        inet_stream_connect+0x3f/0xa0
        mptcp_connect+0x411/0x800
        __inet_stream_connect+0x3ab/0x800
        mptcp_stream_connect+0xac/0x110
        __sys_connect+0x101/0x130
        __x64_sys_connect+0x6e/0xb0
        do_syscall_64+0x59/0x90
        entry_SYSCALL_64_after_hwframe+0x63/0xcd

 -> #0 (sk_lock-AF_INET){+.+.}-{0:0}:
        check_prev_add+0x15e/0x2110
        validate_chain+0xace/0xdf0
        __lock_acquire+0xb6d/0x1860
        lock_acquire+0x1d8/0x620
        lock_sock_nested+0x37/0xd0
        inet_wait_for_connect+0x19c/0x310
        __inet_stream_connect+0x26c/0x800
        tcp_sendmsg_fastopen+0x341/0x650
        mptcp_sendmsg+0x109d/0x1740
        sock_sendmsg+0xe1/0x120
        __sys_sendto+0x1c7/0x2a0
        __x64_sys_sendto+0xdc/0x1b0
        do_syscall_64+0x59/0x90
        entry_SYSCALL_64_after_hwframe+0x63/0xcd

 other info that might help us debug this:

  Possible unsafe locking scenario:

        CPU0                    CPU1
        ----                    ----
   lock(k-sk_lock-AF_INET);
                                lock(sk_lock-AF_INET);
                                lock(k-sk_lock-AF_INET);
   lock(sk_lock-AF_INET);

  *** DEADLOCK ***

 1 lock held by packetdrill/1071:
  #0: ffff8881b8346540 (k-sk_lock-AF_INET){+.+.}-{0:0}, at: mptcp_sendmsg+0xfdf/0x1740
 ======================================================

The problem is caused by the blocking inet_wait_for_connect() releasing
and re-acquiring the msk socket lock while the subflow socket lock is
still held and the MPTCP socket requires that the msk socket lock must
be acquired before the subflow socket lock.

Address the issue always invoking tcp_sendmsg_fastopen() in an
unblocking manner, and later eventually complete the blocking
__inet_stream_connect() as needed.

Fixes: d98a82a6afc7 ("mptcp: handle defer connect in mptcp_sendmsg")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
v1 -> v2:
 - really invoke fastopen in unblocking manner
 - clear copied_syn on completion errors

commit ("mptcp: factor out mptcp_connect()") will need some
mangling on top of this one.

A possible alternative would be targeting even the above to -net.

Not strictly required, but it will also make more striaght-forward/
legit the __inet_stream_connect() invocation here.
---
 net/mptcp/protocol.c | 43 ++++++++++++++++++++++++++++++-------------
 1 file changed, 30 insertions(+), 13 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index f599ad44ed24..21f5676a60e8 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1673,6 +1673,31 @@ static void mptcp_set_nospace(struct sock *sk)
 	set_bit(MPTCP_NOSPACE, &mptcp_sk(sk)->flags);
 }
 
+static int mptcp_sendmsg_fastopen(struct sock *sk, struct sock *ssk, struct msghdr *msg,
+				  size_t len, int *copied_syn)
+{
+	unsigned int saved_flags = msg->msg_flags;
+	int ret;
+
+	lock_sock(ssk);
+	msg->msg_flags |= MSG_DONTWAIT;
+	ret = tcp_sendmsg_fastopen(ssk, msg, copied_syn, len, NULL);
+	msg->msg_flags = saved_flags;
+	release_sock(ssk);
+
+	/* do the blocking bits of inet_stream_connect outside the ssk socket lock*/
+	if (ret == -EINPROGRESS && !(msg->msg_flags & MSG_DONTWAIT)) {
+		ret = __inet_stream_connect(sk->sk_socket, msg->msg_name,
+					    msg->msg_namelen, msg->msg_flags, 1);
+
+		/* On connect hard error fastopen no bytes actually sent. */
+		if (ret && ret != -EINPROGRESS && ret != -ERESTARTSYS && ret != -EINTR)
+			*copied_syn = 0;
+	}
+
+	return ret;
+}
+
 static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 {
 	struct mptcp_sock *msk = mptcp_sk(sk);
@@ -1692,24 +1717,16 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 	lock_sock(sk);
 
 	ssock = __mptcp_nmpc_socket(msk);
-	if (unlikely(ssock && inet_sk(ssock->sk)->defer_connect)) {
-		struct sock *ssk = ssock->sk;
+	if (unlikely(ssock && (inet_sk(ssock->sk)->defer_connect ||
+			       msg->msg_flags & MSG_FASTOPEN))) {
 		int copied_syn = 0;
 
-		lock_sock(ssk);
-
-		ret = tcp_sendmsg_fastopen(ssk, msg, &copied_syn, len, NULL);
+		ret = mptcp_sendmsg_fastopen(sk, ssock->sk, msg, len, &copied_syn);
 		copied += copied_syn;
-		if (ret == -EINPROGRESS && copied_syn > 0) {
-			/* reflect the new state on the MPTCP socket */
-			inet_sk_state_store(sk, inet_sk_state_load(ssk));
-			release_sock(ssk);
+		if (ret == -EINPROGRESS && copied_syn > 0)
 			goto out;
-		} else if (ret) {
-			release_sock(ssk);
+		else if (ret)
 			goto do_error;
-		}
-		release_sock(ssk);
 	}
 
 	timeo = sock_sndtimeo(sk, msg->msg_flags & MSG_DONTWAIT);
-- 
2.37.3

Hi Paolo,

Thank you for having caught the issue and already provided the patch!

On 11/10/2022 19:40, Paolo Abeni wrote:
> Our CI reported lockdep splat in the fastopen code:

(...)

> commit ("mptcp: factor out mptcp_connect()") will need some
> mangling on top of this one.
> 
> A possible alternative would be targeting even the above to -net.
> 
> Not strictly required, but it will also make more striaght-forward/
> legit the __inet_stream_connect() invocation here.

Indeed, we can. We would need more work around that anyway, no? (see my
comment below)

> ---
>  net/mptcp/protocol.c | 43 ++++++++++++++++++++++++++++++-------------
>  1 file changed, 30 insertions(+), 13 deletions(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index f599ad44ed24..21f5676a60e8 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -1673,6 +1673,31 @@ static void mptcp_set_nospace(struct sock *sk)
>  	set_bit(MPTCP_NOSPACE, &mptcp_sk(sk)->flags);
>  }
>  
> +static int mptcp_sendmsg_fastopen(struct sock *sk, struct sock *ssk, struct msghdr *msg,
> +				  size_t len, int *copied_syn)
> +{
> +	unsigned int saved_flags = msg->msg_flags;
> +	int ret;
> +
> +	lock_sock(ssk);
> +	msg->msg_flags |= MSG_DONTWAIT;
> +	ret = tcp_sendmsg_fastopen(ssk, msg, copied_syn, len, NULL);

Would it not be cleaner (and easier to maintain?) if a new parameter is
added to tcp_sendmsg_fastopen() to force passing the O_NONBLOCK flag to
__inet_stream_connect? Not to modify msg->msg_flags to have the
behaviour we want.

> +	msg->msg_flags = saved_flags;
> +	release_sock(ssk);
> +
> +	/* do the blocking bits of inet_stream_connect outside the ssk socket lock*/

(I thought checkpatch.pl was complaining when there was no space before
'*/' but it doesn't, all good :) )

> +	if (ret == -EINPROGRESS && !(msg->msg_flags & MSG_DONTWAIT)) {
> +		ret = __inet_stream_connect(sk->sk_socket, msg->msg_name,
> +					    msg->msg_namelen, msg->msg_flags, 1);

Without the commit ("mptcp: factor out mptcp_connect()"), would we need
to call the -net version of mptcp_stream_connect() but without the lock?

> +
> +		/* On connect hard error fastopen no bytes actually sent. */
> +		if (ret && ret != -EINPROGRESS && ret != -ERESTARTSYS && ret != -EINTR)

The two last ones are there in case the "actual connect()" got
interrupted, right?

> +			*copied_syn = 0;
> +	}
> +
> +	return ret;
> +}
> +
>  static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
>  {
>  	struct mptcp_sock *msk = mptcp_sk(sk);
> @@ -1692,24 +1717,16 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
>  	lock_sock(sk);
>  
>  	ssock = __mptcp_nmpc_socket(msk);
> -	if (unlikely(ssock && inet_sk(ssock->sk)->defer_connect)) {
> -		struct sock *ssk = ssock->sk;
> +	if (unlikely(ssock && (inet_sk(ssock->sk)->defer_connect ||
> +			       msg->msg_flags & MSG_FASTOPEN))) {

Just above in this mptcp_sendmsg() function, we mention we don't support
MSG_FASTOPEN yet. maybe we don't need to add this additional check now?
(Or maybe you want to discretely remove this previous restriction for
-net as well and support MSG_FASTOPEN? :-D )

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

On Wed, 2022-10-12 at 10:22 +0200, Matthieu Baerts wrote:
> Thank you for having caught the issue and already provided the patch!
> 
> On 11/10/2022 19:40, Paolo Abeni wrote:
> > Our CI reported lockdep splat in the fastopen code:
> 
> (...)
> 
> > commit ("mptcp: factor out mptcp_connect()") will need some
> > mangling on top of this one.
> > 
> > A possible alternative would be targeting even the above to -net.
> > 
> > Not strictly required, but it will also make more striaght-forward/
> > legit the __inet_stream_connect() invocation here.
> 
> Indeed, we can. We would need more work around that anyway, no? (see my
> comment below)
> 
> > ---
> >  net/mptcp/protocol.c | 43 ++++++++++++++++++++++++++++++-------------
> >  1 file changed, 30 insertions(+), 13 deletions(-)
> > 
> > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > index f599ad44ed24..21f5676a60e8 100644
> > --- a/net/mptcp/protocol.c
> > +++ b/net/mptcp/protocol.c
> > @@ -1673,6 +1673,31 @@ static void mptcp_set_nospace(struct sock *sk)
> >  	set_bit(MPTCP_NOSPACE, &mptcp_sk(sk)->flags);
> >  }
> >  
> > +static int mptcp_sendmsg_fastopen(struct sock *sk, struct sock *ssk, struct msghdr *msg,
> > +				  size_t len, int *copied_syn)
> > +{
> > +	unsigned int saved_flags = msg->msg_flags;
> > +	int ret;
> > +
> > +	lock_sock(ssk);
> > +	msg->msg_flags |= MSG_DONTWAIT;
> > +	ret = tcp_sendmsg_fastopen(ssk, msg, copied_syn, len, NULL);
> 
> Would it not be cleaner (and easier to maintain?) if a new parameter is
> added to tcp_sendmsg_fastopen() to force passing the O_NONBLOCK flag to
> __inet_stream_connect? Not to modify msg->msg_flags to have the
> behaviour we want.

Uhmmm... cleaner only for this call-site. It could be confusing adding
another parameter for blocking where there is already msg_flags for
that. And I'm personally against adding to many arguments to a single
function...

> > +	msg->msg_flags = saved_flags;
> > +	release_sock(ssk);
> > +
> > +	/* do the blocking bits of inet_stream_connect outside the ssk socket lock*/
> 
> (I thought checkpatch.pl was complaining when there was no space before
> '*/' but it doesn't, all good :) )
> 
> > +	if (ret == -EINPROGRESS && !(msg->msg_flags & MSG_DONTWAIT)) {
> > +		ret = __inet_stream_connect(sk->sk_socket, msg->msg_name,
> > +					    msg->msg_namelen, msg->msg_flags, 1);
> 
> Without the commit ("mptcp: factor out mptcp_connect()"), would we need
> to call the -net version of mptcp_stream_connect() but without the lock?

Not strictly needed. I test this patch on export/net. Possibly cleaner.

> > +
> > +		/* On connect hard error fastopen no bytes actually sent. */
> > +		if (ret && ret != -EINPROGRESS && ret != -ERESTARTSYS && ret != -EINTR)
> 
> The two last ones are there in case the "actual connect()" got
> interrupted, right?

Yes. Plain TCP consider copied_syn == 0 in that case. I did not find
any existing helper to match such condition. Perhapas a longer comment
may help.

> > +			*copied_syn = 0;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> >  static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
> >  {
> >  	struct mptcp_sock *msk = mptcp_sk(sk);
> > @@ -1692,24 +1717,16 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
> >  	lock_sock(sk);
> >  
> >  	ssock = __mptcp_nmpc_socket(msk);
> > -	if (unlikely(ssock && inet_sk(ssock->sk)->defer_connect)) {
> > -		struct sock *ssk = ssock->sk;
> > +	if (unlikely(ssock && (inet_sk(ssock->sk)->defer_connect ||
> > +			       msg->msg_flags & MSG_FASTOPEN))) {
> 
> Just above in this mptcp_sendmsg() function, we mention we don't support
> MSG_FASTOPEN yet. maybe we don't need to add this additional check now?

Thanks for catching it. This is a stray change from a previous
revision.


I can send a v2 addressing the above and cleaning up comments, but
AFAICS there is still pending the main question about including "mptcp:
factor out mptcp_connect()" in -net. Currently I think that could be
the better option.

Thanks,

Paolo

On 12/10/2022 11:10, Paolo Abeni wrote:
> On Wed, 2022-10-12 at 10:22 +0200, Matthieu Baerts wrote:
>> Thank you for having caught the issue and already provided the patch!
>>
>> On 11/10/2022 19:40, Paolo Abeni wrote:
>>> Our CI reported lockdep splat in the fastopen code:
>>
>> (...)
>>
>>> commit ("mptcp: factor out mptcp_connect()") will need some
>>> mangling on top of this one.
>>>
>>> A possible alternative would be targeting even the above to -net.
>>>
>>> Not strictly required, but it will also make more striaght-forward/
>>> legit the __inet_stream_connect() invocation here.
>>
>> Indeed, we can. We would need more work around that anyway, no? (see my
>> comment below)
>>
>>> ---
>>>  net/mptcp/protocol.c | 43 ++++++++++++++++++++++++++++++-------------
>>>  1 file changed, 30 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>>> index f599ad44ed24..21f5676a60e8 100644
>>> --- a/net/mptcp/protocol.c
>>> +++ b/net/mptcp/protocol.c
>>> @@ -1673,6 +1673,31 @@ static void mptcp_set_nospace(struct sock *sk)
>>>  	set_bit(MPTCP_NOSPACE, &mptcp_sk(sk)->flags);
>>>  }
>>>  
>>> +static int mptcp_sendmsg_fastopen(struct sock *sk, struct sock *ssk, struct msghdr *msg,
>>> +				  size_t len, int *copied_syn)
>>> +{
>>> +	unsigned int saved_flags = msg->msg_flags;
>>> +	int ret;
>>> +
>>> +	lock_sock(ssk);
>>> +	msg->msg_flags |= MSG_DONTWAIT;
>>> +	ret = tcp_sendmsg_fastopen(ssk, msg, copied_syn, len, NULL);
>>
>> Would it not be cleaner (and easier to maintain?) if a new parameter is
>> added to tcp_sendmsg_fastopen() to force passing the O_NONBLOCK flag to
>> __inet_stream_connect? Not to modify msg->msg_flags to have the
>> behaviour we want.
> 
> Uhmmm... cleaner only for this call-site. It could be confusing adding
> another parameter for blocking where there is already msg_flags for
> that. And I'm personally against adding to many arguments to a single
> function...

OK.
I was suggesting that because tcp_sendmsg_fastopen() is only called once.
I guess tcp_sendmsg_fastopen() will not be modified in a way that will
break MPTCP support because we force MSG_DONTWAIT, so fine for me.

>>> +	msg->msg_flags = saved_flags;
>>> +	release_sock(ssk);
>>> +
>>> +	/* do the blocking bits of inet_stream_connect outside the ssk socket lock*/
>>
>> (I thought checkpatch.pl was complaining when there was no space before
>> '*/' but it doesn't, all good :) )
>>
>>> +	if (ret == -EINPROGRESS && !(msg->msg_flags & MSG_DONTWAIT)) {
>>> +		ret = __inet_stream_connect(sk->sk_socket, msg->msg_name,
>>> +					    msg->msg_namelen, msg->msg_flags, 1);
>>
>> Without the commit ("mptcp: factor out mptcp_connect()"), would we need
>> to call the -net version of mptcp_stream_connect() but without the lock?
> 
> Not strictly needed. I test this patch on export/net. Possibly cleaner.

OK, thank you!

>>> +
>>> +		/* On connect hard error fastopen no bytes actually sent. */
>>> +		if (ret && ret != -EINPROGRESS && ret != -ERESTARTSYS && ret != -EINTR)
>>
>> The two last ones are there in case the "actual connect()" got
>> interrupted, right?
> 
> Yes. Plain TCP consider copied_syn == 0 in that case. I did not find
> any existing helper to match such condition. Perhapas a longer comment
> may help.

Thank you for the explanation.
Yes, maybe a longer comment but only if you have to send a v3.

>>> +			*copied_syn = 0;
>>> +	}
>>> +
>>> +	return ret;
>>> +}
>>> +
>>>  static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
>>>  {
>>>  	struct mptcp_sock *msk = mptcp_sk(sk);
>>> @@ -1692,24 +1717,16 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
>>>  	lock_sock(sk);
>>>  
>>>  	ssock = __mptcp_nmpc_socket(msk);
>>> -	if (unlikely(ssock && inet_sk(ssock->sk)->defer_connect)) {
>>> -		struct sock *ssk = ssock->sk;
>>> +	if (unlikely(ssock && (inet_sk(ssock->sk)->defer_connect ||
>>> +			       msg->msg_flags & MSG_FASTOPEN))) {
>>
>> Just above in this mptcp_sendmsg() function, we mention we don't support
>> MSG_FASTOPEN yet. maybe we don't need to add this additional check now?
> 
> Thanks for catching it. This is a stray change from a previous
> revision.
> 
> 
> I can send a v2 addressing the above and cleaning up comments, but
> AFAICS there is still pending the main question about including "mptcp:
> factor out mptcp_connect()" in -net. Currently I think that could be
> the better option.

It is fine for me to have the refactoring ("mptcp: factor out
mptcp_connect()") sent to net. I can move it.

Do you think we need to modify the commit message? e.g. "Note that the
fastopen call-path invokes mptcp_connect()" → only with MSG_FASTOPEN,
not in -net.

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

On Wed, 2022-10-12 at 11:38 +0200, Matthieu Baerts wrote:
> On 12/10/2022 11:10, Paolo Abeni wrote:
> > On Wed, 2022-10-12 at 10:22 +0200, Matthieu Baerts wrote:
> > > Thank you for having caught the issue and already provided the patch!
> > > 
> > > On 11/10/2022 19:40, Paolo Abeni wrote:
> > > > Our CI reported lockdep splat in the fastopen code:
> > > 
> > > (...)
> > > 
> > > > commit ("mptcp: factor out mptcp_connect()") will need some
> > > > mangling on top of this one.
> > > > 
> > > > A possible alternative would be targeting even the above to -net.
> > > > 
> > > > Not strictly required, but it will also make more striaght-forward/
> > > > legit the __inet_stream_connect() invocation here.
> > > 
> > > Indeed, we can. We would need more work around that anyway, no? (see my
> > > comment below)
> > > 
> > > > ---
> > > >  net/mptcp/protocol.c | 43 ++++++++++++++++++++++++++++++-------------
> > > >  1 file changed, 30 insertions(+), 13 deletions(-)
> > > > 
> > > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > > > index f599ad44ed24..21f5676a60e8 100644
> > > > --- a/net/mptcp/protocol.c
> > > > +++ b/net/mptcp/protocol.c
> > > > @@ -1673,6 +1673,31 @@ static void mptcp_set_nospace(struct sock *sk)
> > > >  	set_bit(MPTCP_NOSPACE, &mptcp_sk(sk)->flags);
> > > >  }
> > > >  
> > > > +static int mptcp_sendmsg_fastopen(struct sock *sk, struct sock *ssk, struct msghdr *msg,
> > > > +				  size_t len, int *copied_syn)
> > > > +{
> > > > +	unsigned int saved_flags = msg->msg_flags;
> > > > +	int ret;
> > > > +
> > > > +	lock_sock(ssk);
> > > > +	msg->msg_flags |= MSG_DONTWAIT;
> > > > +	ret = tcp_sendmsg_fastopen(ssk, msg, copied_syn, len, NULL);
> > > 
> > > Would it not be cleaner (and easier to maintain?) if a new parameter is
> > > added to tcp_sendmsg_fastopen() to force passing the O_NONBLOCK flag to
> > > __inet_stream_connect? Not to modify msg->msg_flags to have the
> > > behaviour we want.
> > 
> > Uhmmm... cleaner only for this call-site. It could be confusing adding
> > another parameter for blocking where there is already msg_flags for
> > that. And I'm personally against adding to many arguments to a single
> > function...
> 
> OK.
> I was suggesting that because tcp_sendmsg_fastopen() is only called once.
> I guess tcp_sendmsg_fastopen() will not be modified in a way that will
> break MPTCP support because we force MSG_DONTWAIT, so fine for me.
> 
> > > > +	msg->msg_flags = saved_flags;
> > > > +	release_sock(ssk);
> > > > +
> > > > +	/* do the blocking bits of inet_stream_connect outside the ssk socket lock*/
> > > 
> > > (I thought checkpatch.pl was complaining when there was no space before
> > > '*/' but it doesn't, all good :) )
> > > 
> > > > +	if (ret == -EINPROGRESS && !(msg->msg_flags & MSG_DONTWAIT)) {
> > > > +		ret = __inet_stream_connect(sk->sk_socket, msg->msg_name,
> > > > +					    msg->msg_namelen, msg->msg_flags, 1);
> > > 
> > > Without the commit ("mptcp: factor out mptcp_connect()"), would we need
> > > to call the -net version of mptcp_stream_connect() but without the lock?
> > 
> > Not strictly needed. I test this patch on export/net. Possibly cleaner.
> 
> OK, thank you!
> 
> > > > +
> > > > +		/* On connect hard error fastopen no bytes actually sent. */
> > > > +		if (ret && ret != -EINPROGRESS && ret != -ERESTARTSYS && ret != -EINTR)
> > > 
> > > The two last ones are there in case the "actual connect()" got
> > > interrupted, right?
> > 
> > Yes. Plain TCP consider copied_syn == 0 in that case. I did not find
> > any existing helper to match such condition. Perhapas a longer comment
> > may help.
> 
> Thank you for the explanation.
> Yes, maybe a longer comment but only if you have to send a v3.
> 
> > > > +			*copied_syn = 0;
> > > > +	}
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > >  static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
> > > >  {
> > > >  	struct mptcp_sock *msk = mptcp_sk(sk);
> > > > @@ -1692,24 +1717,16 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
> > > >  	lock_sock(sk);
> > > >  
> > > >  	ssock = __mptcp_nmpc_socket(msk);
> > > > -	if (unlikely(ssock && inet_sk(ssock->sk)->defer_connect)) {
> > > > -		struct sock *ssk = ssock->sk;
> > > > +	if (unlikely(ssock && (inet_sk(ssock->sk)->defer_connect ||
> > > > +			       msg->msg_flags & MSG_FASTOPEN))) {
> > > 
> > > Just above in this mptcp_sendmsg() function, we mention we don't support
> > > MSG_FASTOPEN yet. maybe we don't need to add this additional check now?
> > 
> > Thanks for catching it. This is a stray change from a previous
> > revision.
> > 
> > 
> > I can send a v2 addressing the above and cleaning up comments, but
> > AFAICS there is still pending the main question about including "mptcp:
> > factor out mptcp_connect()" in -net. Currently I think that could be
> > the better option.
> 
> It is fine for me to have the refactoring ("mptcp: factor out
> mptcp_connect()") sent to net. I can move it.
> 
> Do you think we need to modify the commit message? e.g. "Note that the
> fastopen call-path invokes mptcp_connect()" → only with MSG_FASTOPEN,
> not in -net.

I think "mptcp: factor out mptcp_connect()" will require quite a bit of
mangling to be move to -net, I'll send a -net version  together with v3
of this patch.

Thanks,

Paolo

On 12/10/2022 12:06, Paolo Abeni wrote:
> On Wed, 2022-10-12 at 11:38 +0200, Matthieu Baerts wrote:
>> On 12/10/2022 11:10, Paolo Abeni wrote:
>>> On Wed, 2022-10-12 at 10:22 +0200, Matthieu Baerts wrote:
>>>> On 11/10/2022 19:40, Paolo Abeni wrote:

(...)

>>>>> @@ -1692,24 +1717,16 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
>>>>>  	lock_sock(sk);
>>>>>  
>>>>>  	ssock = __mptcp_nmpc_socket(msk);
>>>>> -	if (unlikely(ssock && inet_sk(ssock->sk)->defer_connect)) {
>>>>> -		struct sock *ssk = ssock->sk;
>>>>> +	if (unlikely(ssock && (inet_sk(ssock->sk)->defer_connect ||
>>>>> +			       msg->msg_flags & MSG_FASTOPEN))) {
>>>>
>>>> Just above in this mptcp_sendmsg() function, we mention we don't support
>>>> MSG_FASTOPEN yet. maybe we don't need to add this additional check now?
>>>
>>> Thanks for catching it. This is a stray change from a previous
>>> revision.
>>>
>>>
>>> I can send a v2 addressing the above and cleaning up comments, but
>>> AFAICS there is still pending the main question about including "mptcp:
>>> factor out mptcp_connect()" in -net. Currently I think that could be
>>> the better option.
>>
>> It is fine for me to have the refactoring ("mptcp: factor out
>> mptcp_connect()") sent to net. I can move it.
>>
>> Do you think we need to modify the commit message? e.g. "Note that the
>> fastopen call-path invokes mptcp_connect()" → only with MSG_FASTOPEN,
>> not in -net.
> 
> I think "mptcp: factor out mptcp_connect()" will require quite a bit of
> mangling to be move to -net, I'll send a -net version  together with v3
> of this patch.

Do you mean that we need to modify it not to do some actions in -net?
Because for the moment, -net and net-next are similar, it should be easy
to switch from net-next to -net, no?

But yes, please send them together with v3, that will ease the validation :)

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