Re: [PATCH net v2 1/1] mptcp: fix request ownership when cloning reqsk

[Matthieu Baerts <matttbe@kernel.org>]

Hi Ren, Ruide,

On 13/05/2026 11:37, Paolo Abeni wrote:
> On 5/10/26 4:14 PM, Ren Wei wrote:
>> From: Ruide Cao <caoruide123@gmail.com>
>>
>> TCP request migration clones pending request sockets with
>> inet_reqsk_clone(). For MPTCP MP_JOIN requests this raw-copies
>> subflow_req->msk, but the cloned request does not take a new reference.
>>
>> Both the original and the cloned request can later drop the same msk in
>> subflow_req_destructor(), and a migrated request may keep a dangling msk
>> pointer after the original owner has already been released.
>>
>> MP_CAPABLE requests have a similar ownership issue for token_node. The
>> original request is hashed in the token table, and inet_reqsk_clone()
>> raw-copies the hlist pointers into the clone. The clone must not inherit
>> that hashed state on migration failure, and the token ownership must move
>> only after the new req has actually replaced the old one in the ehash.
>>
>> Fix both cases while keeping the change local to MPTCP and the TCP
>> migration path: grab a reference for cloned subflow requests that carry an
>> msk, clear any raw-copied token hash state from the clone, and move the
>> hashed token request ownership only after migration succeeds.

(...)

>> diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
>> index e961936b6be7..578471bb8a2b 100644
>> --- a/net/ipv4/inet_connection_sock.c
>> +++ b/net/ipv4/inet_connection_sock.c

(...)

>> @@ -1115,6 +1121,8 @@ static void reqsk_timer_handler(struct timer_list *t)
>>  			goto no_ownership;
>>  		}
>>  
>> +		if (rsk_is_mptcp(oreq))
>> +			mptcp_subflow_reqsk_migrated(oreq, nreq);
> 
> Sashiko says:
> 
> ---
> Can this sequence trigger a race condition with network RX processing?
> In reqsk_timer_handler(), nreq is inserted into the global ehash table
> via inet_ehash_insert() right before this block. The ehash lock is
> immediately dropped, making nreq globally visible to concurrent network
> RX softirq processing.
> However, the MPTCP token state is not migrated to nreq until the call to
> mptcp_subflow_reqsk_migrated(oreq, nreq).
> If a concurrent RX thread processes an incoming ACK for the connection
> in this narrow window, it could find nreq in the ehash table,
> successfully process it, and invoke mptcp_token_accept(nreq).
> Inside mptcp_token_accept(), __token_lookup_req() searches the token
> bucket and finds oreq, because the timer thread hasn't moved the token yet.
> The code then asserts ownership:
> mptcp_token_accept()
> 	pos = __token_lookup_req(bucket, req->token);
> 	if (!WARN_ON_ONCE(pos != req))
> 		hlist_nulls_del_init_rcu(&req->token_node);
> Since pos is oreq and req is nreq, would this fail the assertion and
> trigger a kernel warning?
> If the calls were reordered so that mptcp_subflow_reqsk_migrated()
> happens before inet_ehash_insert(), could that merely shift the race? If
> a concurrent ACK is already processing oreq, it might call
> mptcp_token_accept(oreq) after the token was moved to nreq, triggering
> the same warning, since the ehash table and the token bucket are
> protected by different locks and the updates are non-atomic.
> Could a remote attacker time ACKs to hit this window during a listener
> migration, potentially leading to a denial of service on systems
> configured to panic on warnings?
> ---
> 
> I _think_ the above race could/should be solved squashing
> mptcp_subflow_reqsk_migrated() into mptcp_subflow_reqsk_clone() and
> adjusting  mptcp_token_destroy_request() and mptcp_token_accept() to
> deal with already removed token, checking (again) for unhased token_node
> under the bucket lock.
> 
> The latter will require a little more work, returning a success status
> to the caller - mptcp_sk_clone_init() - and make the latter fail on
> mptcp_token_accept() failures.
> 
> The above would also make the fix more encapsulated into the mptcp code.

I was checking old patches tracked on our MPTCP Patchwork, and noticed
this one is marked as "Changes Requested". I don't think a v3 has been
posted: are you still working on a v3 addressing Paolo's comments? Or
did I miss the v3 because it had a different name?

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