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

[Paolo Abeni <pabeni@redhat.com>]

On Wed, 2022-09-07 at 19:22 +0200, Matthieu Baerts wrote:
> 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!

Thank you for the review!
> 
> 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?

I personally see this as an improvement, as AFAIK there is no mandated
by RFC return error code for MPTCP.
> 
> > 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?

yes...

> In this case "copied" will be 0 and we will return the new -EPIPE, maybe
> not what we want, no?

no;) we now always return 'copied'. We set 'copied' to the correct
error code only in the error path, saving a conditional in the fast
path.

All gracefully stolen from plain TCP ;)

> 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)

That could be a possible follow-up I guess, but I would refrain from
touching TCP to address issues/295.


Thanks!

Paolo