* [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
* 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
* [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 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.