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
