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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 22 Mar 2024 10:58:56 -0400
From: Josef Bacik <josef@...icpanda.com>
To: Eric Dumazet <edumazet@...gle.com>
Cc: "David S . Miller" <davem@...emloft.net>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	netdev@...r.kernel.org, eric.dumazet@...il.com,
	Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
Subject: Re: [PATCH net] tcp: properly terminate timers for kernel sockets

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