Message ID | 20200121123147.706666-1-jakub@cloudflare.com |
---|---|
State | Accepted |
Delegated to: | David Miller |
Headers | show |
Series | [net] net, sk_msg: Don't check if sock is locked when tearing down psock | expand |
Jakub Sitnicki wrote: > As John Fastabend reports [0], psock state tear-down can happen on receive > path *after* unlocking the socket, if the only other psock user, that is > sockmap or sockhash, releases its psock reference before tcp_bpf_recvmsg > does so: > > tcp_bpf_recvmsg() > psock = sk_psock_get(sk) <- refcnt 2 > lock_sock(sk); > ... > sock_map_free() <- refcnt 1 > release_sock(sk) > sk_psock_put() <- refcnt 0 > > Remove the lockdep check for socket lock in psock tear-down that got > introduced in 7e81a3530206 ("bpf: Sockmap, ensure sock lock held during > tear down"). > > [0] https://lore.kernel.org/netdev/5e25dc995d7d_74082aaee6e465b441@john-XPS-13-9370.notmuch/ > > Fixes: 7e81a3530206 ("bpf: Sockmap, ensure sock lock held during tear down") > Reported-by: syzbot+d73682fcf7fee6982fe3@syzkaller.appspotmail.com > Suggested-by: John Fastabend <john.fastabend@gmail.com> > Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com> > --- > net/core/skmsg.c | 2 -- > 1 file changed, 2 deletions(-) Thanks Jakub, this was not needed I got a bit carried away. I'll add a selftest to catch this case by duplicating the reproducer into test_sockmap.c Acked-by: John Fastabend <john.fastabend@gmail.com>
On 1/21/20 1:31 PM, Jakub Sitnicki wrote: > As John Fastabend reports [0], psock state tear-down can happen on receive > path *after* unlocking the socket, if the only other psock user, that is > sockmap or sockhash, releases its psock reference before tcp_bpf_recvmsg > does so: > > tcp_bpf_recvmsg() > psock = sk_psock_get(sk) <- refcnt 2 > lock_sock(sk); > ... > sock_map_free() <- refcnt 1 > release_sock(sk) > sk_psock_put() <- refcnt 0 > > Remove the lockdep check for socket lock in psock tear-down that got > introduced in 7e81a3530206 ("bpf: Sockmap, ensure sock lock held during > tear down"). > > [0] https://lore.kernel.org/netdev/5e25dc995d7d_74082aaee6e465b441@john-XPS-13-9370.notmuch/ > > Fixes: 7e81a3530206 ("bpf: Sockmap, ensure sock lock held during tear down") > Reported-by: syzbot+d73682fcf7fee6982fe3@syzkaller.appspotmail.com > Suggested-by: John Fastabend <john.fastabend@gmail.com> > Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com> Given it's assigned to you right now, David, feel free to take directly to net: Acked-by: Daniel Borkmann <daniel@iogearbox.net> Thanks, Daniel
From: Jakub Sitnicki <jakub@cloudflare.com> Date: Tue, 21 Jan 2020 13:31:47 +0100 > As John Fastabend reports [0], psock state tear-down can happen on receive > path *after* unlocking the socket, if the only other psock user, that is > sockmap or sockhash, releases its psock reference before tcp_bpf_recvmsg > does so: > > tcp_bpf_recvmsg() > psock = sk_psock_get(sk) <- refcnt 2 > lock_sock(sk); > ... > sock_map_free() <- refcnt 1 > release_sock(sk) > sk_psock_put() <- refcnt 0 > > Remove the lockdep check for socket lock in psock tear-down that got > introduced in 7e81a3530206 ("bpf: Sockmap, ensure sock lock held during > tear down"). > > [0] https://lore.kernel.org/netdev/5e25dc995d7d_74082aaee6e465b441@john-XPS-13-9370.notmuch/ > > Fixes: 7e81a3530206 ("bpf: Sockmap, ensure sock lock held during tear down") > Reported-by: syzbot+d73682fcf7fee6982fe3@syzkaller.appspotmail.com > Suggested-by: John Fastabend <john.fastabend@gmail.com> > Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com> Applied, thank you.
diff --git a/net/core/skmsg.c b/net/core/skmsg.c index 3866d7e20c07..ded2d5227678 100644 --- a/net/core/skmsg.c +++ b/net/core/skmsg.c @@ -594,8 +594,6 @@ EXPORT_SYMBOL_GPL(sk_psock_destroy); void sk_psock_drop(struct sock *sk, struct sk_psock *psock) { - sock_owned_by_me(sk); - sk_psock_cork_free(psock); sk_psock_zap_ingress(psock);
As John Fastabend reports [0], psock state tear-down can happen on receive path *after* unlocking the socket, if the only other psock user, that is sockmap or sockhash, releases its psock reference before tcp_bpf_recvmsg does so: tcp_bpf_recvmsg() psock = sk_psock_get(sk) <- refcnt 2 lock_sock(sk); ... sock_map_free() <- refcnt 1 release_sock(sk) sk_psock_put() <- refcnt 0 Remove the lockdep check for socket lock in psock tear-down that got introduced in 7e81a3530206 ("bpf: Sockmap, ensure sock lock held during tear down"). [0] https://lore.kernel.org/netdev/5e25dc995d7d_74082aaee6e465b441@john-XPS-13-9370.notmuch/ Fixes: 7e81a3530206 ("bpf: Sockmap, ensure sock lock held during tear down") Reported-by: syzbot+d73682fcf7fee6982fe3@syzkaller.appspotmail.com Suggested-by: John Fastabend <john.fastabend@gmail.com> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com> --- net/core/skmsg.c | 2 -- 1 file changed, 2 deletions(-)