[PATCH mptcp-net v2] mptcp: be careful on subflow status propagation on errors

[PATCH mptcp-net v2] mptcp: be careful on subflow status propagation on errors

[Paolo Abeni]

Currently the subflow error report callback unconditionally
propagates the fallback subflow status to the owning msk.

If the msk is already orphaned, the above prevents the code
from correctly tracking the msk moving to the TCP_CLOSE state
and doing the appropriate cleanup.

All the above causes increasing memory usage over time and
sporadic self-tests failures.

There is a great deal of infrastructure trying to propagate
correctly the fallback subflow status to the owning mptcp socket,
e.g. via mptcp_subflow_eof() and subflow_sched_work_if_closed():
in the error propagation path we need only to cope with unorphaned
sockets.

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/339
Fixes: 15cc10453398 ("mptcp: deliver ssk errors to msk")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
--
v1 -> v2:
 - propagate the status for non orphaned sockets
---
 net/mptcp/subflow.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 5e57a9a7178b..39a4b60f6d6b 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1404,6 +1404,7 @@ void __mptcp_error_report(struct sock *sk)
 	mptcp_for_each_subflow(msk, subflow) {
 		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
 		int err = sock_error(ssk);
+		int ssk_state;
 
 		if (!err)
 			continue;
@@ -1414,7 +1415,14 @@ void __mptcp_error_report(struct sock *sk)
 		if (sk->sk_state != TCP_SYN_SENT && !__mptcp_check_fallback(msk))
 			continue;
 
-		inet_sk_state_store(sk, inet_sk_state_load(ssk));
+		/* We need to propagate only transition to CLOSE statue and
+		 * orphaned socket will see such state change via
+		 * subflow_sched_work_if_closed() and that path will properly
+		 * destroy the msk as needed
+		 */
+		ssk_state = inet_sk_state_load(ssk);
+		if (ssk_state == TCP_CLOSE && !sock_flag(sk, SOCK_DEAD))
+			inet_sk_state_store(sk, ssk_state);
 		sk->sk_err = -err;
 
 		/* This barrier is coupled with smp_rmb() in mptcp_poll() */
-- 
2.39.1

Re: mptcp: be careful on subflow status propagation on errors: 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/4931194693877760
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4931194693877760/summary/summary.txt

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

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

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

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


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 v2] mptcp: be careful on subflow status propagation on errors

[Matthieu Baerts]

Hi Paolo,

On 27/01/2023 19:40, Paolo Abeni wrote:
> Currently the subflow error report callback unconditionally
> propagates the fallback subflow status to the owning msk.
> 
> If the msk is already orphaned, the above prevents the code
> from correctly tracking the msk moving to the TCP_CLOSE state
> and doing the appropriate cleanup.
> 
> All the above causes increasing memory usage over time and
> sporadic self-tests failures.
> 
> There is a great deal of infrastructure trying to propagate
> correctly the fallback subflow status to the owning mptcp socket,
> e.g. via mptcp_subflow_eof() and subflow_sched_work_if_closed():
> in the error propagation path we need only to cope with unorphaned
> sockets.

Thank you for the update!

I just have one small comment below (no need to send a v3)

> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/339
> Fixes: 15cc10453398 ("mptcp: deliver ssk errors to msk")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> --
> v1 -> v2:
>  - propagate the status for non orphaned sockets
> ---
>  net/mptcp/subflow.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 5e57a9a7178b..39a4b60f6d6b 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -1404,6 +1404,7 @@ void __mptcp_error_report(struct sock *sk)
>  	mptcp_for_each_subflow(msk, subflow) {
>  		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
>  		int err = sock_error(ssk);
> +		int ssk_state;
>  
>  		if (!err)
>  			continue;
> @@ -1414,7 +1415,14 @@ void __mptcp_error_report(struct sock *sk)
>  		if (sk->sk_state != TCP_SYN_SENT && !__mptcp_check_fallback(msk))
>  			continue;
>  
> -		inet_sk_state_store(sk, inet_sk_state_load(ssk));
> +		/* We need to propagate only transition to CLOSE statue and
> +		 * orphaned socket will see such state change via
> +		 * subflow_sched_work_if_closed() and that path will properly
> +		 * destroy the msk as needed

Do you mind if I "break" the sentence above in two?

> We need to propagate only transition to CLOSE status.
> Orphaned socket will get such state change via
> subflow_sched_work_if_closed() and that path will properly
> destroy the msk as needed.

If it is OK for you, I can do the modification when applying the patch!

Other than that, it looks good to me :-)

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

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

Re: mptcp: be careful on subflow status propagation on errors: 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):
  - Unstable: 1 failed test(s): packetdrill_syscalls 🔴:
  - Task: https://cirrus-ci.com/task/5015285221031936
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5015285221031936/summary/summary.txt

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

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

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

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


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: mptcp: be careful on subflow status propagation on errors: 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/4854224215867392
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4854224215867392/summary/summary.txt

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

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

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

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


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)