Re: [PATCH mptcp-next 01/11] mptcp: avoid dup SUB_CLOSED events after disconnect

[Geliang Tang <geliang@kernel.org>]

Hi Matt,

On Fri, 2025-12-26 at 07:40 +0100, Matthieu Baerts (NGI0) wrote:
> In case of subflow disconnect(), which can also happen with the first
> subflow in case of errors like timeout or reset,
> mptcp_subflow_ctx_reset
> will reset most fields from the mptcp_subflow_context structure,
> including close_event_done. Then, when another subflow is closed, yet
> another SUB_CLOSED event for the disconnected initial subflow is
> sent.
> Because of the previous reset, there are no source address and
> destination port.
> 
> A solution is then to also check the subflow's local id: it shouldn't
> be
> negative anyway.
> 
> Another solution would be not to reset subflow->close_event_done at
> disconnect time, but when reused. But then, probably the whole reset
> could be done when being reused. Let's not change this logic, similar
> to TCP with tcp_disconnect().
> 
> Fixes: d82809b6c5f2 ("mptcp: avoid duplicated SUB_CLOSED events")
> Reported-by: Marco Angaroni <marco.angaroni@italtel.com>
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/603
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
>  net/mptcp/protocol.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 900f26e21acd..1d68648b5194 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -2622,8 +2622,8 @@ void mptcp_close_ssk(struct sock *sk, struct
> sock *ssk,
>  	struct mptcp_sock *msk = mptcp_sk(sk);
>  	struct sk_buff *skb;
>  
> -	/* The first subflow can already be closed and still in the
> list */
> -	if (subflow->close_event_done)
> +	/* The 1st subflow can already be closed/disco and still in
> the

I suggest that we avoid abbreviating it as "disco" here and instead
write out "disconnected" in full. If that makes this line of comment
too long, it can be split into two lines.

Besides this, this patch looks good to me.

    Reviewed-by: Geliang Tang <geliang@kernel.org>

Thanks,
-Geliang

> list */
> +	if (subflow->close_event_done || READ_ONCE(subflow-
> >local_id) < 0)
>  		return;
>  
>  	subflow->close_event_done = true;