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@xxxxxxxxxxxxxxx
> Tested-by: Arınç ÜNAL <arinc.unal@xxxxxxxxxx>
> Reviewed-by: Mat Martineau <martineau@xxxxxxxxxx>
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@xxxxxxxxxx>
> Signed-off-by: Paolo Abeni <pabeni@xxxxxxxxxx>
> [ 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@xxxxxxxxxx>
> ---
> 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.
