Commit 3f74957f authored by Stefano Garzarella's avatar Stefano Garzarella Committed by David S. Miller

vsock: fix potential deadlock in transport->release()

Some transports (hyperv, virtio) acquire the sock lock during the
.release() callback.

In the vsock_stream_connect() we call vsock_assign_transport(); if
the socket was previously assigned to another transport, the
vsk->transport->release() is called, but the sock lock is already
held in the vsock_stream_connect(), causing a deadlock reported by
syzbot:

    INFO: task syz-executor280:9768 blocked for more than 143 seconds.
      Not tainted 5.6.0-rc1-syzkaller #0
    "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
    syz-executor280 D27912  9768   9766 0x00000000
    Call Trace:
     context_switch kernel/sched/core.c:3386 [inline]
     __schedule+0x934/0x1f90 kernel/sched/core.c:4082
     schedule+0xdc/0x2b0 kernel/sched/core.c:4156
     __lock_sock+0x165/0x290 net/core/sock.c:2413
     lock_sock_nested+0xfe/0x120 net/core/sock.c:2938
     virtio_transport_release+0xc4/0xd60 net/vmw_vsock/virtio_transport_common.c:832
     vsock_assign_transport+0xf3/0x3b0 net/vmw_vsock/af_vsock.c:454
     vsock_stream_connect+0x2b3/0xc70 net/vmw_vsock/af_vsock.c:1288
     __sys_connect_file+0x161/0x1c0 net/socket.c:1857
     __sys_connect+0x174/0x1b0 net/socket.c:1874
     __do_sys_connect net/socket.c:1885 [inline]
     __se_sys_connect net/socket.c:1882 [inline]
     __x64_sys_connect+0x73/0xb0 net/socket.c:1882
     do_syscall_64+0xfa/0x790 arch/x86/entry/common.c:294
     entry_SYSCALL_64_after_hwframe+0x49/0xbe

To avoid this issue, this patch remove the lock acquiring in the
.release() callback of hyperv and virtio transports, and it holds
the lock when we call vsk->transport->release() in the vsock core.

Reported-by: syzbot+731710996d79d0d58fbc@syzkaller.appspotmail.com
Fixes: 408624af ("vsock: use local transport when it is loaded")
Signed-off-by: default avatarStefano Garzarella <sgarzare@redhat.com>
Reviewed-by: default avatarStefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
parent 5c05a164
...@@ -451,6 +451,12 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk) ...@@ -451,6 +451,12 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
if (vsk->transport == new_transport) if (vsk->transport == new_transport)
return 0; return 0;
/* transport->release() must be called with sock lock acquired.
* This path can only be taken during vsock_stream_connect(),
* where we have already held the sock lock.
* In the other cases, this function is called on a new socket
* which is not assigned to any transport.
*/
vsk->transport->release(vsk); vsk->transport->release(vsk);
vsock_deassign_transport(vsk); vsock_deassign_transport(vsk);
} }
...@@ -753,20 +759,18 @@ static void __vsock_release(struct sock *sk, int level) ...@@ -753,20 +759,18 @@ static void __vsock_release(struct sock *sk, int level)
vsk = vsock_sk(sk); vsk = vsock_sk(sk);
pending = NULL; /* Compiler warning. */ pending = NULL; /* Compiler warning. */
/* The release call is supposed to use lock_sock_nested()
* rather than lock_sock(), if a sock lock should be acquired.
*/
if (vsk->transport)
vsk->transport->release(vsk);
else if (sk->sk_type == SOCK_STREAM)
vsock_remove_sock(vsk);
/* When "level" is SINGLE_DEPTH_NESTING, use the nested /* When "level" is SINGLE_DEPTH_NESTING, use the nested
* version to avoid the warning "possible recursive locking * version to avoid the warning "possible recursive locking
* detected". When "level" is 0, lock_sock_nested(sk, level) * detected". When "level" is 0, lock_sock_nested(sk, level)
* is the same as lock_sock(sk). * is the same as lock_sock(sk).
*/ */
lock_sock_nested(sk, level); lock_sock_nested(sk, level);
if (vsk->transport)
vsk->transport->release(vsk);
else if (sk->sk_type == SOCK_STREAM)
vsock_remove_sock(vsk);
sock_orphan(sk); sock_orphan(sk);
sk->sk_shutdown = SHUTDOWN_MASK; sk->sk_shutdown = SHUTDOWN_MASK;
......
...@@ -526,12 +526,9 @@ static bool hvs_close_lock_held(struct vsock_sock *vsk) ...@@ -526,12 +526,9 @@ static bool hvs_close_lock_held(struct vsock_sock *vsk)
static void hvs_release(struct vsock_sock *vsk) static void hvs_release(struct vsock_sock *vsk)
{ {
struct sock *sk = sk_vsock(vsk);
bool remove_sock; bool remove_sock;
lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
remove_sock = hvs_close_lock_held(vsk); remove_sock = hvs_close_lock_held(vsk);
release_sock(sk);
if (remove_sock) if (remove_sock)
vsock_remove_sock(vsk); vsock_remove_sock(vsk);
} }
......
...@@ -829,7 +829,6 @@ void virtio_transport_release(struct vsock_sock *vsk) ...@@ -829,7 +829,6 @@ void virtio_transport_release(struct vsock_sock *vsk)
struct sock *sk = &vsk->sk; struct sock *sk = &vsk->sk;
bool remove_sock = true; bool remove_sock = true;
lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
if (sk->sk_type == SOCK_STREAM) if (sk->sk_type == SOCK_STREAM)
remove_sock = virtio_transport_close(vsk); remove_sock = virtio_transport_close(vsk);
...@@ -837,7 +836,6 @@ void virtio_transport_release(struct vsock_sock *vsk) ...@@ -837,7 +836,6 @@ void virtio_transport_release(struct vsock_sock *vsk)
list_del(&pkt->list); list_del(&pkt->list);
virtio_transport_free_pkt(pkt); virtio_transport_free_pkt(pkt);
} }
release_sock(sk);
if (remove_sock) if (remove_sock)
vsock_remove_sock(vsk); vsock_remove_sock(vsk);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment