March 20, 2025 at 08:06, "Cong Wang" <xiyou.wangcong@xxxxxxxxx> wrote: > > On Wed, Mar 19, 2025 at 11:36:13PM +0000, Jiayuan Chen wrote: > > > > > 2025/3/20 07:02, "Cong Wang" <xiyou.wangcong@xxxxxxxxx> wrote: > > > > > > > > > > > > On Mon, Mar 17, 2025 at 05:22:54PM +0800, Jiayuan Chen wrote: > > > > > > > > > > > > > > The sk->sk_socket is not locked or referenced, and during the call to > > > > > > > > > > > > > Hm? We should have a reference in socket map, whether directly or > > > > > > > > indirectly, right? When we add a socket to a socket map, we do call > > > > > > > > sock_map_psock_get_checked() to obtain a reference. > > > > > > > > > > > > Yes, but we remove psock from sockmap when sock_map_close() was called > > > > ''' > > > > sock_map_close > > > > lock_sock(sk); > > > > rcu_read_lock(); > > > > psock = sk_psock(sk); > > > > // here we remove psock and the reference of psock become 0 > > > > sock_map_remove_links(sk, psock) > > > > sk_psock_drop() also calls cancel_delayed_work_sync(&psock->work), > > althrough in yet another work. Is this also a contribution to this bug? > Maybe it's related. Calling cancel_delayed_work_sync() in sk_psock_drop() is too late for our scenario. To be more precise, the core goal of this patch is to prevent sock_map_close() from executing until the backlog work completes. This is because sock_map_close() resides in the close(fd) path, once it finishes, subsequent steps will release the sk_socket. Therefore, performing cancellation in sk_psock_drop() is too late. Upon reviewing historical commits, I found that the backlog work originally held lock_sk, which naturally synchronized with lock_sk in sock_map_close. However, when the backlog work later removed lock_sk, an alternative synchronization mechanism(just hold psock reference like this patch) became necessary. > > > > psock = sk_psock_get(sk); > > > > if (unlikely(!psock)) > > > > goto no_psock; <=== jmp to no_psock > > > > rcu_read_unlock(); > > > > release_sock(sk); > > > > cancel_delayed_work_sync(&psock->work); <== no chance to run cancel > > > > ''' > > > > I have to say sock_map_close() becomes harder and harder to understand > > now. And I am feeling we may have more bugs since we have two flying > > work's here: psock->rwork and psock->work. > > Thanks. Yes, this patch prevent sock_map_close() from executing until the backlog work completes. This likely makes the cancel_delayed_work in sk_psock_destroy redundant. The code has undergone too many iterations. While sk_psock_destroy certainly contains redundant operations, we should retain it for now. There may be hidden dependencies we haven't fully untangled. Thanks.
