Re: [PATCH mptcp-next 03/11] mptcp: only reset subflow errors when propagated

[Geliang Tang <geliang@kernel.org>]

Hi Matt,

On Fri, 2025-12-26 at 07:40 +0100, Matthieu Baerts (NGI0) wrote:
> Some subflow socket errors need to be reported to the MPTCP socket:
> the
> initial subflow connect (MP_CAPABLE), and the ones from the fallback
> sockets. The others are not propagated.
> 
> The issue is that sock_error() was used to retrieve the error, which
> was
> also resetting the sk_err field. Because of that, when notifying the
> userspace about subflow close events later on from the MPTCP worker,
> the
> ssk->sk_err field was always 0.
> 
> Now, the error (sk_err) is only reset when propagating it to the msk.
> 
> Fixes: 15cc10453398 ("mptcp: deliver ssk errors to msk")
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
> Note: I guess we could also duplicate the error in
> mptcp_subflow_context
> struct before scheduling the worker, and use this field in
> mptcp_event_put_token_and_ssk(), but I don't see why we should always
> reset ssk->sk_err in __mptcp_subflow_error_report() in all cases.
> ---
>  net/mptcp/protocol.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 1d68648b5194..5d6c987089c6 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -817,10 +817,9 @@ static bool __mptcp_ofo_queue(struct mptcp_sock
> *msk)
>  
>  static bool __mptcp_subflow_error_report(struct sock *sk, struct
> sock *ssk)
>  {
> -	int err = sock_error(ssk);
>  	int ssk_state;
>  
> -	if (!err)
> +	if (!READ_ONCE(ssk->sk_err))
>  		return false;

Your fix works. I'm wondering if swapping the order of 'if (sk-
>sk_state != TCP_SYN_SENT && !__mptcp_check_fallback(mptcp_sk(sk)))'
and 'if (!err)' would be a bit simpler, i.e.:

int err = sock_error(ssk);
int ssk_state;

if (sk->sk_state != TCP_SYN_SENT &&
    !__mptcp_check_fallback(mptcp_sk(sk)))
        return false;

if (!err)
        return false;

WDYT?

Thanks,
-Geliang

>  
>  	/* only propagate errors on fallen-back sockets or
> @@ -837,7 +836,7 @@ static bool __mptcp_subflow_error_report(struct
> sock *sk, struct sock *ssk)
>  	ssk_state = inet_sk_state_load(ssk);
>  	if (ssk_state == TCP_CLOSE && !sock_flag(sk, SOCK_DEAD))
>  		mptcp_set_state(sk, ssk_state);
> -	WRITE_ONCE(sk->sk_err, -err);
> +	WRITE_ONCE(sk->sk_err, -sock_error(ssk));
>  
>  	/* This barrier is coupled with smp_rmb() in mptcp_poll() */
>  	smp_wmb();