Re: [PATCH] vsock/virtio: fix memory leak in virtio_transport_recv_listen()

[Stefano Garzarella <sgarzare@redhat.com>]

On Fri, Apr 24, 2026 at 08:33:10PM +0530, Deepanshu Kartikey wrote:
>Two bugs exist in virtio_transport_recv_listen():

Two bugs, two fixes, two patches usually.

>
>1. On the transport assignment error path, sk_acceptq_added() is called
>   but sk_acceptq_removed() is never called when vsock_assign_transport()
>   fails or assigns a different transport than expected. This causes the
>   parent listener's accept backlog counter to be permanently inflated,
>   eventually causing sk_acceptq_is_full() to reject legitimate incoming
>   connections.

Wait, I can't see this issue. sk_acceptq_added() is called after the
vsock_assign_transport(), so why we should call sk_acceptq_removed()
in the error path of vsock_assign_transport()?

Maybe I'm missing something.

>
>2. There is a race between __vsock_release() and vsock_enqueue_accept().
>   __vsock_release() sets sk->sk_shutdown to SHUTDOWN_MASK and flushes
>   the accept queue under the parent socket lock. However,
>   virtio_transport_recv_listen() checks sk_shutdown and subsequently
>   calls vsock_enqueue_accept() without holding the parent socket lock.

Are you sure about this?

virtio_transport_recv_listen() is called only by 
virtio_transport_recv_pkt() after calling lock_sock(sk), so I'm really 
confused.

>   This means a child socket can be enqueued after __vsock_release() has
>   already flushed the queue, causing the child socket and its associated
>   resources to leak
>   permanently. The existing comment in the code hints at this race but
>   the fix was never implemented.

Are you referring to:
	/* __vsock_release() might have already flushed accept_queue.
	 * Subsequent enqueues would lead to a memory leak.
	 */
	if (sk->sk_shutdown == SHUTDOWN_MASK) {
		virtio_transport_reset_no_sock(t, skb, sock_net(sk));
		return -ESHUTDOWN;
	}

In this case I think we are saying that we are doing this check exactly 
to avoid that issue.

>
>Fix both issues: add sk_acceptq_removed() on the transport error path,

Again, better to fix the 2 issues with 2 patches (same series is fine).

>and re-check sk->sk_shutdown under the parent socket lock before calling
>vsock_enqueue_accept() to close the race window. The child socket lock
>is released before acquiring the parent socket lock to maintain correct
>lock ordering (parent before child).
>

We are missing the Fixes tag, and I think we can target the `net` tree
with this patch (i.e. [PATCH net]), see:
https://www.kernel.org/doc/html/next/process/maintainer-netdev.html

>Reported-by: syzbot+1b2c9c4a0f8708082678@syzkaller.appspotmail.com
>Closes: https://syzkaller.appspot.com/bug?extid=1b2c9c4a0f8708082678
>Tested-by: syzbot+1b2c9c4a0f8708082678@syzkaller.appspotmail.com
>Signed-off-by: Deepanshu Kartikey <kartikey406@gmail.com>
>---
> net/vmw_vsock/virtio_transport_common.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
>diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>index 416d533f493d..fad5fa4a4296 100644
>--- a/net/vmw_vsock/virtio_transport_common.c
>+++ b/net/vmw_vsock/virtio_transport_common.c
>@@ -1578,6 +1578,7 @@ virtio_transport_recv_listen(struct sock *sk, struct sk_buff *skb,
> 	 */
> 	if (ret || vchild->transport != &t->transport) {
> 		release_sock(child);
>+		sk_acceptq_removed(sk);

Ditto, are we sure about this?

> 		virtio_transport_reset_no_sock(t, skb, sock_net(sk));
> 		sock_put(child);
> 		return ret;
>@@ -1588,11 +1589,19 @@ virtio_transport_recv_listen(struct sock *sk, struct sk_buff *skb,
> 		child->sk_write_space(child);
>
> 	vsock_insert_connected(vchild);
>+	release_sock(child);
>+	lock_sock(sk);

IMO this is a deadlock with the lock_sock(sk) called by the caller.

Also a comment would be helpful here to explain why we're doing this.

>+	if (sk->sk_shutdown == SHUTDOWN_MASK) {
>+		release_sock(sk);
>+		sk_acceptq_removed(sk);
>+		virtio_transport_reset_no_sock(t, skb, sock_net(sk));
>+		sock_put(child);
>+		return -ESHUTDOWN;

Since this is very similar to the error path of 
vsock_assign_transport(), I think it would be better to start by 
defining a common error path for the function and use labels to exit, so 
we can avoid duplicating the code multiple times.

>+	}
> 	vsock_enqueue_accept(sk, child);
>+	release_sock(sk);
> 	virtio_transport_send_response(vchild, skb);
>
>-	release_sock(child);
>-

TBH I'm really worried about this patch since both fixes are completely 
wrong IMO.

Thanks,
Stefano

> 	sk->sk_data_ready(sk);
> 	return 0;
> }
>-- 
>2.43.0
>