[PATCH v2] vsock/hv_sock: don't finish a connection on a socket being released

[PATCH v2] vsock/hv_sock: don't finish a connection on a socket being released

[Bartłomiej Dmitruk]

hvs_open_connection() only checks sk->sk_state (TCP_LISTEN for a
host-initiated connection, TCP_SYN_SENT for a guest-initiated one) before
proceeding. __vsock_release() sets sk->sk_shutdown = SHUTDOWN_MASK and, for
a listener, flushes the accept queue, while leaving sk_state unchanged. A
VMBUS channel offer that races close() therefore still passes the state
check and either enqueues a child onto the already-flushed accept queue
(conn_from_host) or completes a connection on a socket being torn down
(!conn_from_host); the socket and its VMBUS channel then leak.

The virtio transport guards the listener case in
virtio_transport_recv_listen(); hv_sock has no equivalent. Add a
sk_shutdown == SHUTDOWN_MASK check to the common path right after
lock_sock(), so both the host- and guest-initiated paths bail out.
hv_sock holds lock_sock(sk) across hvs_open_connection(), so the check is
race-free.

This was found by code inspection; I do not have Hyper-V hardware to test on.

Fixes: ae0078fcf0a5 ("hv_sock: implements Hyper-V transport for Virtual Sockets (AF_VSOCK)")
Signed-off-by: Bartłomiej Dmitruk <bartlomiej.dmitruk@isec.pl>
---
v2: move the SHUTDOWN_MASK check to the common checks after lock_sock() so it
    also covers the guest-initiated (!conn_from_host) path; add Fixes tag
    (Stefano Garzarella).
v1: https://lore.kernel.org/netdev/20260917220156.56015-1-bartlomiej.dmitruk@isec.pl/

diff --git a/net/vmw_vsock/hyperv_transport.c b/net/vmw_vsock/hyperv_transport.c
--- a/net/vmw_vsock/hyperv_transport.c
+++ b/net/vmw_vsock/hyperv_transport.c
@@ -322,6 +322,15 @@
 	    (!conn_from_host && sk->sk_state != TCP_SYN_SENT))
 		goto out;
 
+	/* __vsock_release() may have already set sk_shutdown = SHUTDOWN_MASK
+	 * (and, for a listener, flushed the accept queue) while leaving the
+	 * socket in TCP_LISTEN/TCP_SYN_SENT.  Proceeding would enqueue a child
+	 * onto a released listener, or complete a connection on a released
+	 * socket, leaking it and its VMBUS channel.
+	 */
+	if (sk->sk_shutdown == SHUTDOWN_MASK)
+		goto out;
+
 	if (conn_from_host) {
 		if (sk_acceptq_is_full(sk))
 			goto out;

Re: [PATCH v2] vsock/hv_sock: don't finish a connection on a socket being released

[Stefano Garzarella]

On Sat, Sep 19, 2026 at 02:31:27PM +0200, Bartłomiej Dmitruk wrote:
>hvs_open_connection() only checks sk->sk_state (TCP_LISTEN for a
>host-initiated connection, TCP_SYN_SENT for a guest-initiated one) before
>proceeding. __vsock_release() sets sk->sk_shutdown = SHUTDOWN_MASK and, for
>a listener, flushes the accept queue, while leaving sk_state unchanged. A
>VMBUS channel offer that races close() therefore still passes the state
>check and either enqueues a child onto the already-flushed accept queue
>(conn_from_host) or completes a connection on a socket being torn down
>(!conn_from_host); the socket and its VMBUS channel then leak.
>
>The virtio transport guards the listener case in
>virtio_transport_recv_listen(); hv_sock has no equivalent. Add a
>sk_shutdown == SHUTDOWN_MASK check to the common path right after
>lock_sock(), so both the host- and guest-initiated paths bail out.
>hv_sock holds lock_sock(sk) across hvs_open_connection(), so the check is
>race-free.
>
>This was found by code inspection; I do not have Hyper-V hardware to test on.
>
>Fixes: ae0078fcf0a5 ("hv_sock: implements Hyper-V transport for Virtual Sockets (AF_VSOCK)")
>Signed-off-by: Bartłomiej Dmitruk <bartlomiej.dmitruk@isec.pl>
>---
>v2: move the SHUTDOWN_MASK check to the common checks after lock_sock() so it
>    also covers the guest-initiated (!conn_from_host) path; add Fixes tag
>    (Stefano Garzarella).
>v1: https://lore.kernel.org/netdev/20260917220156.56015-1-bartlomiej.dmitruk@isec.pl/
>

About v1, I see sashiko reporting some pre-existing issues
https://lore.kernel.org/netdev/179003264046.2160803.3176959842906352107@kernel.org/

The critical one seems real, can you check?
Maybe it should be fixed before this change.

Stefano

>diff --git a/net/vmw_vsock/hyperv_transport.c b/net/vmw_vsock/hyperv_transport.c
>--- a/net/vmw_vsock/hyperv_transport.c
>+++ b/net/vmw_vsock/hyperv_transport.c
>@@ -322,6 +322,15 @@
> 	    (!conn_from_host && sk->sk_state != TCP_SYN_SENT))
> 		goto out;
>
>+	/* __vsock_release() may have already set sk_shutdown = SHUTDOWN_MASK
>+	 * (and, for a listener, flushed the accept queue) while leaving the
>+	 * socket in TCP_LISTEN/TCP_SYN_SENT.  Proceeding would enqueue a child
>+	 * onto a released listener, or complete a connection on a released
>+	 * socket, leaking it and its VMBUS channel.
>+	 */
>+	if (sk->sk_shutdown == SHUTDOWN_MASK)
>+		goto out;
>+
> 	if (conn_from_host) {
> 		if (sk_acceptq_is_full(sk))
> 			goto out;
>

Re: [PATCH v2] vsock/hv_sock: don't finish a connection on a socket being released

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 1 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Critical] The new `goto out` makes the reversed teardown order at the
  `out:` label of hvs_open_connection() (`sock_put(sk)` at…
- [Medium] Commit message understates the impact of the race it fixes:
  for the guest-initiated (!conn_from_host) path the pre-patch race is a…

--

vsock/hv_sock: don't finish a connection on a socket being released

This adds a sk_shutdown == SHUTDOWN_MASK check to hvs_open_connection()
right after lock_sock(), so a VMBUS channel offer that races close() bails
out instead of completing a connection or enqueueing a child on a socket
that __vsock_release() has already torn down.  The check is placed in the
common path so it covers both the conn_from_host and !conn_from_host cases.

Two questions below, one about the commit message and one about the exit
path the new check now jumps to.

> (!conn_from_host && sk->sk_state != TCP_SYN_SENT))
> 		goto out;

