Re: [PATCH 6.6.y] mptcp: avoid duplicated SUB_CLOSED events

[Greg KH <gregkh@linuxfoundation.org>]

On Tue, Sep 03, 2024 at 12:17:48PM +0200, Matthieu Baerts (NGI0) wrote:
> commit d82809b6c5f2676b382f77a5cbeb1a5d91ed2235 upstream.
> 
> The initial subflow might have already been closed, but still in the
> connection list. When the worker is instructed to close the subflows
> that have been marked as closed, it might then try to close the initial
> subflow again.
> 
>  A consequence of that is that the SUB_CLOSED event can be seen twice:
> 
>   # ip mptcp endpoint
>   1.1.1.1 id 1 subflow dev eth0
>   2.2.2.2 id 2 subflow dev eth1
> 
>   # ip mptcp monitor &
>   [         CREATED] remid=0 locid=0 saddr4=1.1.1.1 daddr4=9.9.9.9
>   [     ESTABLISHED] remid=0 locid=0 saddr4=1.1.1.1 daddr4=9.9.9.9
>   [  SF_ESTABLISHED] remid=0 locid=2 saddr4=2.2.2.2 daddr4=9.9.9.9
> 
>   # ip mptcp endpoint delete id 1
>   [       SF_CLOSED] remid=0 locid=0 saddr4=1.1.1.1 daddr4=9.9.9.9
>   [       SF_CLOSED] remid=0 locid=0 saddr4=1.1.1.1 daddr4=9.9.9.9
> 
> The first one is coming from mptcp_pm_nl_rm_subflow_received(), and the
> second one from __mptcp_close_subflow().
> 
> To avoid doing the post-closed processing twice, the subflow is now
> marked as closed the first time.
> 
> Note that it is not enough to check if we are dealing with the first
> subflow and check its sk_state: the subflow might have been reset or
> closed before calling mptcp_close_ssk().
> 
> Fixes: b911c97c7dc7 ("mptcp: add netlink event support")
> Cc: stable@vger.kernel.org
> Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> Reviewed-by: Mat Martineau <martineau@kernel.org>
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> [ Conflict in protocol.h due to commit f1f26512a9bf ("mptcp: use plain
>   bool instead of custom binary enum") and more that are not in this
>   version, because they modify the context and the size of __unused. The
>   conflict is easy to resolve, by not modifying data_avail type. ]
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
>  net/mptcp/protocol.c | 6 ++++++
>  net/mptcp/protocol.h | 3 ++-
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index e4d446f32761..ba6248372aee 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -2471,6 +2471,12 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
>  void mptcp_close_ssk(struct sock *sk, struct sock *ssk,
>  		     struct mptcp_subflow_context *subflow)
>  {
> +	/* The first subflow can already be closed and still in the list */
> +	if (subflow->close_event_done)
> +		return;
> +
> +	subflow->close_event_done = true;
> +
>  	if (sk->sk_state == TCP_ESTABLISHED)
>  		mptcp_event(MPTCP_EVENT_SUB_CLOSED, mptcp_sk(sk), ssk, GFP_KERNEL);
>  
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index ecbea95970f6..b9a4f6364b78 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -500,7 +500,8 @@ struct mptcp_subflow_context {
>  		stale : 1,	    /* unable to snd/rcv data, do not use for xmit */
>  		valid_csum_seen : 1,        /* at least one csum validated */
>  		is_mptfo : 1,	    /* subflow is doing TFO */
> -		__unused : 10;
> +		close_event_done : 1,       /* has done the post-closed part */
> +		__unused : 9;
>  	enum mptcp_data_avail data_avail;
>  	bool	scheduled;
>  	u32	remote_nonce;
> -- 
> 2.45.2
> 
> 

Now applied.