netdev - Re: [PATCH bpf-next v5] bpf, sockmap: avoid using sk

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20250515055356.bgevcqwkyv3q7acr@gmail.com>
Date: Wed, 14 May 2025 22:53:56 -0700
From: John Fastabend <john.fastabend@...il.com>
To: Jiayuan Chen <jiayuan.chen@...ux.dev>
Cc: bpf@...r.kernel.org, Michal Luczaj <mhal@...x.co>,
	Jakub Sitnicki <jakub@...udflare.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Simon Horman <horms@...nel.org>,
	Alexei Starovoitov <ast@...nel.org>,
	Cong Wang <cong.wang@...edance.com>, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH bpf-next v5] bpf, sockmap: avoid using sk_socket after
 free when sending

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@...x.co>
> Fixes: 799aa7f98d53 ("skmsg: Avoid lock_sock() in sk_psock_backlog()")
> Signed-off-by: Jiayuan Chen <jiayuan.chen@...ux.dev>

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