Regarding this part of the changelog:

    "or completes a connection on a socket being torn down
    (!conn_from_host); the socket and its VMBUS channel then leak."

[Severity: Medium]
For the guest-initiated path, is a leak really the pre-patch outcome?  In
__vsock_release() sock_orphan() runs immediately before the shutdown mask
is set:

net/vmw_vsock/af_vsock.c:__vsock_release() {
	...
	sock_orphan(sk);
	sk->sk_shutdown = SHUTDOWN_MASK;
	...
}

so sk->sk_socket is already NULL in exactly the state the new check
detects.  For a socket in TCP_SYN_SENT, hvs_release() ->
hvs_close_lock_held() returns early and leaves sk_state alone:

net/vmw_vsock/hyperv_transport.c:hvs_close_lock_held() {
	if (!(sk->sk_state == TCP_ESTABLISHED ||
	      sk->sk_state == TCP_CLOSING))
		return true;
	...
}

which means the pre-patch state check in hvs_open_connection() passes and
the !conn_from_host completion path runs:

net/vmw_vsock/hyperv_transport.c:hvs_open_connection() {
	...
	} else {
		sk->sk_state = TCP_ESTABLISHED;
		sk->sk_socket->state = SS_CONNECTED;
	...
}

Wouldn't that dereference a NULL sk->sk_socket and oops the guest rather
than leak the socket and channel?  Should the changelog describe that as
the impact for the guest-initiated path, since it also changes how urgent
a stable backport looks?

> +	/* __vsock_release() may have already set sk_shutdown = SHUTDOWN_MASK
> +	 * (and, for a listener, flushed the accept queue) while leaving the
> +	 * socket in TCP_LISTEN/TCP_SYN_SENT.  Proceeding would enqueue a child
> +	 * onto a released listener, or complete a connection on a released
> +	 * socket, leaking it and its VMBUS channel.
> +	 */
> +	if (sk->sk_shutdown == SHUTDOWN_MASK)
> +		goto out;

[Severity: Critical]
Can this new goto out free sk and then touch it?  The out: label in
hvs_open_connection() drops the reference before unlocking:

net/vmw_vsock/hyperv_transport.c:hvs_open_connection() {
	...
out:
	/* Release refcnt obtained when we called vsock_find_bound_socket() */
	sock_put(sk);

	release_sock(sk);
}

Counting references for the race the patch targets:

CPU1 (VMBUS offer worker)
hvs_open_connection()
   vsock_find_bound_socket() -> sock_hold()	/* refs: alloc + bound + this */
   lock_sock(sk)				/* blocks */

CPU0 (close)
__vsock_release()
   sock_set_flag(sk, SOCK_DEAD);
   vsk->transport->release() -> hvs_release() -> hvs_close_lock_held()
       -> vsock_remove_sock()			/* bound ref dropped */
   sk->sk_shutdown = SHUTDOWN_MASK;
   while ((pending = vsock_dequeue_accept(sk)) != NULL) { ... }
   release_sock(sk);
   sock_put(sk);				/* only CPU1's ref left */

CPU1 then acquires the lock, the new sk_shutdown == SHUTDOWN_MASK test
fires, and control goes straight to out: without ever reaching the later
sock_hold()/vsock_enqueue_accept() that used to keep sk alive on the
success path.  sock_put() therefore takes the refcount 1 -> 0.  vsock does
not set SOCK_RCU_FREE anywhere under net/vmw_vsock/, so sk_free() ->
__sk_free() -> sk_prot_free() frees the struct sock right there, and the
following release_sock(sk) spin-locks sk->sk_lock.slock, runs the backlog
and wakes sk->sk_lock.wq on freed memory.

The rest of the file already uses the other order, for example:

net/vmw_vsock/hyperv_transport.c:hvs_close_connection() {
	lock_sock(sk);
	hvs_do_close_lock_held(vsock_sk(sk), true);
	release_sock(sk);

	/* Release the refcnt for the channel that's opened in
	 * hvs_open_connection().
	 */
	sock_put(sk);
}

Should the out: label be reordered to release_sock(sk) before
sock_put(sk) as part of this change, given the new check makes this exit
the expected outcome of the race?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260919123128.28868-1-bartlomiej.dmitruk%40isec.pl