[PATCH bpf-next] bpf: sockmap calling sleepable function in teardown path

[PATCH bpf-next] bpf: sockmap calling sleepable function in teardown path

[John Fastabend]

syzbot reproduced the BUG,

 BUG: sleeping function called from invalid context at kernel/workqueue.c:3010

with the following stack trace fragment

 start_flush_work kernel/workqueue.c:3010 [inline]
 __flush_work+0x109/0xb10 kernel/workqueue.c:3074
 __cancel_work_timer+0x3f9/0x570 kernel/workqueue.c:3162
 sk_psock_stop+0x4cb/0x630 net/core/skmsg.c:802
 sock_map_destroy+0x333/0x760 net/core/sock_map.c:1581
 inet_csk_destroy_sock+0x196/0x440 net/ipv4/inet_connection_sock.c:1130
 __tcp_close+0xd5b/0x12b0 net/ipv4/tcp.c:2897
 tcp_close+0x29/0xc0 net/ipv4/tcp.c:2909

introduced by d8616ee2affc. Do a quick trace of the code path and the
bug is obvious,

   inet_csk_destroy_sock(sk)
     sk_prot->destroy(sk);      <--- sock_map_destroy
        sk_psock_stop(, true);   <--- true so cancel workqueue
          cancel_work_sync()     <--- splat, because *_bh_disable()

We can not call cancel_work_sync() from inside destroy path. So mark the
sk_psock_stop call to skip this cancel_work_sync(). This will avoid
the BUG, but means we may run sk_psock_backlog after or during the
destroy op. We zapped the ingress_skb queue in sk_psock_stop (safe to
do with local_bh_disable) so its empty and the sk_psock_backlog work item
will not find any pkts to process here. However, because we are
not going to wait for it or clear its ->state its possible it
kicks off or is already running. This should be 'safe' up until
pssock drops its refcnt to psock->sk. The sock_put() that drops this
reference is only done at psock destroy time from sk_psock_destroy().
This is done through workqueue wihen sk_psock_drop() is called on
psock refnt reaches 0. And importantly sk_psock_destroy() does a
cancel_work_sync(). So trivial fix works.

I've had hit or miss luck reproducing this caught it once or twice with
the provided reproducer when running with many runners. However, syzkaller
is very good at reproducing so relying on syzkaller to verify fix.

Suggested-by: Hillf Danton <hdanton@sina.com>
Reported-by: Reported-by: syzbot+140186ceba0c496183bc@syzkaller.appspotmail.com
Fixes: d8616ee2affc ("bpf, sockmap: Fix sk->sk_forward_alloc warn_on in sk_stream_kill_queues")
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
 net/core/sock_map.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/sock_map.c b/net/core/sock_map.c
index 9f08ccfaf6da..028813dfecb0 100644
--- a/net/core/sock_map.c
+++ b/net/core/sock_map.c
@@ -1578,7 +1578,7 @@ void sock_map_destroy(struct sock *sk)
 	saved_destroy = psock->saved_destroy;
 	sock_map_remove_links(sk, psock);
 	rcu_read_unlock();
-	sk_psock_stop(psock, true);
+	sk_psock_stop(psock, false);
 	sk_psock_put(sk, psock);
 	saved_destroy(sk);
 }
-- 
2.33.0

Re: [PATCH bpf-next] bpf: sockmap calling sleepable function in teardown path

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to bpf/bpf-next.git (master)
by Daniel Borkmann <daniel@iogearbox.net>:

On Mon, 27 Jun 2022 20:58:03 -0700 you wrote:
> syzbot reproduced the BUG,
> 
>  BUG: sleeping function called from invalid context at kernel/workqueue.c:3010
> 
> with the following stack trace fragment
> 
>  start_flush_work kernel/workqueue.c:3010 [inline]
>  __flush_work+0x109/0xb10 kernel/workqueue.c:3074
>  __cancel_work_timer+0x3f9/0x570 kernel/workqueue.c:3162
>  sk_psock_stop+0x4cb/0x630 net/core/skmsg.c:802
>  sock_map_destroy+0x333/0x760 net/core/sock_map.c:1581
>  inet_csk_destroy_sock+0x196/0x440 net/ipv4/inet_connection_sock.c:1130
>  __tcp_close+0xd5b/0x12b0 net/ipv4/tcp.c:2897
>  tcp_close+0x29/0xc0 net/ipv4/tcp.c:2909
> 
> [...]

Here is the summary with links:
  - [bpf-next] bpf: sockmap calling sleepable function in teardown path
    https://git.kernel.org/bpf/bpf-next/c/697fb80a5364

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html