May 15, 2025 at 13:53, "John Fastabend" <john.fastabend@xxxxxxxxx> wrote:
>
> On 2025-05-08 14:18:25, Jiayuan Chen wrote:
>
> >
> > The sk->sk_socket is not locked or referenced in backlog thread, and
> >
> > during the call to skb_send_sock(), there is a race condition with
> >
> > the release of sk_socket. All types of sockets(tcp/udp/unix/vsock)
> >
> > will be affected.
> >
> >
> >
> > Race conditions:
> >
> > '''
> >
> > CPU0 CPU1
> >
> >
> >
> > backlog::skb_send_sock
> >
> > sendmsg_unlocked
> >
> > sock_sendmsg
> >
> > sock_sendmsg_nosec
> >
> > close(fd):
> >
> > ...
> >
> > ops->release() -> sock_map_close()
> >
> > sk_socket->ops = NULL
> >
> > free(socket)
> >
> > sock->ops->sendmsg
> >
> > ^
> >
> > panic here
> >
> > '''
> >
> >
> >
> > The ref of psock become 0 after sock_map_close() executed.
> >
> > '''
> >
> > void sock_map_close()
> >
> > {
> >
> > ...
> >
> > if (likely(psock)) {
> >
> > ...
> >
> > // !! here we remove psock and the ref of psock become 0
> >
> > sock_map_remove_links(sk, psock)
> >
> > psock = sk_psock_get(sk);
> >
> > if (unlikely(!psock))
> >
> > goto no_psock; <=== Control jumps here via goto
> >
> > ...
> >
> > cancel_delayed_work_sync(&psock->work); <=== not executed
> >
> > sk_psock_put(sk, psock);
> >
> > ...
> >
> > }
> >
> > '''
> >
> >
> >
> > Based on the fact that we already wait for the workqueue to finish in
> >
> > sock_map_close() if psock is held, we simply increase the psock
> >
> > reference count to avoid race conditions.
> >
> >
> >
> > With this patch, if the backlog thread is running, sock_map_close() will
> >
> > wait for the backlog thread to complete and cancel all pending work.
> >
> >
> >
> > If no backlog running, any pending work that hasn't started by then will
> >
> > fail when invoked by sk_psock_get(), as the psock reference count have
> >
> > been zeroed, and sk_psock_drop() will cancel all jobs via
> >
> > cancel_delayed_work_sync().
> >
> >
> >
> > In summary, we require synchronization to coordinate the backlog thread
> >
> > and close() thread.
> >
> >
> >
> > The panic I catched:
> >
> > '''
> >
> > Workqueue: events sk_psock_backlog
> >
> > RIP: 0010:sock_sendmsg+0x21d/0x440
> >
> > RAX: 0000000000000000 RBX: ffffc9000521fad8 RCX: 0000000000000001
> >
> > ...
> >
> > Call Trace:
> >
> > <TASK>
> >
> > ? die_addr+0x40/0xa0
> >
> > ? exc_general_protection+0x14c/0x230
> >
> > ? asm_exc_general_protection+0x26/0x30
> >
> > ? sock_sendmsg+0x21d/0x440
> >
> > ? sock_sendmsg+0x3e0/0x440
> >
> > ? __pfx_sock_sendmsg+0x10/0x10
> >
> > __skb_send_sock+0x543/0xb70
> >
> > sk_psock_backlog+0x247/0xb80
> >
> > ...
> >
> > '''
> >
> >
> >
> > Reported-by: Michal Luczaj <mhal@xxxxxxx>
> >
> > Fixes: 799aa7f98d53 ("skmsg: Avoid lock_sock() in sk_psock_backlog()")
> >
> > Signed-off-by: Jiayuan Chen <jiayuan.chen@xxxxxxxxx>
> >
>
> Is the fixes tag actually,
>
> 4b4647add7d3c sock_map: avoid race between sock_map_close and sk_psock_put
>
> Before that we should call the cancel correctly?
>
> Thanks,
>
> John
>
I missed this patch, the fixes should be 4b4647add7d3c.
Yes, before this patch, the workqueue can be canceled correctly.
