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