Re: [PATCH v3 mptcp-next 1/3] mptcp: propagate fastclose error.

[Matthieu Baerts <matthieu.baerts@tessares.net>]

Hi Paolo,

On 01/09/2022 19:31, Paolo Abeni wrote:
> When an mptcp socket is closed due to an incoming FASTCLOSE
> option, so specific sk_err is set and later syscall will
> fail usually with EPIPE.
> 
> Align the current fastclose error handling with TCP reset,
> properly setting the socket error according to the current
> msk state and propagating such error.

Thank you for the patch and sorry for the delay!

I have some questions here below:

> Additionally sendmsg() is currently not handling properly
> the sk_err, always returning EPIPE.
Could it be seen as a bug-fix and deserving a dedicated commit?

> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
>  net/mptcp/protocol.c | 45 ++++++++++++++++++++++++++++++++++----------
>  1 file changed, 35 insertions(+), 10 deletions(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index f4891db86217..b04f184695e4 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -1685,9 +1685,13 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
>  	if ((1 << sk->sk_state) & ~(TCPF_ESTABLISHED | TCPF_CLOSE_WAIT)) {
>  		ret = sk_stream_wait_connect(sk, &timeo);
>  		if (ret)
> -			goto out;
> +			goto do_error;
>  	}
>  
> +	ret = -EPIPE;

Regarding this line above... (see below)

> +	if (unlikely(sk->sk_err || (sk->sk_shutdown & SEND_SHUTDOWN)))
> +		goto do_error;
> +
>  	pfrag = sk_page_frag(sk);
>  
>  	while (msg_data_left(msg)) {

...Could we eventually have no data left and no going through the while
loop?

In this case "copied" will be 0 and we will return the new -EPIPE, maybe
not what we want, no?
Or should we change 'ret' value just before calling 'goto do_error' to
avoid that?

(...)

> @@ -2404,12 +2410,31 @@ static void mptcp_check_fastclose(struct mptcp_sock *msk)
>  		unlock_sock_fast(tcp_sk, slow);
>  	}
>  
> +	/* Mirror the tcp_reset() error propagation */
> +	switch (sk->sk_state) {
> +	case TCP_SYN_SENT:
> +		sk->sk_err = ECONNREFUSED;
> +		break;
> +	case TCP_CLOSE_WAIT:
> +		sk->sk_err = EPIPE;
> +		break;
> +	case TCP_CLOSE:
> +		return;
> +	default:
> +		sk->sk_err = ECONNRESET;
> +	}

Should we eventually move this out of tcp_reset(), export it (inline
function?) and re-using it here to avoid the duplication (and eventually
being out-of-sync in case of changes on TCP side? even if I guess this
would not change but well)

> +
>  	inet_sk_state_store(sk, TCP_CLOSE);
>  	sk->sk_shutdown = SHUTDOWN_MASK;
>  	smp_mb__before_atomic(); /* SHUTDOWN must be visible first */
>  	set_bit(MPTCP_WORK_CLOSE_SUBFLOW, &msk->flags);
>  
> -	mptcp_close_wake_up(sk);
> +	/* the calling mptcp_worker will properly destroy the socket */
> +	if (sock_flag(sk, SOCK_DEAD))
> +		return;
> +
> +	sk->sk_state_change(sk);
> +	sk_error_report(sk);
>  }
>  
>  static void __mptcp_retrans(struct sock *sk)

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