[v1] mptcp: fix subflow bitfield misuse

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

Posted by Paolo Abeni 4 days, 18 hours ago

The subflow status bitfield is protected by the subflow socket
lock, with one notable exception: the `close_event_done` bit
is actually under the msk socket lock protection.

Flipping the latter bit may corrupt other entries in the same
bitfield.

Move the  `close_event_done` outside the bitfield in a 64bit
hole. Since the mentioned field is now not cleared at subflow
disconnect time, additionally remove the now unneeded `local_id`
check in mptcp_close_ssk.

Fixes: d82809b6c5f2 ("mptcp: avoid duplicated SUB_CLOSED events")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/protocol.c | 2 +-
 net/mptcp/protocol.h | 6 ++++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 09779428e9ea..226cd1befe91 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2786,7 +2786,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;
 
 	subflow->close_event_done = true;
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 4bf04f9ecbd9..4be7450c3c83 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -598,10 +598,9 @@ struct mptcp_subflow_context {
 		closing : 1,	    /* must not pass rx data to msk anymore */
 		valid_csum_seen : 1,        /* at least one csum validated */
 		is_mptfo : 1,	    /* subflow is doing TFO */
-		close_event_done : 1,       /* has done the post-closed part */
 		mpc_drop : 1,	    /* the MPC option has been dropped in a rtx */
 		resetting : 1,	    /* subflow is resetting */
-		__unused : 8;
+		__unused : 9;
 	bool	data_avail;
 	bool	scheduled;
 	bool	pm_listener;	    /* a listener managed by the kernel PM? */
@@ -639,6 +638,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 */
-- 
2.55.0

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

Posted by Matthieu Baerts 4 days, 3 hours ago

Hi Paolo,

On 21/09/2026 18:45, Paolo Abeni wrote:
> The subflow status bitfield is protected by the subflow socket
> lock, with one notable exception: the `close_event_done` bit
> is actually under the msk socket lock protection.
> 
> Flipping the latter bit may corrupt other entries in the same
> bitfield.

Thank you for looking at this!

> Move the  `close_event_done` outside the bitfield in a 64bit

(32?)

> hole. Since the mentioned field is now not cleared at subflow
> disconnect time, additionally remove the now unneeded `local_id`
> check in mptcp_close_ssk.

I'm sorry, I'm not sure to understand that: if it is not cleared at
subflow disconnect time, should we not clear it explicitly at msk
disconnect time?

If I remember well, this flag was added to avoid duplicated SUB_CLOSED
events only for the first subflow. Instead, I wonder if this flag
shouldn't be moved to the msk, and have a:

  if (ssk == msk->first) {
      if (msk->first_close_event_done)
          return;
      msk->first_close_event_done = true;  // reset at msk disco
  }

Or maybe the whole logic is wrong, and we should just focus on not
sending the netlink close event for the first subflow twice?

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

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

Posted by Paolo Abeni 3 days ago

On 9/22/26 09:35, Matthieu Baerts wrote:
> Hi Paolo,
> 
> On 21/09/2026 18:45, Paolo Abeni wrote:
>> The subflow status bitfield is protected by the subflow socket
>> lock, with one notable exception: the `close_event_done` bit
>> is actually under the msk socket lock protection.
>>
>> Flipping the latter bit may corrupt other entries in the same
>> bitfield.
> 
> Thank you for looking at this!
> 
>> Move the  `close_event_done` outside the bitfield in a 64bit
> 
> (32?)
> 
>> hole. Since the mentioned field is now not cleared at subflow
>> disconnect time, additionally remove the now unneeded `local_id`
>> check in mptcp_close_ssk.
> 
> I'm sorry, I'm not sure to understand that: if it is not cleared at
> subflow disconnect time, should we not clear it explicitly at msk
> disconnect time?
Yes, indeed. I rushed this one out of the door, sorry.

/P

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

Posted by MPTCP CI 4 days, 16 hours ago

Hi Paolo,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal (except selftest_mptcp_join): Success! ✅
- KVM Validation: normal (only selftest_mptcp_join): Success! ✅
- KVM Validation: debug (except selftest_mptcp_join): Success! ✅
- KVM Validation: debug (only selftest_mptcp_join): Success! ✅
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Perf: Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/35632757495

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/ce82aea56ef1
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=1170553


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)

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

Posted by sashiko-bot@kernel.org 4 days, 17 hours ago

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