Re: [PATCH mptcp-next v14 3/7] mptcp: add __mptcp_subflow_disconnect helper

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang, Paolo,

On 11/10/2023 15:34, Geliang Tang wrote:
> When closing the msk->first socket in __mptcp_close_ssk(), if there's
> another subflow available, it's better to avoid resetting it, just shut
> down it.
> 
> This patch adds a new helper __mptcp_subflow_disconnect(), and reuse
> flag MPTCP_CF_FASTCLOSE in this case. When MPTCP_CF_FASTCLOSE isn't set,
> we invoke tcp_shutdown() instead of tcp_disconnect().
> 
> Co-developed-by: Paolo Abeni <pabeni@redhat.com>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
>  net/mptcp/protocol.c | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 30e0c29ae0a4..1a54d55f8bb2 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -2366,6 +2366,26 @@ bool __mptcp_retransmit_pending_data(struct sock *sk)
>  #define MPTCP_CF_PUSH		BIT(1)
>  #define MPTCP_CF_FASTCLOSE	BIT(2)
>  
> +/* be sure to send a reset only if the caller asked for it, also
> + * clean completely the subflow status when the subflow reaches
> + * TCP_CLOSE state
> + */
> +static void __mptcp_subflow_disconnect(struct sock *ssk,
> +				       struct mptcp_subflow_context *subflow,
> +				       unsigned int flags)
> +{
> +	if (((1 << ssk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN)) ||
> +	    (flags & MPTCP_CF_FASTCLOSE)) {
> +		/* The MPTCP code never wait on the subflow sockets, TCP-level
> +		 * disconnect should never fail
> +		 */
> +		WARN_ON_ONCE(tcp_disconnect(ssk, 0));
> +		mptcp_subflow_ctx_reset(subflow);
> +	} else {
> +		tcp_shutdown(ssk, SEND_SHUTDOWN);

I didn't check in detail but should we not also call
__mptcp_subflow_error_report()?

And maybe other helpers? What is done between the 'goto out' and the
'out' label in __mptcp_close_ssk() here below? → what we do when other
subflows are being closed.

Cheers,
Matt

> +	}
> +}
> +
>  /* subflow sockets can be either outgoing (connect) or incoming
>   * (accept).
>   *
> @@ -2403,7 +2423,7 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
>  	lock_sock_nested(ssk, SINGLE_DEPTH_NESTING);
>  
>  	if ((flags & MPTCP_CF_FASTCLOSE) && !__mptcp_check_fallback(msk)) {
> -		/* be sure to force the tcp_disconnect() path,
> +		/* be sure to force the tcp_close path
>  		 * to generate the egress reset
>  		 */
>  		ssk->sk_lingertime = 0;
> @@ -2413,11 +2433,7 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
>  
>  	need_push = (flags & MPTCP_CF_PUSH) && __mptcp_retransmit_pending_data(sk);
>  	if (!dispose_it) {
> -		/* The MPTCP code never wait on the subflow sockets, TCP-level
> -		 * disconnect should never fail
> -		 */
> -		WARN_ON_ONCE(tcp_disconnect(ssk, 0));
> -		mptcp_subflow_ctx_reset(subflow);
> +		__mptcp_subflow_disconnect(ssk, subflow, flags);
>  		release_sock(ssk);
>  
>  		goto out;

-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net