mptcp: avoid tcp_done() on already closed sockets

[PATCH mptcp-net 1/2] mptcp: avoid setting TCP_CLOSE state twice

Posted by Matthieu Baerts 2 years, 10 months ago

tcp_set_state() is called from tcp_done() already.

There is then no need to first set the state to TCP_CLOSE, then call
tcp_done().

Fixes: d582484726c4 ("mptcp: fix fallback for MP_JOIN subflows")
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/362
Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---
 net/mptcp/subflow.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 28c64811a8af..a36d1b1088d7 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -406,7 +406,6 @@ void mptcp_subflow_reset(struct sock *ssk)
 	/* must hold: tcp_done() could drop last reference on parent */
 	sock_hold(sk);
 
-	tcp_set_state(ssk, TCP_CLOSE);
 	tcp_send_active_reset(ssk, GFP_ATOMIC);
 	tcp_done(ssk);
 	if (!test_and_set_bit(MPTCP_WORK_CLOSE_SUBFLOW, &mptcp_sk(sk)->flags) &&

-- 
2.38.1

Re: [PATCH mptcp-net 1/2] mptcp: avoid setting TCP_CLOSE state twice

Posted by Paolo Abeni 2 years, 10 months ago

On Tue, 2023-02-21 at 18:54 +0100, Matthieu Baerts wrote:
> tcp_set_state() is called from tcp_done() already.
> 
> There is then no need to first set the state to TCP_CLOSE, then call
> tcp_done().
> 
> Fixes: d582484726c4 ("mptcp: fix fallback for MP_JOIN subflows")
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/362
> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> ---
>  net/mptcp/subflow.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 28c64811a8af..a36d1b1088d7 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -406,7 +406,6 @@ void mptcp_subflow_reset(struct sock *ssk)
>  	/* must hold: tcp_done() could drop last reference on parent */
>  	sock_hold(sk);
>  
> -	tcp_set_state(ssk, TCP_CLOSE);
>  	tcp_send_active_reset(ssk, GFP_ATOMIC);
>  	tcp_done(ssk);
>  	if (!test_and_set_bit(MPTCP_WORK_CLOSE_SUBFLOW, &mptcp_sk(sk)->flags) &&
> 

I think this one could be squashed to:

mptcp: fix possible deadlock in subflow_error_report

WDYT?

thanks,

Paolo

Re: [PATCH mptcp-net 1/2] mptcp: avoid setting TCP_CLOSE state twice

Posted by Matthieu Baerts 2 years, 10 months ago

Hi Paolo,

On 22/02/2023 15:32, Paolo Abeni wrote:
> On Tue, 2023-02-21 at 18:54 +0100, Matthieu Baerts wrote:
>> tcp_set_state() is called from tcp_done() already.
>>
>> There is then no need to first set the state to TCP_CLOSE, then call
>> tcp_done().
>>
>> Fixes: d582484726c4 ("mptcp: fix fallback for MP_JOIN subflows")
>> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/362
>> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
>> ---
>>  net/mptcp/subflow.c | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
>> index 28c64811a8af..a36d1b1088d7 100644
>> --- a/net/mptcp/subflow.c
>> +++ b/net/mptcp/subflow.c
>> @@ -406,7 +406,6 @@ void mptcp_subflow_reset(struct sock *ssk)
>>  	/* must hold: tcp_done() could drop last reference on parent */
>>  	sock_hold(sk);
>>  
>> -	tcp_set_state(ssk, TCP_CLOSE);
>>  	tcp_send_active_reset(ssk, GFP_ATOMIC);
>>  	tcp_done(ssk);
>>  	if (!test_and_set_bit(MPTCP_WORK_CLOSE_SUBFLOW, &mptcp_sk(sk)->flags) &&
>>
> 
> I think this one could be squashed to:
> 
> mptcp: fix possible deadlock in subflow_error_report
> 
> WDYT?

Thank you for your reply!

I'm a bit confused: the commit you mention ("mptcp: fix possible
deadlock in subflow_error_report"):
- modifies subflow_error_report() function
- fixes 15cc10453398 ("mptcp: deliver ssk errors to msk")

While my patch here:
- modifies mptcp_subflow_reset() function
- fixes: d582484726c4 ("mptcp: fix fallback for MP_JOIN subflows")

They seem to fix different issues: better to keep them separated, no?

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

Re: [PATCH mptcp-net 1/2] mptcp: avoid setting TCP_CLOSE state twice

Posted by Paolo Abeni 2 years, 9 months ago

On Wed, 2023-02-22 at 16:25 +0100, Matthieu Baerts wrote:
> Hi Paolo,
> 
> On 22/02/2023 15:32, Paolo Abeni wrote:
> > On Tue, 2023-02-21 at 18:54 +0100, Matthieu Baerts wrote:
> > > tcp_set_state() is called from tcp_done() already.
> > > 
> > > There is then no need to first set the state to TCP_CLOSE, then call
> > > tcp_done().
> > > 
> > > Fixes: d582484726c4 ("mptcp: fix fallback for MP_JOIN subflows")
> > > Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/362
> > > Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> > > ---
> > >  net/mptcp/subflow.c | 1 -
> > >  1 file changed, 1 deletion(-)
> > > 
> > > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> > > index 28c64811a8af..a36d1b1088d7 100644
> > > --- a/net/mptcp/subflow.c
> > > +++ b/net/mptcp/subflow.c
> > > @@ -406,7 +406,6 @@ void mptcp_subflow_reset(struct sock *ssk)
> > >  	/* must hold: tcp_done() could drop last reference on parent */
> > >  	sock_hold(sk);
> > >  
> > > -	tcp_set_state(ssk, TCP_CLOSE);
> > >  	tcp_send_active_reset(ssk, GFP_ATOMIC);
> > >  	tcp_done(ssk);
> > >  	if (!test_and_set_bit(MPTCP_WORK_CLOSE_SUBFLOW, &mptcp_sk(sk)->flags) &&
> > > 
> > 
> > I think this one could be squashed to:
> > 
> > mptcp: fix possible deadlock in subflow_error_report
> > 
> > WDYT?
> 
> Thank you for your reply!
> 
> I'm a bit confused: the commit you mention ("mptcp: fix possible
> deadlock in subflow_error_report"):
> - modifies subflow_error_report() function
> - fixes 15cc10453398 ("mptcp: deliver ssk errors to msk")
> 
> While my patch here:
> - modifies mptcp_subflow_reset() function
> - fixes: d582484726c4 ("mptcp: fix fallback for MP_JOIN subflows")
> 
> They seem to fix different issues: better to keep them separated, no?

I'm sorry I picked the wrong commit.

What I meant is that I thought the above change could stay together
with the commit introducing:

	if (ssk->sk_state == TCP_CLOSE)
		return;

in mptcp_subflow_reset() - that is "mptcp: refactor passive socket
initialization" unless I'm wrong again.

Reading again the code and the git history, I now agree it's better
keep this change separate.

Cheers,

Paolo

Re: [PATCH mptcp-net 1/2] mptcp: avoid setting TCP_CLOSE state twice

Posted by Matthieu Baerts 2 years, 9 months ago

Hi Paolo,

On 23/02/2023 16:00, Paolo Abeni wrote:
> On Wed, 2023-02-22 at 16:25 +0100, Matthieu Baerts wrote:
>> Hi Paolo,
>>
>> On 22/02/2023 15:32, Paolo Abeni wrote:
>>> On Tue, 2023-02-21 at 18:54 +0100, Matthieu Baerts wrote:
>>>> tcp_set_state() is called from tcp_done() already.
>>>>
>>>> There is then no need to first set the state to TCP_CLOSE, then call
>>>> tcp_done().
>>>>
>>>> Fixes: d582484726c4 ("mptcp: fix fallback for MP_JOIN subflows")
>>>> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/362
>>>> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
>>>> ---
>>>>  net/mptcp/subflow.c | 1 -
>>>>  1 file changed, 1 deletion(-)
>>>>
>>>> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
>>>> index 28c64811a8af..a36d1b1088d7 100644
>>>> --- a/net/mptcp/subflow.c
>>>> +++ b/net/mptcp/subflow.c
>>>> @@ -406,7 +406,6 @@ void mptcp_subflow_reset(struct sock *ssk)
>>>>  	/* must hold: tcp_done() could drop last reference on parent */
>>>>  	sock_hold(sk);
>>>>  
>>>> -	tcp_set_state(ssk, TCP_CLOSE);
>>>>  	tcp_send_active_reset(ssk, GFP_ATOMIC);
>>>>  	tcp_done(ssk);
>>>>  	if (!test_and_set_bit(MPTCP_WORK_CLOSE_SUBFLOW, &mptcp_sk(sk)->flags) &&
>>>>
>>>
>>> I think this one could be squashed to:
>>>
>>> mptcp: fix possible deadlock in subflow_error_report
>>>
>>> WDYT?
>>
>> Thank you for your reply!
>>
>> I'm a bit confused: the commit you mention ("mptcp: fix possible
>> deadlock in subflow_error_report"):
>> - modifies subflow_error_report() function
>> - fixes 15cc10453398 ("mptcp: deliver ssk errors to msk")
>>
>> While my patch here:
>> - modifies mptcp_subflow_reset() function
>> - fixes: d582484726c4 ("mptcp: fix fallback for MP_JOIN subflows")
>>
>> They seem to fix different issues: better to keep them separated, no?
> 
> I'm sorry I picked the wrong commit.
> 
> What I meant is that I thought the above change could stay together
> with the commit introducing:
> 
> 	if (ssk->sk_state == TCP_CLOSE)
> 		return;
> 
> in mptcp_subflow_reset() - that is "mptcp: refactor passive socket
> initialization" unless I'm wrong again.

All good, thank you for the explanations, now I see the link!

> Reading again the code and the git history, I now agree it's better
> keep this change separate.

Great!

Can I interpret that as a "Acked-by" for the small series? :-)

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

Re: [PATCH mptcp-net 1/2] mptcp: avoid setting TCP_CLOSE state twice

Posted by Paolo Abeni 2 years, 9 months ago

On Thu, 2023-02-23 at 16:40 +0100, Matthieu Baerts wrote:
> Hi Paolo,
> 
> On 23/02/2023 16:00, Paolo Abeni wrote:
> > On Wed, 2023-02-22 at 16:25 +0100, Matthieu Baerts wrote:
> > > Hi Paolo,
> > > 
> > > On 22/02/2023 15:32, Paolo Abeni wrote:
> > > > On Tue, 2023-02-21 at 18:54 +0100, Matthieu Baerts wrote:
> > > > > tcp_set_state() is called from tcp_done() already.
> > > > > 
> > > > > There is then no need to first set the state to TCP_CLOSE, then call
> > > > > tcp_done().
> > > > > 
> > > > > Fixes: d582484726c4 ("mptcp: fix fallback for MP_JOIN subflows")
> > > > > Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/362
> > > > > Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> > > > > ---
> > > > >  net/mptcp/subflow.c | 1 -
> > > > >  1 file changed, 1 deletion(-)
> > > > > 
> > > > > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> > > > > index 28c64811a8af..a36d1b1088d7 100644
> > > > > --- a/net/mptcp/subflow.c
> > > > > +++ b/net/mptcp/subflow.c
> > > > > @@ -406,7 +406,6 @@ void mptcp_subflow_reset(struct sock *ssk)
> > > > >  	/* must hold: tcp_done() could drop last reference on parent */
> > > > >  	sock_hold(sk);
> > > > >  
> > > > > -	tcp_set_state(ssk, TCP_CLOSE);
> > > > >  	tcp_send_active_reset(ssk, GFP_ATOMIC);
> > > > >  	tcp_done(ssk);
> > > > >  	if (!test_and_set_bit(MPTCP_WORK_CLOSE_SUBFLOW, &mptcp_sk(sk)->flags) &&
> > > > > 
> > > > 
> > > > I think this one could be squashed to:
> > > > 
> > > > mptcp: fix possible deadlock in subflow_error_report
> > > > 
> > > > WDYT?
> > > 
> > > Thank you for your reply!
> > > 
> > > I'm a bit confused: the commit you mention ("mptcp: fix possible
> > > deadlock in subflow_error_report"):
> > > - modifies subflow_error_report() function
> > > - fixes 15cc10453398 ("mptcp: deliver ssk errors to msk")
> > > 
> > > While my patch here:
> > > - modifies mptcp_subflow_reset() function
> > > - fixes: d582484726c4 ("mptcp: fix fallback for MP_JOIN subflows")
> > > 
> > > They seem to fix different issues: better to keep them separated, no?
> > 
> > I'm sorry I picked the wrong commit.
> > 
> > What I meant is that I thought the above change could stay together
> > with the commit introducing:
> > 
> > 	if (ssk->sk_state == TCP_CLOSE)
> > 		return;
> > 
> > in mptcp_subflow_reset() - that is "mptcp: refactor passive socket
> > initialization" unless I'm wrong again.
> 
> All good, thank you for the explanations, now I see the link!
> 
> > Reading again the code and the git history, I now agree it's better
> > keep this change separate.
> 
> Great!
> 
> Can I interpret that as a "Acked-by" for the small series? :-)

Sure! Just to be redundant: 

Acked-by: Paolo Abeni <pabeni@redhat.com>

/P

Re: [PATCH mptcp-net 1/2] mptcp: avoid setting TCP_CLOSE state twice

Posted by Matthieu Baerts 2 years, 9 months ago

On 23/02/2023 17:38, Paolo Abeni wrote:
> On Thu, 2023-02-23 at 16:40 +0100, Matthieu Baerts wrote:
>> Hi Paolo,
>>
>> On 23/02/2023 16:00, Paolo Abeni wrote:
>>> On Wed, 2023-02-22 at 16:25 +0100, Matthieu Baerts wrote:
>>>> Hi Paolo,
>>>>
>>>> On 22/02/2023 15:32, Paolo Abeni wrote:
>>>>> On Tue, 2023-02-21 at 18:54 +0100, Matthieu Baerts wrote:
>>>>>> tcp_set_state() is called from tcp_done() already.
>>>>>>
>>>>>> There is then no need to first set the state to TCP_CLOSE, then call
>>>>>> tcp_done().
>>>>>>
>>>>>> Fixes: d582484726c4 ("mptcp: fix fallback for MP_JOIN subflows")
>>>>>> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/362
>>>>>> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
>>>>>> ---
>>>>>>  net/mptcp/subflow.c | 1 -
>>>>>>  1 file changed, 1 deletion(-)
>>>>>>
>>>>>> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
>>>>>> index 28c64811a8af..a36d1b1088d7 100644
>>>>>> --- a/net/mptcp/subflow.c
>>>>>> +++ b/net/mptcp/subflow.c
>>>>>> @@ -406,7 +406,6 @@ void mptcp_subflow_reset(struct sock *ssk)
>>>>>>  	/* must hold: tcp_done() could drop last reference on parent */
>>>>>>  	sock_hold(sk);
>>>>>>  
>>>>>> -	tcp_set_state(ssk, TCP_CLOSE);
>>>>>>  	tcp_send_active_reset(ssk, GFP_ATOMIC);
>>>>>>  	tcp_done(ssk);
>>>>>>  	if (!test_and_set_bit(MPTCP_WORK_CLOSE_SUBFLOW, &mptcp_sk(sk)->flags) &&
>>>>>>
>>>>>
>>>>> I think this one could be squashed to:
>>>>>
>>>>> mptcp: fix possible deadlock in subflow_error_report
>>>>>
>>>>> WDYT?
>>>>
>>>> Thank you for your reply!
>>>>
>>>> I'm a bit confused: the commit you mention ("mptcp: fix possible
>>>> deadlock in subflow_error_report"):
>>>> - modifies subflow_error_report() function
>>>> - fixes 15cc10453398 ("mptcp: deliver ssk errors to msk")
>>>>
>>>> While my patch here:
>>>> - modifies mptcp_subflow_reset() function
>>>> - fixes: d582484726c4 ("mptcp: fix fallback for MP_JOIN subflows")
>>>>
>>>> They seem to fix different issues: better to keep them separated, no?
>>>
>>> I'm sorry I picked the wrong commit.
>>>
>>> What I meant is that I thought the above change could stay together
>>> with the commit introducing:
>>>
>>> 	if (ssk->sk_state == TCP_CLOSE)
>>> 		return;
>>>
>>> in mptcp_subflow_reset() - that is "mptcp: refactor passive socket
>>> initialization" unless I'm wrong again.
>>
>> All good, thank you for the explanations, now I see the link!
>>
>>> Reading again the code and the git history, I now agree it's better
>>> keep this change separate.
>>
>> Great!
>>
>> Can I interpret that as a "Acked-by" for the small series? :-)
> 
> Sure! Just to be redundant: 
> 
> Acked-by: Paolo Abeni <pabeni@redhat.com>

Thank you for the review!

Now in our tree (fix for -net + squashed twice):

New patches for t/upstream-net:
- df6d0b9cc589: mptcp: avoid setting TCP_CLOSE state twice
- a1904eac72d7: "squashed" patch 2/2 in "DO-NOT-MERGE: mptcp: use
kmalloc on kasan build (net)"
- db88e3c96f21: tg:msg: rename subject: improve code coverage for CI (net)
- Results: 2d0e2918d1a9..b0d9441c818b (export-net)

For t/upstream:
- bcffcc8152ee: "squashed" patch 2/2 in "DO-NOT-MERGE: mptcp: use
kmalloc on kasan build"
- aa72cd1b9cf2: tg:msg: rename subject: improve code coverage for CI
- Results: e438a165544f..6ad4eca4e1af (export)

Tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export-net/20230223T165416
https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20230223T165416

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