Re: [PATCH v2 net] tcp: wait for TIME_WAIT teardown before freeing per-netns ehash

[Jiayuan Chen <jiayuan.chen@linux.dev>]

On 10/30/26 3:58 PM, Eric Dumazet wrote:
> From: Kyle Zeng <kylebot@openai.com>
>
> inet_twsk_kill() removes a TIME_WAIT socket from the established hash
> before it finishes using the ehash lock, hashinfo and bind tables, and
> before it drops the death-row reference. A running timer can therefore
> unhash its socket before inet_twsk_purge() reaches it. The purge cannot
> synchronize with that timer, and tcp_sk_exit_batch() can free the
> private ehash and hashinfo while inet_twsk_kill() is still using them.
>
> An unprivileged reproducer using user and network namespaces, a private
> TCP ehash and a FIN_WAIT2 timer triggers a KASAN slab-use-after-free in
> inet_twsk_kill(). The allocation and free stacks point to
> inet_pernet_hashinfo_alloc() and inet_pernet_hashinfo_free().
>
> Wait for an RCU grace period after purging the whole batch, if one
> netns in the batch uses a private ehash. TIME_WAIT removal runs with
> bottom halves disabled, so this also waits for removers that were
> already invisible to the purge. Keep the wait under
> tcp_exit_batch_mutex, before freeing any private hashinfo.
>
> The same window exists with the shared tcp_hashinfo. There, the
> hashinfo memory stays valid, but tcp_sk_exit_batch() can decrement
> tcp_death_row.tw_refcount from 2 to 1 before inet_twsk_kill() drops its
> own reference. This triggers the WARN_ON_ONCE() in tcp_sk_exit_batch(),
> then a refcount_t "decrement hit 0" warning from inet_twsk_kill().
>
> Adding a grace period for the shared case would be costly on busy
> hosts. It is not needed: struct net is freed by a later cleanup_net(),
> after the synchronize_rcu() in ops_undo_list(), so inet_twsk_kill()
> can safely drop the last tw_refcount reference itself.
> Use refcount_dec_if_one() and only warn if tw_refcount is not 1
> after a grace period.
>
> Fixes: d1e5e6408b30 ("tcp: Introduce optional per-netns ehash.")
> Signed-off-by: Kyle Zeng <kylebot@openai.com>
> [edumazet: only call synchronize_net() if one netns in the batch uses a private ehash,
>   use refcount_dec_if_one() to avoid a false WARN with the shared ehash]
> Signed-off-by: Eric Dumazet <edumazet@kernel.org>
> ---
> Cc: Jiayuan Chen <jiayuan.chen@linux.dev>


Reviewed-by: Jiayuan Chen <jiayuan.chen@linux.dev>


> ---
> v2: [edumazet]
>      Avoid a false WARN_ON_ONCE() in tcp_sk_exit_batch() when a concurrent
>      inet_twsk_kill() still holds a tw_refcount reference, as reported by
>      Jiayuan Chen and Sashiko. Use refcount_dec_if_one() and only warn
>      after a grace period, instead of adding synchronize_net() for the
>      shared ehash, which would be costly on busy hosts.
>
>      When net-next reopens, I plan to free the private hashinfo after
>      an RCU grace period (call_rcu()), so that tcp_sk_exit_batch() no
>      longer needs synchronize_net(). This would also cover request
>      sockets and RCU lookups that could still touch a private ehash.
>
> v1: https://lore.kernel.org/netdev/20261028162510.2180528-1-edumazet@kernel.org/
>
>   net/ipv4/tcp_ipv4.c | 21 ++++++++++++++++++++-
>   1 file changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> index a7f9242f1e429033da2c1284b7c1a1964b2ab171..0dc3b087665dc09ffbdc5c124c4ea2ce7d813c56 100644
> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c
> @@ -3549,6 +3549,7 @@ static int __net_init tcp_sk_init(struct net *net)
>   
>   static void __net_exit tcp_sk_exit_batch(struct list_head *net_exit_list)
>   {
> +	bool synced = false;
>   	struct net *net;
>   
>   	/* make sure concurrent calls to tcp_sk_exit_batch from net_cleanup_work
> @@ -3563,9 +3564,27 @@ static void __net_exit tcp_sk_exit_batch(struct list_head *net_exit_list)
>   
>   	tcp_twsk_purge(net_exit_list);
>   
> +	/* inet_twsk_kill() unhashes a twsk before it is done with ehash
> +	 * and bhash, and before it drops its tw_refcount reference,
> +	 * so tcp_twsk_purge() can miss it. Such callers run with BH
> +	 * disabled: wait for them before freeing a private ehash.
> +	 */
> +	list_for_each_entry(net, net_exit_list, exit_list) {
> +		if (net->ipv4.tcp_death_row.hashinfo->pernet) {
> +			synchronize_net();
> +			synced = true;
> +			break;
> +		}
> +	}
> +
>   	list_for_each_entry(net, net_exit_list, exit_list) {
>   		inet_pernet_hashinfo_free(net->ipv4.tcp_death_row.hashinfo);
> -		WARN_ON_ONCE(!refcount_dec_and_test(&net->ipv4.tcp_death_row.tw_refcount));
> +		/* Without a grace period, a concurrent inet_twsk_kill() might
> +		 * still hold a tw_refcount reference. Let it drop it: struct net
> +		 * is freed by a later cleanup_net(), after an RCU grace period.
> +		 */
> +		if (!refcount_dec_if_one(&net->ipv4.tcp_death_row.tw_refcount))
> +			WARN_ON_ONCE(synced);
>   		tcp_fastopen_ctx_destroy(net);
>   	}
>