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

Paolo Abeni posted 1 patch 1 year, 2 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/multipath-tcp/mptcp_net-next tags/patchew/b9cfd415281fc206300a3adbcdbd36047facb31c.1675095773.git.pabeni@redhat.com
Maintainers: Matthieu Baerts <matthieu.baerts@tessares.net>, "David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, Mat Martineau <mathew.j.martineau@linux.intel.com>
net/mptcp/subflow.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
[PATCH mptcp-net v3] mptcp: be careful on subflow status propagation on errors
Posted by Paolo Abeni 1 year, 2 months ago
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")
Reviewed-by: Matthieu Baerts <matthieu.baerts@tessares.net>
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
--
v2 -> v3:
 - cleanup code comment
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..f075607adad4 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 state.
+		 * 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: [PATCH mptcp-net v3] mptcp: be careful on subflow status propagation on errors
Posted by Matthieu Baerts 1 year, 2 months ago
Hi Paolo,

On 30/01/2023 17:23, 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 patch!

Now in our tree (fix for -net):

New patches for t/upstream-net:
- 8db5a7061505: mptcp: be careful on subflow status propagation on errors
- Results: bc024ce64d00..2215d62f4f56 (export-net)
- Results: c8daebf53701..0825e0ffa158 (export)

Tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export-net/20230131T091051
https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20230131T091051

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