Re: [PATCH mptcp-net] mptcp: fix subflow bitfield misuse

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mptcp: resource leak due to uncleared close_event_done in reused subflow
--

commit 3af2e392c470dbc8520a6d4b6cca07a2f647ac5f
Author: Paolo Abeni <pabeni@redhat.com>

mptcp: fix subflow bitfield misuse

This commit moves the close_event_done flag out of a bitfield in
mptcp_subflow_context to prevent concurrent access corruption. It also
removes a local_id check in mptcp_close_ssk, as the flag is no longer
cleared at disconnect time.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index e89a69ab927c9..9b98f25690036 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -2750,7 +2750,7 @@ void mptcp_close_ssk(struct sock *sk, struct sock *ssk,
>  		     struct mptcp_subflow_context *subflow)
>  {
>  	/* The first subflow can already be closed or disconnected */
> -	if (subflow->close_event_done || READ_ONCE(subflow->local_id) < 0)
> +	if (subflow->close_event_done)
>  		return;

[Severity: High]
Does this early return cause a resource leak when a subflow is reused?

Since the patch moves close_event_done out of the reset struct_group, it is
no longer cleared by mptcp_subflow_ctx_reset() when a socket is disconnected
and reconnected. If the first subflow receives a closure event,
close_event_done becomes true.

When the reconnected subflow later terminates and calls mptcp_close_ssk(),
close_event_done is already true, which causes it to return early here. Could
this bypass __mptcp_close_ssk() and permanently leak the subflow socket and
associated memory?

>  
>  	subflow->close_event_done = true;
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 0384d6a023f9d..9a294b29102ab 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
[ ... ]
> @@ -625,6 +624,9 @@ struct mptcp_subflow_context {
>  	int     cached_sndbuf;	    /* sndbuf size when last synced with the msk sndbuf,
>  				     * protected by the msk socket lock
>  				     */
> +	bool	close_event_done;   /* netlink event generated,
> +				     * protected by msk socket lock
> +				     */
>  
>  	struct	sock *tcp_sock;	    /* tcp sk backpointer */
>  	struct	sock *conn;	    /* parent mptcp_sock */

[Severity: High]
Should close_event_done still be cleared during a subflow disconnect?

By placing close_event_done outside of the struct_group(reset) boundary,
mptcp_subflow_ctx_reset() misses it during the memset operation. If this flag
remains set on a disconnected and reused subflow, subsequent netlink events
for closures might be dropped.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/0342bcf7f4bc6c4598ecbe58cfd8ec75505c328e.1790009113.git.pabeni@redhat.com?part=1