* [PATCH net 0/2] mptcp: subflow context lifetime fixes
@ 2026-10-26 12:04 Cen Zhang
2026-10-26 12:04 ` [PATCH net 1/2] mptcp: keep delegated subflow contexts alive until socket release Cen Zhang
2026-10-26 12:04 ` [PATCH net 2/2] mptcp: release the parent socket reference with the subflow context Cen Zhang
0 siblings, 2 replies; 6+ messages in thread
From: Cen Zhang @ 2026-10-26 12:04 UTC (permalink / raw)
To: matttbe, martineau, davem, edumazet, kuba, pabeni
Cc: geliang, horms, netdev, mptcp, linux-kernel, baijiaju1990,
jjzuming, zzzccc427
Two use-after-free fixes around the lifetime of the MPTCP subflow
context, both found with KASAN.
The delegated-action queue pins the TCP subflow socket, but its queue
entry is embedded in the separately allocated subflow context, which the
ULP release frees on its own. Patch 1 keeps a retired context attached
to its socket until the socket is destroyed, and lets NAPI free a
context that the plain-TCP fallback dropped while it was still queued.
That context also owns the only reference a holder of the TCP child has
on the MPTCP parent. Patch 2 moves that release to the same socket
destructor. This covers a BPF program that converted a TCP child with
bpf_skc_to_mptcp_sock() as well as a delegated action that updates the
parent after the ULP was released. It depends on patch 1, also for
stable backports.
Cen Zhang (2):
mptcp: keep delegated subflow contexts alive until socket release
mptcp: release the parent socket reference with the subflow context
net/mptcp/protocol.c | 18 +++++++++++++++-
net/mptcp/protocol.h | 8 +++++--
net/mptcp/subflow.c | 51 +++++++++++++++++++++++++++++++++++++++-----
3 files changed, 69 insertions(+), 8 deletions(-)
---
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH net 1/2] mptcp: keep delegated subflow contexts alive until socket release
2026-10-26 12:04 [PATCH net 0/2] mptcp: subflow context lifetime fixes Cen Zhang
@ 2026-10-26 12:04 ` Cen Zhang
2026-10-30 4:54 ` sashiko-bot
2026-10-30 12:25 ` netdev-bot+sashiko
2026-10-26 12:04 ` [PATCH net 2/2] mptcp: release the parent socket reference with the subflow context Cen Zhang
1 sibling, 2 replies; 6+ messages in thread
From: Cen Zhang @ 2026-10-26 12:04 UTC (permalink / raw)
To: matttbe, martineau, davem, edumazet, kuba, pabeni
Cc: geliang, horms, netdev, mptcp, linux-kernel, baijiaju1990,
jjzuming, zzzccc427
mptcp_subflow_delegate() links the subflow context into a per-CPU list
and pins only the TCP subflow socket with sock_hold(). The context is
freed independently of that reference: subflow_ulp_release() frees a
disposable context with kfree_rcu() when inet_csk_destroy_sock() tears
the ULP down, and __mptcp_close_ssk() does the same for a context whose
ULP release ran earlier. Neither path checks whether delegated_node is
still linked, and TCP protocol destruction runs before the final
sock_put(), so the queued socket reference does not protect the
context.
When mptcp_napi_poll() dequeues the entry on the scheduling CPU only
after that RCU grace period has elapsed, for example because the poll
returned at its budget or runs from ksoftirqd, it unlinks the node from
freed memory, as the KASAN report at the end shows:
TCP / NAPI Close RCU
enqueue ctx, sock_hold(ssk)
return at budget
__mptcp_close_ssk()
ctx->disposable = 1
__tcp_close()
tcp_cleanup_ulp()
kfree_rcu(ctx)
reclaim ctx
dequeue ctx->delegated_node
Keep a retired context attached to its socket until the socket itself
is destroyed, which happens only after every pending reference, the
delegated one included, has been dropped. mptcp_subflow_ctx_release()
chains sk_destruct: the replacement clears the ULP data pointer, calls
the original TCP destructor and then queues the context for RCU
freeing. It is only used for an unhashed socket that is already dead.
TCP never clones such a socket, so a child cannot inherit the
overridden destructor.
mptcp_napi_poll() skips a subflow whose ULP has been released or that
fell back to plain TCP: its protocol ops have been restored and nothing
is left to process. The ops pointer is cleared with the socket owned or
under its bh lock, so a racing read lands in the owned branch, which
only dequeues the entry.
The fallback branch of mptcp_subflow_drop_ctx() cannot use the socket
destructor: that socket lives on as a plain TCP socket and must keep
neither a stale ULP pointer nor an overridden destructor. It is reached
with a live socket when an unaccepted MPTCP socket is force-closed at
listener shutdown, and a delegated action may still be queued for its
first subflow. Hand such a context over to mptcp_napi_poll() instead:
the dropping path sets a new RETIRED bit in delegated_status and frees
the context only if no action was scheduled, while the poll frees a
dequeued context that carries that bit. Both sides use atomic
read-modify-write operations on the same word, so exactly one of them
frees the context.
BUG: KASAN: slab-use-after-free in __list_del_entry_valid_or_report+0x1ed/0x210
Read of size 8 at addr ffff8881271668c8 by task kworker/0:9/1315
Workqueue: events mptcp_worker
Call Trace:
<IRQ>
__list_del_entry_valid_or_report+0x1ed/0x210
mptcp_napi_poll+0x1de/0xa50
__napi_poll.constprop.0+0xab/0x670
net_rx_action+0x4df/0xe60
handle_softirqs+0x201/0x930
do_softirq+0x47/0xa0
</IRQ>
<TASK>
__local_bh_enable_ip+0xf6/0x120
__mptcp_close_ssk+0x7a/0x1680
mptcp_close_ssk+0x11f/0x170
mptcp_worker+0x7df/0x11c0
process_one_work+0x908/0x19c0
worker_thread+0x65c/0xe40
kthread+0x34f/0x460
ret_from_fork+0x659/0x940
ret_from_fork_asm+0x1a/0x30
</TASK>
Allocated by task 1308:
__kmalloc_cache_noprof+0x251/0x610
subflow_create_ctx+0x91/0x290
subflow_ulp_init+0x66/0x460
tcp_set_ulp+0x2d0/0x720
mptcp_subflow_create_socket+0x32b/0x9a0
__mptcp_subflow_connect+0x30a/0x12b0
mptcp_pm_create_subflow_or_signal_addr+0xf4a/0x1630
mptcp_pm_nl_add_addr_doit+0x4cf/0xb80
genl_family_rcv_msg_doit+0x1f6/0x2e0
genl_rcv_msg+0x48b/0x700
netlink_rcv_skb+0x133/0x390
genl_rcv+0x2d/0x40
netlink_unicast+0x504/0x840
netlink_sendmsg+0x7f7/0xcf0
____sys_sendmsg+0x88b/0x9d0
___sys_sendmsg+0x125/0x1d0
__sys_sendmsg+0x13e/0x1e0
do_syscall_64+0x115/0x6a0
entry_SYSCALL_64_after_hwframe+0x77/0x7f
Freed by task 29:
__rcu_free_sheaf_prepare+0x65/0x2a0
rcu_free_sheaf+0x1f/0xe0
rcu_core+0x661/0x1d10
handle_softirqs+0x201/0x930
run_ksoftirqd+0x33/0x60
smpboot_thread_fn+0x3bd/0x9a0
kthread+0x34f/0x460
ret_from_fork+0x659/0x940
ret_from_fork_asm+0x1a/0x30
Fixes: b19bc2945b40 ("mptcp: implement delegated actions")
Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
Assisted-by: LLM
---
net/mptcp/protocol.c | 18 +++++++++++++++++-
net/mptcp/protocol.h | 8 ++++++--
net/mptcp/subflow.c | 44 +++++++++++++++++++++++++++++++++++++++++---
3 files changed, 64 insertions(+), 6 deletions(-)
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 7a22e9388e0e..01b94a280c81 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2705,7 +2705,7 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
*/
if (!inet_csk(ssk)->icsk_ulp_ops) {
WARN_ON_ONCE(!sock_flag(ssk, SOCK_DEAD));
- kfree_rcu(subflow, rcu);
+ mptcp_subflow_ctx_release(ssk);
} else {
/* otherwise tcp will dispose of the ssk and subflow ctx */
__tcp_close(ssk, 0);
@@ -4793,6 +4793,21 @@ static int mptcp_napi_poll(struct napi_struct *napi, int budget)
struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
bh_lock_sock_nested(ssk);
+ if (unlikely(!READ_ONCE(inet_csk(ssk)->icsk_ulp_ops))) {
+ /* The subflow was retired, by ULP release or by a
+ * fallback to plain TCP: nothing is left to process.
+ * The ops pointer is cleared with the socket owned or
+ * under its bh lock, so a racing read lands in the
+ * owned branch below instead.
+ * mptcp_subflow_drop_ctx() leaves a still queued
+ * context to us: free it now that it is dequeued.
+ */
+ if (xchg(&subflow->delegated_status, 0) &
+ BIT(MPTCP_DELEGATE_RETIRED))
+ kfree_rcu(subflow, rcu);
+ goto unlock;
+ }
+
if (!sock_owned_by_user(ssk)) {
mptcp_subflow_process_delegated(ssk, xchg(&subflow->delegated_status, 0));
} else {
@@ -4804,6 +4819,7 @@ static int mptcp_napi_poll(struct napi_struct *napi, int budget)
smp_wmb();
clear_bit(MPTCP_DELEGATE_SCHEDULED, &subflow->delegated_status);
}
+unlock:
bh_unlock_sock(ssk);
sock_put(ssk);
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 011e73d04ee6..45bae97a4e21 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -553,8 +553,10 @@ DECLARE_PER_CPU(struct mptcp_delegated_action, mptcp_delegated_actions);
#define MPTCP_DELEGATE_SEND 1
#define MPTCP_DELEGATE_ACK 2
#define MPTCP_DELEGATE_SNDBUF 3
+#define MPTCP_DELEGATE_RETIRED 4 /* unreachable ctx, NAPI frees it */
-#define MPTCP_DELEGATE_ACTIONS_MASK (~BIT(MPTCP_DELEGATE_SCHEDULED))
+#define MPTCP_DELEGATE_ACTIONS_MASK (~(BIT(MPTCP_DELEGATE_SCHEDULED) | \
+ BIT(MPTCP_DELEGATE_RETIRED)))
/* MPTCP subflow context */
struct mptcp_subflow_context {
struct list_head node;/* conn_list of subflows */
@@ -593,7 +595,7 @@ struct mptcp_subflow_context {
send_fastclose : 1,
send_infinite_map : 1,
remote_key_valid : 1, /* received the peer key from */
- disposable : 1, /* ctx can be free at ulp release time */
+ disposable : 1, /* ctx is no longer owned by the msk */
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 */
@@ -646,6 +648,7 @@ struct mptcp_subflow_context {
const struct inet_connection_sock_af_ops *icsk_af_ops;
void (*tcp_state_change)(struct sock *sk);
void (*tcp_error_report)(struct sock *sk);
+ void (*tcp_sock_destruct)(struct sock *sk);
struct rcu_head rcu;
};
@@ -761,6 +764,7 @@ mptcp_subflow_get_mapped_dsn(const struct mptcp_subflow_context *subflow)
}
void mptcp_subflow_process_delegated(struct sock *ssk, long actions);
+void mptcp_subflow_ctx_release(struct sock *ssk);
static inline void mptcp_subflow_delegate(struct mptcp_subflow_context *subflow, int action)
{
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 78e886b20258..8dc0677dc551 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -814,6 +814,33 @@ static void subflow_ulp_fallback(struct sock *sk,
mptcp_subflow_ops_undo_override(sk);
}
+static void subflow_ctx_destruct(struct sock *ssk)
+{
+ struct mptcp_subflow_context *ctx = mptcp_subflow_ctx(ssk);
+
+ rcu_assign_pointer(inet_csk(ssk)->icsk_ulp_data, NULL);
+ ctx->tcp_sock_destruct(ssk);
+ kfree_rcu(ctx, rcu);
+}
+
+/* Retire the context of a subflow socket that is being destroyed: a
+ * delegated action, or any other holder of a socket reference, can still
+ * reach the context, so free it together with the socket.
+ *
+ * Only an unhashed, dead socket may be retired this way. TCP never clones
+ * such a socket, so the overridden destructor can't leak into a child.
+ */
+void mptcp_subflow_ctx_release(struct sock *ssk)
+{
+ struct mptcp_subflow_context *ctx = mptcp_subflow_ctx(ssk);
+
+ if (WARN_ON_ONCE(ssk->sk_destruct == subflow_ctx_destruct))
+ return;
+
+ ctx->tcp_sock_destruct = ssk->sk_destruct;
+ ssk->sk_destruct = subflow_ctx_destruct;
+}
+
void mptcp_subflow_drop_ctx(struct sock *ssk)
{
struct mptcp_subflow_context *ctx = mptcp_subflow_ctx(ssk);
@@ -826,9 +853,20 @@ void mptcp_subflow_drop_ctx(struct sock *ssk)
subflow_ulp_fallback(ssk, ctx);
if (ctx->conn)
sock_put(ctx->conn);
- }
- kfree_rcu(ctx, rcu);
+ /* The socket lives on as a plain TCP socket: it must keep
+ * neither a stale ULP context nor an overridden destructor,
+ * so the context can't be retired through sk_destruct here.
+ * A delegated action may still be queued for it, though:
+ * hand the context over to mptcp_napi_poll() in that case.
+ */
+ if (!(set_mask_bits(&ctx->delegated_status, 0,
+ BIT(MPTCP_DELEGATE_RETIRED)) &
+ BIT(MPTCP_DELEGATE_SCHEDULED)))
+ kfree_rcu(ctx, rcu);
+ } else {
+ mptcp_subflow_ctx_release(ssk);
+ }
}
void __mptcp_subflow_fully_established(struct mptcp_sock *msk,
@@ -2087,7 +2125,7 @@ static void subflow_ulp_release(struct sock *ssk)
mptcp_subflow_ops_undo_override(ssk);
if (release)
- kfree_rcu(ctx, rcu);
+ mptcp_subflow_ctx_release(ssk);
}
static void subflow_ulp_clone(const struct request_sock *req,
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH net 2/2] mptcp: release the parent socket reference with the subflow context
2026-10-26 12:04 [PATCH net 0/2] mptcp: subflow context lifetime fixes Cen Zhang
2026-10-26 12:04 ` [PATCH net 1/2] mptcp: keep delegated subflow contexts alive until socket release Cen Zhang
@ 2026-10-26 12:04 ` Cen Zhang
2026-10-30 12:25 ` netdev-bot+sashiko
1 sibling, 1 reply; 6+ messages in thread
From: Cen Zhang @ 2026-10-26 12:04 UTC (permalink / raw)
To: matttbe, martineau, davem, edumazet, kuba, pabeni
Cc: geliang, horms, netdev, mptcp, linux-kernel, baijiaju1990,
jjzuming, zzzccc427
The subflow context owns a reference on its MPTCP parent socket through
ctx->conn, and subflow_ulp_release() drops it when the TCP protocol
state of the subflow is destroyed. Two users of the TCP subflow socket
can outlive that point and still follow the pointer.
bpf_skc_to_mptcp_sock() returns ctx->conn for any TCP socket that has
is_mptcp set. An SK_SKB program can look up a connected subflow with
bpf_sk_lookup_tcp() and hold the TCP child without holding the MPTCP
socket. A concurrent zero-linger close destroys the TCP protocol state
and releases the ULP despite that reference, and once the remaining
MPTCP references drain the program reads the freed MPTCP socket:
SK_SKB reader MPTCP close
rcu_read_lock()
bpf_sk_lookup_tcp() -> ssk
bpf_skc_to_mptcp_sock(ssk) -> msk
mptcp_close()
__mptcp_destroy_sock()
__mptcp_close_ssk()
__tcp_close()
subflow_ulp_release()
sock_put(ctx->conn)
final sock_put(msk)
read msk->token
A delegated action pins the TCP subflow socket, but
mptcp_subflow_process_delegated() locks and updates ctx->conn. A peer
reset followed by the application closing the MPTCP socket releases
the parent before mptcp_napi_poll() takes its data lock:
Delegation / NAPI Peer reset / application close
queue SEND or SNDBUF
sock_hold(ssk)
subflow_ulp_release(ssk)
sock_put(parent)
mptcp_close(parent)
last sock_put(parent)
mptcp_napi_poll()
mptcp_subflow_process_delegated()
mptcp_data_lock(parent)
Now that a retired context stays attached to its socket until
sk_destruct, release the parent reference from the context destructor
instead of from the ULP release. Every reference on the subflow socket
then also keeps the parent alive, and delegated actions need no
reference of their own. The final parent sock_put() may now run from
the subflow destructor in softirq or RCU callback context; the parent
is already closed and its protocol destroy callback has run by then, so
only the generic socket release is left. This depends on the previous
commit, which keeps the context alive until the subflow socket is
destroyed.
The hold around tcp_done() in mptcp_subflow_reset() stays, with its
comment updated: that call can destroy the subflow socket and thereby
release the parent reference.
Both reports below were taken with KASAN poisoning forced for the
SLAB_TYPESAFE_BY_RCU MPTCP socket cache. The first one is the BPF
reader, the second one the delegated action.
BUG: KASAN: slab-use-after-free in ___bpf_prog_run+0x2507/0xbcf0
Read of size 4 at addr ffff888119920930 by task mptcp_skb_looku/492
Call Trace:
<IRQ>
___bpf_prog_run+0x2507/0xbcf0
__bpf_prog_run32+0xda/0x120
bpf_prog_run_pin_on_cpu+0x136/0x3d0
sk_psock_verdict_recv+0x319/0x12c0
tcp_read_skb+0x349/0x500
sk_psock_verdict_data_ready+0x164/0x6f0
tcp_data_ready+0xfe/0x3c0
tcp_data_queue+0x181b/0x48e0
tcp_rcv_established+0x9c5/0x3480
tcp_v4_do_rcv+0x5bf/0xc60
tcp_v4_rcv+0x29cb/0x3da0
ip_protocol_deliver_rcu+0xad/0x430
ip_local_deliver_finish+0x383/0x610
ip_local_deliver+0x16a/0x1c0
ip_rcv+0x2d1/0x380
__netif_receive_skb_one_core+0x19c/0x1f0
__netif_receive_skb+0x22/0x150
process_backlog+0x43c/0x1260
__napi_poll.constprop.0+0xab/0x670
net_rx_action+0x4df/0xe60
handle_softirqs+0x201/0x930
do_softirq+0x47/0xa0
</IRQ>
Allocated by task 492:
kmem_cache_alloc_noprof+0x20b/0x5f0
sk_prot_alloc+0x65/0x260
sk_alloc+0x37/0xb40
inet_create+0x33a/0xf40
__sock_create+0x2c4/0x730
__sys_socket+0x141/0x210
__x64_sys_socket+0x77/0xc0
do_syscall_64+0x115/0x6a0
entry_SYSCALL_64_after_hwframe+0x77/0x7f
Freed by task 496:
kmem_cache_free+0x293/0x670
__sk_destruct+0x4dd/0x8f0
sk_destruct+0xb3/0xd0
__sk_free+0xdd/0x370
sk_free+0x51/0x80
mptcp_close+0x7d/0x100
inet_release+0x11f/0x250
__sock_release+0xb8/0x270
sock_close+0x21/0x30
__fput+0x39f/0xa60
fput_close_sync+0xff/0x200
__x64_sys_close+0x8c/0xf0
do_syscall_64+0x115/0x6a0
entry_SYSCALL_64_after_hwframe+0x77/0x7f
BUG: KASAN: slab-use-after-free in _raw_spin_lock_bh+0x38/0x50
Read of size 1 at addr ffff88810acf25d8 by task ksoftirqd/2/29
Call Trace:
<TASK>
lock_acquire+0x113/0x2f0
_raw_spin_lock_bh+0x38/0x50
mptcp_subflow_process_delegated+0xa6/0x9a0
mptcp_napi_poll+0x50d/0x12f0
__napi_poll.constprop.0+0xab/0x670
net_rx_action+0x4df/0xe60
handle_softirqs+0x201/0x930
run_ksoftirqd+0x33/0x60
smpboot_thread_fn+0x3bd/0x9a0
kthread+0x34f/0x460
ret_from_fork+0x659/0x940
ret_from_fork_asm+0x1a/0x30
</TASK>
Allocated by task 586:
kmem_cache_alloc_noprof+0x20b/0x5f0
sk_prot_alloc+0x65/0x260
sk_alloc+0x37/0xb40
inet_create+0x33a/0xf40
__sock_create+0x2c4/0x730
__sys_socket+0x141/0x210
__x64_sys_socket+0x77/0xc0
do_syscall_64+0x115/0x6a0
entry_SYSCALL_64_after_hwframe+0x77/0x7f
Freed by task 586:
kmem_cache_free+0x259/0x5a0
__sk_destruct+0x4a0/0x790
sk_destruct+0xb3/0xd0
__sk_free+0xdd/0x370
sk_free+0x51/0x80
mptcp_close+0x150/0x530
inet_release+0x11f/0x250
__sock_release+0xb8/0x270
sock_close+0x21/0x30
__fput+0x39f/0xa60
fput_close_sync+0xff/0x200
__x64_sys_close+0x8c/0xf0
do_syscall_64+0x115/0x6a0
entry_SYSCALL_64_after_hwframe+0x77/0x7f
Fixes: 3bc253c2e652 ("bpf: Add bpf_skc_to_mptcp_sock_proto")
Fixes: b19bc2945b40 ("mptcp: implement delegated actions")
Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
Assisted-by: LLM
---
net/mptcp/subflow.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 8dc0677dc551..ebcface15661 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -471,7 +471,9 @@ void mptcp_subflow_reset(struct sock *ssk)
if (ssk->sk_state == TCP_CLOSE)
return;
- /* must hold: tcp_done() could drop last reference on parent */
+ /* must hold: the parent reference owned by the subflow context is
+ * released together with ssk, which tcp_done() can destroy
+ */
sock_hold(sk);
subflow->resetting = 1;
@@ -820,6 +822,8 @@ static void subflow_ctx_destruct(struct sock *ssk)
rcu_assign_pointer(inet_csk(ssk)->icsk_ulp_data, NULL);
ctx->tcp_sock_destruct(ssk);
+ if (ctx->conn)
+ sock_put(ctx->conn);
kfree_rcu(ctx, rcu);
}
@@ -2120,7 +2124,6 @@ static void subflow_ulp_release(struct sock *ssk)
if (!release && !test_and_set_bit(MPTCP_WORK_CLOSE_SUBFLOW,
&mptcp_sk(sk)->flags))
mptcp_schedule_work(sk);
- sock_put(sk);
}
mptcp_subflow_ops_undo_override(ssk);
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net 1/2] mptcp: keep delegated subflow contexts alive until socket release
2026-10-26 12:04 ` [PATCH net 1/2] mptcp: keep delegated subflow contexts alive until socket release Cen Zhang
@ 2026-10-30 4:54 ` sashiko-bot
2026-10-30 12:25 ` netdev-bot+sashiko
1 sibling, 0 replies; 6+ messages in thread
From: sashiko-bot @ 2026-10-30 4:54 UTC (permalink / raw)
To: Cen Zhang; +Cc: mptcp
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] mptcp: mptcp_subflow_ctx_reset() clears delegated_status while node is queued
- [High] mptcp: subflow context leak when mptcp_subflow_drop_ctx() races with poll
--
commit 9ef5cf9c57c696888614efd45592ee8ab775f65d
Author: Cen Zhang <zzzccc427@gmail.com>
mptcp: keep delegated subflow contexts alive until socket release
This commit defers freeing retired MPTCP subflow contexts until socket
destruction via a chained sk_destruct callback, or hands live fallback
contexts over to mptcp_napi_poll() via MPTCP_DELEGATE_RETIRED so queued
delegated_node entries are not freed before being dequeued.
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 7a22e9388e0e9..01b94a280c810 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
[ ... ]
> @@ -4793,6 +4793,21 @@ static int mptcp_napi_poll(struct napi_struct *napi, int budget)
> struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
>
> bh_lock_sock_nested(ssk);
> + if (unlikely(!READ_ONCE(inet_csk(ssk)->icsk_ulp_ops))) {
> + /* The subflow was retired, by ULP release or by a
> + * fallback to plain TCP: nothing is left to process.
> + * The ops pointer is cleared with the socket owned or
> + * under its bh lock, so a racing read lands in the
> + * owned branch below instead.
> + * mptcp_subflow_drop_ctx() leaves a still queued
> + * context to us: free it now that it is dequeued.
> + */
> + if (xchg(&subflow->delegated_status, 0) &
> + BIT(MPTCP_DELEGATE_RETIRED))
> + kfree_rcu(subflow, rcu);
> + goto unlock;
> + }
> +
> if (!sock_owned_by_user(ssk)) {
> mptcp_subflow_process_delegated(ssk, xchg(&subflow->delegated_status, 0));
> } else {
> @@ -4804,6 +4819,7 @@ static int mptcp_napi_poll(struct napi_struct *napi, int budget)
> smp_wmb();
> clear_bit(MPTCP_DELEGATE_SCHEDULED, &subflow->delegated_status);
> }
[Severity: High]
Does this race with mptcp_subflow_drop_ctx() and leak struct
mptcp_subflow_context when a read of icsk_ulp_ops lands in the owned branch?
When an unaccepted MPTCP socket is force-closed during listener teardown via
mptcp_subflow_queue_clean() -> __mptcp_unaccepted_force_close() ->
__mptcp_destroy_sock() -> mptcp_destroy() -> mptcp_destroy_common() ->
__mptcp_close_ssk(), __mptcp_close_ssk() acquires lock_sock_nested(ssk,
SINGLE_DEPTH_NESTING) and calls mptcp_subflow_drop_ctx(ssk):
net/mptcp/protocol.c:__mptcp_close_ssk() {
lock_sock_nested(ssk, SINGLE_DEPTH_NESTING);
...
if (msk->in_accept_queue && msk->first == ssk &&
(sock_flag(sk, SOCK_DEAD) || sock_flag(ssk, SOCK_DEAD))) {
sock_set_flag(sk, SOCK_DEAD);
mptcp_set_close_tout(sk, tcp_jiffies32 - (mptcp_close_timeout(sk) + 1));
mptcp_subflow_drop_ctx(ssk);
goto out_release;
}
}
Because lock_sock_nested() only sets ssk->sk_lock.owned = 1 and drops the BH
spinlock ssk->sk_lock.slock, mptcp_subflow_drop_ctx() runs without holding
bh_lock_sock(ssk).
If a delegated action was scheduled on ssk, mptcp_napi_poll() on the
scheduling CPU can concurrently dequeue subflow and take
bh_lock_sock_nested(ssk):
CPU0 (mptcp_napi_poll) CPU1 (__mptcp_close_ssk)
bh_lock_sock_nested(ssk)
READ_ONCE(inet_csk(ssk)->icsk_ulp_ops)
(sees non-NULL)
mptcp_subflow_drop_ctx(ssk)
subflow_ulp_fallback(ssk, ctx)
icsk->icsk_ulp_ops = NULL
rcu_assign_pointer(icsk_ulp_data, NULL)
set_mask_bits(&ctx->delegated_status, 0,
BIT(MPTCP_DELEGATE_RETIRED))
(sees MPTCP_DELEGATE_SCHEDULED set,
skips kfree_rcu)
sock_owned_by_user(ssk) is true
clear_bit(MPTCP_DELEGATE_SCHEDULED,
&subflow->delegated_status)
In this window, mptcp_subflow_drop_ctx() observes MPTCP_DELEGATE_SCHEDULED
set and expects mptcp_napi_poll() to free ctx, while the owned branch in
mptcp_napi_poll() only clears MPTCP_DELEGATE_SCHEDULED without checking
MPTCP_DELEGATE_RETIRED or calling kfree_rcu(subflow, rcu). Since
subflow_ulp_fallback() already cleared icsk_ulp_data and restored
ssk->sk_prot, neither side frees ctx.
[ ... ]
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 78e886b202585..8dc0677dc5514 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
[ ... ]
> @@ -826,9 +853,20 @@ void mptcp_subflow_drop_ctx(struct sock *ssk)
> subflow_ulp_fallback(ssk, ctx);
> if (ctx->conn)
> sock_put(ctx->conn);
> - }
>
> - kfree_rcu(ctx, rcu);
> + /* The socket lives on as a plain TCP socket: it must keep
> + * neither a stale ULP context nor an overridden destructor,
> + * so the context can't be retired through sk_destruct here.
> + * A delegated action may still be queued for it, though:
> + * hand the context over to mptcp_napi_poll() in that case.
> + */
> + if (!(set_mask_bits(&ctx->delegated_status, 0,
> + BIT(MPTCP_DELEGATE_RETIRED)) &
> + BIT(MPTCP_DELEGATE_SCHEDULED)))
> + kfree_rcu(ctx, rcu);
[Severity: Critical]
Can mptcp_subflow_ctx_reset() clear MPTCP_DELEGATE_SCHEDULED while
ctx->delegated_node is still queued, causing mptcp_subflow_drop_ctx() and
mptcp_napi_poll() to both free ctx or trigger a use-after-free?
In struct mptcp_subflow_context, delegated_status is inside
struct_group(reset, ...), whereas delegated_node is outside it:
net/mptcp/protocol.h:
struct mptcp_subflow_context {
struct list_head node;/* conn_list of subflows */
struct_group(reset,
...
long delegated_status;
unsigned long fail_tout;
);
struct list_head delegated_node;
...
};
static inline void
mptcp_subflow_ctx_reset(struct mptcp_subflow_context *subflow)
{
memset(&subflow->reset, 0, sizeof(subflow->reset));
subflow->request_mptcp = 1;
WRITE_ONCE(subflow->local_id, -1);
}
If an unaccepted passive MPTCP connection's first subflow (msk->in_accept_queue
set, msk->first == ssk, msk->free_first == 0) has a delegated action queued
on mptcp_delegated_actions and transitions to TCP_CLOSE (for example, on an
incoming peer RST via tcp_reset() -> tcp_done_with_error() -> tcp_done())
before accept() or listener shutdown, sock_flag(sk, SOCK_DEAD) and
sock_flag(ssk, SOCK_DEAD) are both false.
When mptcp_worker() runs __mptcp_close_subflow() -> mptcp_close_ssk() ->
__mptcp_close_ssk(), dispose_it (msk->free_first || ssk != msk->first) is
false, so __mptcp_close_ssk() calls __mptcp_subflow_disconnect():
net/mptcp/protocol.c:__mptcp_subflow_disconnect() {
if (((1 << ssk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN)) ||
fastclosing) {
WARN_ON_ONCE(tcp_disconnect(ssk, 0));
mptcp_subflow_ctx_reset(subflow);
}
}
mptcp_subflow_ctx_reset(subflow) zeroes subflow->delegated_status (clearing
MPTCP_DELEGATE_SCHEDULED) while subflow->delegated_node remains linked in the
per-CPU mptcp_delegated_actions list.
When the listener is later closed via mptcp_subflow_queue_clean() ->
__mptcp_unaccepted_force_close() -> __mptcp_destroy_sock() ->
mptcp_destroy_common() -> __mptcp_close_ssk(), sock_flag(sk, SOCK_DEAD) is
now set, so __mptcp_close_ssk() calls mptcp_subflow_drop_ctx(ssk). Because
MPTCP_DELEGATE_SCHEDULED was wiped by mptcp_subflow_ctx_reset(),
set_mask_bits() sets MPTCP_DELEGATE_RETIRED and returns 0, causing
mptcp_subflow_drop_ctx() to immediately call kfree_rcu(ctx, rcu) while
ctx->delegated_node is still queued.
When mptcp_napi_poll() later processes the queued entry, it either
dereferences freed memory in mptcp_subflow_delegated_next() after the RCU
grace period or observes MPTCP_DELEGATE_RETIRED set and invokes
kfree_rcu(subflow, rcu) a second time on the same rcu_head.
--
Sashiko AI review · https://sashiko.dev/#/patchset/pm-mptcp-objects-candidate-0010-v3-14c229ea28a7d31f60a0-0@gmail.com?part=1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net 1/2] mptcp: keep delegated subflow contexts alive until socket release
2026-10-26 12:04 ` [PATCH net 1/2] mptcp: keep delegated subflow contexts alive until socket release Cen Zhang
2026-10-30 4:54 ` sashiko-bot
@ 2026-10-30 12:25 ` netdev-bot+sashiko
1 sibling, 0 replies; 6+ messages in thread
From: netdev-bot+sashiko @ 2026-10-30 12:25 UTC (permalink / raw)
To: zzzccc427
Cc: matttbe, martineau, davem, edumazet, kuba, pabeni, geliang, horms,
netdev, mptcp, linux-kernel, baijiaju1990, jjzuming
Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider.
Critical: 1 · High: 1 · Medium: 1 · Low: 2
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [Critical] The patch rests on an invariant that does not hold, and the
result is a double free / use-after-free of the listener's subflow…
- [High] The fallback retirement handshake in mptcp_subflow_drop_ctx()
relies on MPTCP_DELEGATE_SCHEDULED to tell whether the ctx is still on…
- [Medium] Memory leak: the RETIRED/SCHEDULED handshake loses the free
when mptcp_napi_poll() takes its owned branch.
- [Low] Mismatched access annotations on icsk_ulp_ops.
- [Low] Stale comment in subflow_ulp_release() (net/mptcp/subflow.c
around line 2115-2117).
--
mptcp: keep delegated subflow contexts alive until socket release
This patch moves the free of a retired mptcp_subflow_context into a
chained sk_destruct callback, so a queued delegated action can no longer
unlink freed memory. mptcp_napi_poll() now skips subflows whose ULP ops
were cleared. For the fallback branch of mptcp_subflow_drop_ctx(), a new
MPTCP_DELEGATE_RETIRED bit hands the free over to mptcp_napi_poll() when
an action is still scheduled.
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 7a22e9388e0e9..01b94a280c810 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
[ ... ]
> @@ -4793,6 +4793,21 @@ static int mptcp_napi_poll(struct napi_struct *napi, int budget)
> struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
>
> bh_lock_sock_nested(ssk);
> + if (unlikely(!READ_ONCE(inet_csk(ssk)->icsk_ulp_ops))) {
[Severity: Low]
The comment below says this lockless read races with the store that clears
icsk_ulp_ops. Both writers use plain stores, though:
net/mptcp/subflow.c:subflow_ulp_fallback() {
...
icsk->icsk_ulp_ops = NULL;
...
}
net/ipv4/tcp_ulp.c:tcp_cleanup_ulp() {
...
icsk->icsk_ulp_ops = NULL;
...
}
The mptcp_subflow_drop_ctx() caller in __mptcp_close_ssk() holds socket
ownership but not slock. That writer really does run concurrently with
this reader.
Should those stores be WRITE_ONCE() to pair with this READ_ONCE()?
> + /* The subflow was retired, by ULP release or by a
> + * fallback to plain TCP: nothing is left to process.
> + * The ops pointer is cleared with the socket owned or
> + * under its bh lock, so a racing read lands in the
> + * owned branch below instead.
> + * mptcp_subflow_drop_ctx() leaves a still queued
> + * context to us: free it now that it is dequeued.
> + */
> + if (xchg(&subflow->delegated_status, 0) &
> + BIT(MPTCP_DELEGATE_RETIRED))
> + kfree_rcu(subflow, rcu);
> + goto unlock;
> + }
> +
> if (!sock_owned_by_user(ssk)) {
> mptcp_subflow_process_delegated(ssk, xchg(&subflow->delegated_status, 0));
> } else {
> @@ -4804,6 +4819,7 @@ static int mptcp_napi_poll(struct napi_struct *napi, int budget)
> smp_wmb();
> clear_bit(MPTCP_DELEGATE_SCHEDULED, &subflow->delegated_status);
[Severity: Medium]
Can the subflow context leak when this owned branch races with the
fallback branch of mptcp_subflow_drop_ctx()?
mptcp_subflow_drop_ctx() is reached at listener shutdown through
mptcp_subflow_queue_clean()->__mptcp_unaccepted_force_close()->
__mptcp_destroy_sock()->mptcp_destroy_common()->__mptcp_close_ssk(). It
runs with the ssk owned via lock_sock_nested(), but without
sk_lock.slock, so it can run concurrently with this loop:
CPU0 (mptcp_napi_poll) CPU1 (mptcp_subflow_drop_ctx)
READ_ONCE(icsk_ulp_ops) != NULL
sock_owned_by_user() is true
subflow_ulp_fallback()
icsk->icsk_ulp_ops = NULL;
set_mask_bits(..., RETIRED)
old value has SCHEDULED,
kfree_rcu() is skipped
clear_bit(MPTCP_DELEGATE_SCHEDULED)
RETIRED is never looked at
After this, neither side frees the ctx. It is also unreachable at that
point: icsk_ulp_data is NULL, the ctx is off conn_list, and sk_destruct
is the original TCP one.
The commit message says "a racing read lands in the owned branch, which
only dequeues the entry" and "exactly one of them frees the context". The
owned branch does not take part in the RETIRED handoff, though.
Would a fetch-style operation in the owned branch close this window?
if (set_mask_bits(&subflow->delegated_status,
BIT(MPTCP_DELEGATE_SCHEDULED), 0) &
BIT(MPTCP_DELEGATE_RETIRED))
kfree_rcu(subflow, rcu);
> }
> +unlock:
> bh_unlock_sock(ssk);
> sock_put(ssk);
>
[ ... ]
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 78e886b202585..8dc0677dc5514 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -814,6 +814,33 @@ static void subflow_ulp_fallback(struct sock *sk,
> mptcp_subflow_ops_undo_override(sk);
> }
>
> +static void subflow_ctx_destruct(struct sock *ssk)
> +{
> + struct mptcp_subflow_context *ctx = mptcp_subflow_ctx(ssk);
> +
> + rcu_assign_pointer(inet_csk(ssk)->icsk_ulp_data, NULL);
> + ctx->tcp_sock_destruct(ssk);
> + kfree_rcu(ctx, rcu);
> +}
> +
> +/* Retire the context of a subflow socket that is being destroyed: a
> + * delegated action, or any other holder of a socket reference, can still
> + * reach the context, so free it together with the socket.
> + *
> + * Only an unhashed, dead socket may be retired this way. TCP never clones
> + * such a socket, so the overridden destructor can't leak into a child.
> + */
> +void mptcp_subflow_ctx_release(struct sock *ssk)
> +{
> + struct mptcp_subflow_context *ctx = mptcp_subflow_ctx(ssk);
> +
> + if (WARN_ON_ONCE(ssk->sk_destruct == subflow_ctx_destruct))
> + return;
> +
> + ctx->tcp_sock_destruct = ssk->sk_destruct;
> + ssk->sk_destruct = subflow_ctx_destruct;
> +}
[Severity: Critical]
Does "TCP never clones such a socket" hold for an MPTCP listener subflow?
On listener close, __mptcp_close_ssk() on the listener ssk (disposable=1)
goes through:
__tcp_close()
inet_csk_destroy_sock()
tcp_cleanup_ulp()
subflow_ulp_release()
mptcp_subflow_ctx_release()
That installs subflow_ctx_destruct on the listener before
tcp_cleanup_ulp() clears icsk_ulp_ops. icsk_ulp_data still points at the
listener ctx at that point.
Meanwhile, tcp_v4_rcv() (and tcp_v6_rcv()) checks the listener state
without holding the listener lock:
net/ipv4/tcp_ipv4.c:tcp_v4_rcv() {
...
if (unlikely(sk->sk_state != TCP_LISTEN)) {
nsk = reuseport_migrate_sock(sk, req_to_sk(req), skb);
...
}
If that check passes, the path continues through tcp_check_req()->
subflow_syn_recv_sock()->tcp_v4_syn_recv_sock()->inet_csk_clone_lock()->
sk_clone()->sock_copy(). sock_copy() copies sk_destruct, icsk_ulp_ops and
icsk_ulp_data into the child. TCP expects children of a closing listener:
inet_csk_reqsk_queue_add() handles them with inet_child_forget().
If the copy happens after tcp_cleanup_ulp(), the child has icsk_ulp_ops
NULL, so inet_clone_ulp() does nothing. It also has icsk_ulp_data equal to
the listener ctx and sk_destruct equal to subflow_ctx_destruct.
mptcp_subflow_drop_ctx(child) in subflow_syn_recv_sock() ends up in the
WARN_ON_ONCE() above and returns without fixing anything.
When the child is freed, subflow_ctx_destruct(child) calls kfree_rcu() on
the listener ctx. Later the listener's own subflow_ctx_destruct() reads
ctx->tcp_sock_destruct from freed memory, calls through it, and frees the
ctx a second time.
The old mptcp_subflow_drop_ctx() already freed the inherited ctx on the
fallback path, so that part partly predates this patch. The inherited
destructor is new, though, and it also turns the non-fallback
inet_child_forget() child path into a double free.
If the copy instead lands between the sk_destruct override and
icsk_ulp_ops = NULL, subflow_ulp_clone() runs for the child. It either
falls back, leaving icsk_ulp_data NULL so subflow_ctx_destruct()
dereferences a NULL ctx, or it allocates a zeroed ctx whose
tcp_sock_destruct is NULL, which subflow_ctx_destruct() then calls.
The parent msk reference may also be dropped together with the ctx, as
in "mptcp: release the parent socket reference with the subflow context".
In that case the child's destructor would also put the listener msk one
extra time.
> void mptcp_subflow_drop_ctx(struct sock *ssk)
> {
> struct mptcp_subflow_context *ctx = mptcp_subflow_ctx(ssk);
> @@ -826,9 +853,20 @@ void mptcp_subflow_drop_ctx(struct sock *ssk)
> subflow_ulp_fallback(ssk, ctx);
> if (ctx->conn)
> sock_put(ctx->conn);
> - }
>
> - kfree_rcu(ctx, rcu);
> + /* The socket lives on as a plain TCP socket: it must keep
> + * neither a stale ULP context nor an overridden destructor,
> + * so the context can't be retired through sk_destruct here.
> + * A delegated action may still be queued for it, though:
> + * hand the context over to mptcp_napi_poll() in that case.
> + */
> + if (!(set_mask_bits(&ctx->delegated_status, 0,
> + BIT(MPTCP_DELEGATE_RETIRED)) &
> + BIT(MPTCP_DELEGATE_SCHEDULED)))
> + kfree_rcu(ctx, rcu);
[Severity: High]
Can MPTCP_DELEGATE_SCHEDULED be clear here while delegated_node is still
on a per-CPU list?
delegated_status sits inside the reset struct_group and delegated_node
sits outside it. So mptcp_subflow_ctx_reset() clears SCHEDULED without
unlinking the node:
net/mptcp/protocol.h:mptcp_subflow_ctx_reset() {
memset(&subflow->reset, 0, sizeof(subflow->reset));
...
}
One possible sequence:
1. A delegated action (for example MPTCP_DELEGATE_SNDBUF) is queued for
the first subflow of an unaccepted msk. NAPI on that CPU is delayed,
which is the same premise as the scenario in the commit message.
2. The peer sends MP_FASTCLOSE. mptcp_worker()->mptcp_check_fastclose()
moves the ssk to TCP_CLOSE without marking it SOCK_DEAD.
3. __mptcp_close_subflow()->mptcp_close_ssk()->__mptcp_close_ssk() runs.
Neither socket is SOCK_DEAD and dispose_it is false, so
__mptcp_subflow_disconnect()->mptcp_subflow_ctx_reset() zeroes
delegated_status.
4. The listener is closed. mptcp_subflow_queue_clean()->
__mptcp_unaccepted_force_close()->__mptcp_destroy_sock()->
mptcp_destroy_common()->__mptcp_close_ssk()->mptcp_subflow_drop_ctx()
takes this fallback branch. set_mask_bits() returns a value without
SCHEDULED, so kfree_rcu(ctx) runs while the node is still queued.
5. After the grace period, mptcp_napi_poll()->
mptcp_subflow_delegated_next() does list_del_init() on freed memory.
Isn't that the same use-after-free this patch is meant to close? It looks
like the handoff needs queue-ownership state that survives a ctx reset.
> + } else {
> + mptcp_subflow_ctx_release(ssk);
> + }
> }
>
> void __mptcp_subflow_fully_established(struct mptcp_sock *msk,
> @@ -2087,7 +2125,7 @@ static void subflow_ulp_release(struct sock *ssk)
>
> mptcp_subflow_ops_undo_override(ssk);
> if (release)
> - kfree_rcu(ctx, rcu);
> + mptcp_subflow_ctx_release(ssk);
> }
[Severity: Low]
This isn't a bug, but the comment earlier in subflow_ulp_release() still
says:
/* if the msk has been orphaned, keep the ctx
* alive, will be freed by __mptcp_close_ssk(),
* when the subflow is still unaccepted
*/
After this patch, neither __mptcp_close_ssk() nor the !icsk_ulp_ops
branch of mptcp_subflow_drop_ctx() frees the ctx. Both call
mptcp_subflow_ctx_release(), and the actual free happens later in
subflow_ctx_destruct().
Should this comment be updated along with the one for disposable?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/pm-mptcp-objects-candidate-0010-v3-14c229ea28a7d31f60a0-0%40gmail.com
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net 2/2] mptcp: release the parent socket reference with the subflow context
2026-10-26 12:04 ` [PATCH net 2/2] mptcp: release the parent socket reference with the subflow context Cen Zhang
@ 2026-10-30 12:25 ` netdev-bot+sashiko
0 siblings, 0 replies; 6+ messages in thread
From: netdev-bot+sashiko @ 2026-10-30 12:25 UTC (permalink / raw)
To: zzzccc427
Cc: matttbe, martineau, davem, edumazet, kuba, pabeni, geliang, horms,
netdev, mptcp, linux-kernel, baijiaju1990, jjzuming
Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.
Critical: 1 · High: 1 · Medium: 1 · Low: 1
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [Critical] The new sock_put(ctx->conn) sits in subflow_ctx_destruct(),
and a TCP child cloned from a retired MPTCP listener subflow can…
- [High] The fix is incomplete.
- [Medium] The commit message presents the delegated-action use-after-
free as a live bug that this patch fixes: the second interleaving, the…
- [Low] The comment in subflow_ulp_release()
(net/mptcp/subflow.c:2115-2118) says the context 'will be freed by…
--
I'm sending this review as a reply to the patch on the list. It has four inline comments:
- **Medium:** the commit message credits this patch with a fix that the previous commit in the series already makes.
- **High:** mptcp_subflow_drop_ctx() still drops the parent reference immediately, so the fix is incomplete.
- **Critical:** a possible double free of the subflow context when a child is cloned from a listener that is being closed.
- **Low:** a stale comment.
mptcp: release the parent socket reference with the subflow context
This commit moves the sock_put() on the MPTCP parent socket from
subflow_ulp_release() into subflow_ctx_destruct(). The parent reference
then lives as long as the subflow socket. The aim is to fix use-after-free
in bpf_skc_to_mptcp_sock() readers and in delegated action processing,
building on the previous commit that keeps a retired context attached
until sk_destruct.
> A delegated action pins the TCP subflow socket, but
> mptcp_subflow_process_delegated() locks and updates ctx->conn. A peer
> reset followed by the application closing the MPTCP socket releases
> the parent before mptcp_napi_poll() takes its data lock:
>
> Delegation / NAPI Peer reset / application close
> queue SEND or SNDBUF
> sock_hold(ssk)
> subflow_ulp_release(ssk)
> sock_put(parent)
> mptcp_close(parent)
> last sock_put(parent)
> mptcp_napi_poll()
> mptcp_subflow_process_delegated()
> mptcp_data_lock(parent)
[ ... ]
> Fixes: b19bc2945b40 ("mptcp: implement delegated actions")
[Severity: Medium]
Can this delegated action sequence still happen on the tree this patch
applies to?
The previous commit in the series, "mptcp: keep delegated subflow contexts
alive until socket release", added this guard to mptcp_napi_poll() in
net/mptcp/protocol.c:
bh_lock_sock_nested(ssk);
if (unlikely(!READ_ONCE(inet_csk(ssk)->icsk_ulp_ops))) {
...
goto unlock;
}
Before this patch, the parent sock_put() ran in subflow_ulp_release().
That function is called from tcp_cleanup_ulp() right before:
icsk->icsk_ulp_ops = NULL;
At that point the subflow is owned by the user or bh-locked. So
mptcp_napi_poll() sees one of three states:
- An attached ULP, where ctx->conn still pins the parent.
- An owned socket, where it only clears the scheduled bit.
- NULL ops, where it skips the entry.
Also, tcp_release_cb_override() isn't reached after the ULP release,
because mptcp_subflow_ops_undo_override() restores sk_prot.
If that's right, this patch only fixes the bpf_skc_to_mptcp_sock() case.
For delegated actions it is defence in depth.
Should the second interleaving, the second KASAN report and the
b19bc2945b40 Fixes tag move to the previous commit instead?
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 8dc0677dc5514..ebcface15661a 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
[ ... ]
> @@ -820,6 +822,8 @@ static void subflow_ctx_destruct(struct sock *ssk)
>
> rcu_assign_pointer(inet_csk(ssk)->icsk_ulp_data, NULL);
> ctx->tcp_sock_destruct(ssk);
> + if (ctx->conn)
> + sock_put(ctx->conn);
> kfree_rcu(ctx, rcu);
> }
[Severity: High]
The commit message says "Every reference on the subflow socket then also
keeps the parent alive". Does that hold for mptcp_subflow_drop_ctx()?
That function still drops the parent reference right away when the ULP
is attached:
net/mptcp/subflow.c:mptcp_subflow_drop_ctx() {
...
if (inet_csk(ssk)->icsk_ulp_ops) {
subflow_ulp_fallback(ssk, ctx);
if (ctx->conn)
sock_put(ctx->conn);
...
}
In that branch the TCP socket stays hashed as plain TCP, so a reference
on it no longer pins the msk. bpf_mptcp_sock_from_subflow() returns
ctx->conn without taking a reference of its own.
One way to reach it is closing a listener that has an unaccepted
connection:
mptcp_check_listen_stop()
mptcp_subflow_queue_clean()
__mptcp_unaccepted_force_close(msk)
mptcp_do_fastclose(msk) <- only sends RST, child stays in ehash
__mptcp_destroy_sock(msk)
mptcp_destroy() <- sets free_first
mptcp_destroy_common()
__mptcp_close_ssk(first) <- in_accept_queue branch
mptcp_subflow_drop_ctx(first)
sock_put(msk)
sock_put(msk)
mptcp_cancel_work(msk)
sock_put(msk) <- msk freed
Suppose a TC or SK_SKB program on another CPU called bpf_sk_lookup_tcp()
on that 4-tuple, then bpf_skc_to_mptcp_sock(), before the fallback. It
holds only the TCP reference. Wouldn't it then read the freed msk, the
same way as in the first KASAN report?
The dispose_child label in subflow_syn_recv_sock() looks like a second
path. It is reached after:
ctx->conn = (struct sock *)owner;
and by then tcp_v4_syn_recv_sock() has already hashed the child.
Does this put also need to be deferred, for example past an RCU grace
period, or otherwise tied to the TCP socket's lifetime?
[Severity: Critical]
Can a TCP child cloned from a retired MPTCP listener subflow inherit
subflow_ctx_destruct()? If so, it would run this new sock_put() on the
listener's ctx.
The comment on mptcp_subflow_ctx_release() from the previous commit says
"TCP never clones such a socket". However, tcp_v4_rcv() checks the
listener state without holding the listener lock:
CPU1 (softirq)
tcp_v4_rcv()
if (unlikely(sk->sk_state != TCP_LISTEN)) { ... } <- passes
sock_hold(sk)
tcp_check_req()
->syn_recv_sock()
inet_csk_clone_lock()
sk_clone_lock()
CPU2
mptcp_close()
mptcp_check_listen_stop()
__mptcp_destroy_sock()
mptcp_destroy() <- free_first = 1
__mptcp_close_ssk(listener ssk)
__tcp_close()
inet_csk_destroy_sock()
tcp_cleanup_ulp()
subflow_ulp_release() <- release == true
mptcp_subflow_ctx_release()
ssk->sk_destruct = subflow_ctx_destruct;
icsk->icsk_ulp_ops = NULL;
tcp_cleanup_ulp() leaves icsk_ulp_data pointing at the listener's ctx.
Suppose CPU2 gets this far before sk_clone_lock() copies the listener:
- sk_clone_lock() keeps the inherited sk_destruct on success. It only
clears it on the failure path.
- inet_clone_ulp() returns early because icsk_ulp_ops is NULL.
The child then has sk_destruct == subflow_ctx_destruct, icsk_ulp_data
pointing at the listener's ctx, and is_mptcp set.
inet_csk_reqsk_queue_add() then sees that the listener is no longer
TCP_LISTEN and calls inet_child_forget(). When the child is freed,
subflow_ctx_destruct(child) runs ctx->tcp_sock_destruct, then
sock_put(ctx->conn) on the listener msk, then kfree_rcu(ctx).
Later the listener ssk is destroyed, and subflow_ctx_destruct() runs
again on the same ctx. Wouldn't that call ctx->tcp_sock_destruct through
a pointer read from memory that may already be freed? It would also put
the listener msk a second time and kfree_rcu() the ctx twice.
The destructor inheritance comes from the previous commit in this series.
This patch adds the extra put on the listener msk on top of it.
[ ... ]
> @@ -2120,7 +2124,6 @@ static void subflow_ulp_release(struct sock *ssk)
> if (!release && !test_and_set_bit(MPTCP_WORK_CLOSE_SUBFLOW,
> &mptcp_sk(sk)->flags))
> mptcp_schedule_work(sk);
> - sock_put(sk);
> }
>
> mptcp_subflow_ops_undo_override(ssk);
[Severity: Low]
This isn't a bug, but the comment a few lines above in
subflow_ulp_release() still says:
/* if the msk has been orphaned, keep the ctx
* alive, will be freed by __mptcp_close_ssk(),
* when the subflow is still unaccepted
*/
After this series, __mptcp_close_ssk() only retires the context through
mptcp_subflow_ctx_release(). The context is freed in subflow_ctx_destruct()
when the subflow socket is destroyed, and with this patch the parent
reference is dropped there too.
Should this comment be updated?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/pm-mptcp-objects-candidate-0010-v3-14c229ea28a7d31f60a0-0%40gmail.com
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-10-30 12:25 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-26 12:04 [PATCH net 0/2] mptcp: subflow context lifetime fixes Cen Zhang
2026-10-26 12:04 ` [PATCH net 1/2] mptcp: keep delegated subflow contexts alive until socket release Cen Zhang
2026-10-30 4:54 ` sashiko-bot
2026-10-30 12:25 ` netdev-bot+sashiko
2026-10-26 12:04 ` [PATCH net 2/2] mptcp: release the parent socket reference with the subflow context Cen Zhang
2026-10-30 12:25 ` netdev-bot+sashiko
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.