Re: [PATCH net] tcp: properly terminate timers for kernel sockets

[Josef Bacik <josef@toxicpanda.com>]

On Fri, Mar 22, 2024 at 01:57:32PM +0000, Eric Dumazet wrote:
> We had various syzbot reports about tcp timers firing after
> the corresponding netns has been dismantled.
> 
> Fortunately Josef Bacik could trigger the issue more often,
> and could test a patch I wrote two years ago.
> 
> When TCP sockets are closed, we call inet_csk_clear_xmit_timers()
> to 'stop' the timers.
> 
> inet_csk_clear_xmit_timers() can be called from any context,
> including when socket lock is held.
> This is the reason it uses sk_stop_timer(), aka del_timer().
> This means that ongoing timers might finish much later.
> 
> For user sockets, this is fine because each running timer
> holds a reference on the socket, and the user socket holds
> a reference on the netns.
> 
> For kernel sockets, we risk that the netns is freed before
> timer can complete, because kernel sockets do not hold
> reference on the netns.
> 
> This patch adds inet_csk_clear_xmit_timers_sync() function
> that using sk_stop_timer_sync() to make sure all timers
> are terminated before the kernel socket is released.
> Modules using kernel sockets close them in their netns exit()
> handler.
> 
> Also add sock_not_owned_by_me() helper to get LOCKDEP
> support : inet_csk_clear_xmit_timers_sync() must not be called
> while socket lock is held.
> 
> It is very possible we can revert in the future commit
> 3a58f13a881e ("net: rds: acquire refcount on TCP sockets")
> which attempted to solve the issue in rds only.
> (net/smc/af_smc.c and net/mptcp/subflow.c have similar code)
> 
> We probably can remove the check_net() tests from
> tcp_out_of_resources() and __tcp_close() in the future.
> 

Thanks so much for your help with this Eric!

Josef