[PATCH mptcp-net] mptcp: ensure subflow is unhashed before cleaning the backlog

[PATCH mptcp-net] mptcp: ensure subflow is unhashed before cleaning the backlog

[Paolo Abeni]

While tacking care of the mptcp-level listener I unintentionally
moved the subflow level unhash after the subflow listener backlog
cleanup.

That could cause some nasty race and makes the code harder to read.

Address the issue restoring the proper order of operations.

Fixes: 57fc0f1ceaa4 ("mptcp: ensure listener is unhashed before updating the sk status")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/protocol.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index f2abae254524..03a7c83bc2d2 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2957,10 +2957,10 @@ static void mptcp_check_listen_stop(struct sock *sk)
 		return;
 
 	lock_sock_nested(ssk, SINGLE_DEPTH_NESTING);
+	tcp_set_state(ssk, TCP_CLOSE);
 	mptcp_subflow_queue_clean(sk, ssk);
 	inet_csk_listen_stop(ssk);
 	mptcp_event_pm_listener(ssk, MPTCP_EVENT_LISTENER_CLOSED);
-	tcp_set_state(ssk, TCP_CLOSE);
 	release_sock(ssk);
 }
 
-- 
2.40.1

Re: mptcp: ensure subflow is unhashed before cleaning the backlog: Tests Results

[MPTCP CI]

Hi Paolo,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal (except selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/5428204190564352
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5428204190564352/summary/summary.txt

- KVM Validation: normal (only selftest_mptcp_join):
  - Unstable: 1 failed test(s): selftest_mptcp_join 🔴:
  - Task: https://cirrus-ci.com/task/6554104097406976
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6554104097406976/summary/summary.txt

- KVM Validation: debug (only selftest_mptcp_join):
  - Unstable: 1 failed test(s): selftest_mptcp_join 🔴:
  - Task: https://cirrus-ci.com/task/5850416655630336
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5850416655630336/summary/summary.txt

- KVM Validation: debug (except selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/6220233875128320
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6220233875128320/summary/summary.txt

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/038400f3a829


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-debug

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (Tessares)

Re: [PATCH mptcp-net] mptcp: ensure subflow is unhashed before cleaning the backlog

[Matthieu Baerts]

Hi Paolo,

On 23/06/2023 16:51, Paolo Abeni wrote:
> While tacking care of the mptcp-level listener I unintentionally
> moved the subflow level unhash after the subflow listener backlog
> cleanup.
> 
> That could cause some nasty race and makes the code harder to read.
> 
> Address the issue restoring the proper order of operations.

Thank you for this fix!

I was surprised to see the CI complaining about the "fastclose server
test" test, with *and* without the debug kernel config. But I don't see
the link with that, it looks like this test is not stable. I just updated:

https://github.com/multipath-tcp/mptcp_net-next/issues/324

The patch looks then good to me!

Reviewed-by: Matthieu Baerts <matthieu.baerts@tessares.net>

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

Re: [PATCH mptcp-net] mptcp: ensure subflow is unhashed before cleaning the backlog

[Matthieu Baerts]

Hi Paolo,

On 23/06/2023 16:51, Paolo Abeni wrote:
> While tacking care of the mptcp-level listener I unintentionally
> moved the subflow level unhash after the subflow listener backlog
> cleanup.
> 
> That could cause some nasty race and makes the code harder to read.
> 
> Address the issue restoring the proper order of operations.
> 
> Fixes: 57fc0f1ceaa4 ("mptcp: ensure listener is unhashed before updating the sk status")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

Thank you for the patch!

Now in our tree (fixes for -net):

New patches for t/upstream-net and t/upstream:
- 61f0a42bb0c0: mptcp: ensure subflow is unhashed before cleaning the
backlog
- Results: d3dabc07c83c..4303dba6139f (export-net)
- Results: cc9edd912a76..3b62cbd7cc66 (export)

Tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export-net/20230626T134743
https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20230626T134743

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

Re: mptcp: ensure subflow is unhashed before cleaning the backlog: Tests Results

[MPTCP CI]

Hi Paolo,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal (except selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/4635897983926272
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4635897983926272/summary/summary.txt

- KVM Validation: debug (only selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/6324747844190208
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6324747844190208/summary/summary.txt

- KVM Validation: normal (only selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/5761797890768896
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5761797890768896/summary/summary.txt

- KVM Validation: debug (except selftest_mptcp_join):
  - Unstable: 1 failed test(s): packetdrill_add_addr 🔴:
  - Task: https://cirrus-ci.com/task/5198847937347584
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5198847937347584/summary/summary.txt

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/1dc606871ef9


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-debug

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (Tessares)

Re: [PATCH mptcp-net] mptcp: ensure subflow is unhashed before cleaning the backlog

[Paolo Abeni]

On Mon, 2023-06-26 at 15:20 +0200, Matthieu Baerts wrote:
> Hi Paolo,
> 
> On 23/06/2023 16:51, Paolo Abeni wrote:
> > While tacking care of the mptcp-level listener I unintentionally
> > moved the subflow level unhash after the subflow listener backlog
> > cleanup.
> > 
> > That could cause some nasty race and makes the code harder to read.
> > 
> > Address the issue restoring the proper order of operations.
> 
> Thank you for this fix!
> 
> I was surprised to see the CI complaining about the "fastclose server
> test" test, with *and* without the debug kernel config. 

Same here!

On the plus side, it's more job security ;)

I'll try to have a look when time allows

/P