Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:
Critical: 0 · High: 1 · Medium: 2 · Low: 0
- [High] Lost-update race: the new lockless store `vsk->peer_shutdown =
SHUTDOWN_MASK;` in virtio_vsock_reset_sock()…
- [Medium] Unannotated (plain) store to a locklessly-read field:
virtio_vsock_reset_sock() (net/vmw_vsock/virtio_transport.c:423)…
- [Medium] Commit message does not describe the bug being fixed nor the
user-visible behaviour change, while carrying a Fixes: tag.
--
vsock/virtio: Streamline socket reset on transport/PM event
This makes virtio_vsock_reset_sock() set vsk->peer_shutdown to
SHUTDOWN_MASK in addition to setting TCP_CLOSE and ECONNRESET, matching
what vhost_vsock_reset_orphans() and vmci_transport_handle_detach() do.
The changelog describes it as groundwork for later patches in the series.
A few questions below, one about the changelog and two about the new
store itself.
> vsock/virtio: Streamline socket reset on transport/PM event
>
> Follow vhost's vhost_vsock_reset_orphans() and VMCI's
> vmci_transport_handle_detach(): set SHUTDOWN_MASK, which will come handy
> later in the series.
>
> Fixes: 0ea9e1d3a9e3 ("VSOCK: Introduce virtio_transport.ko")
[Severity: Medium]
The changelog says the new state "will come handy later in the series",
but the patch carries a Fixes: tag against the original transport commit.
Could the changelog state the failure mode that is being fixed, so this
can be judged (and backported) on its own?
The change also looks user-visible by itself. After
VIRTIO_VSOCK_EVENT_TRANSPORT_RESET or device removal/PM freeze,
vsock_poll_shutdown() now contributes EPOLLRDHUP:
if (sk->sk_shutdown & RCV_SHUTDOWN ||
peer_shutdown & SEND_SHUTDOWN)
mask |= EPOLLRDHUP;
and vsock_poll() adds EPOLLIN|EPOLLRDNORM, while the blocking send/recv
loops now terminate on the peer_shutdown condition rather than only on
sk_err.
The errno also changes for send(). In vsock_connectible_sendmsg() the
peer shutdown test runs before the state test:
if (sk->sk_shutdown & SEND_SHUTDOWN ||
vsk->peer_shutdown & RCV_SHUTDOWN) {
err = -EPIPE;
goto out;
}
if (!transport || sk->sk_state != TCP_ESTABLISHED ||
For SOCK_STREAM the error is then passed through sk_stream_error(), which
consumes the pending ECONNRESET on the first call and raises SIGPIPE on
subsequent calls unless MSG_NOSIGNAL is set:
net/core/stream.c:sk_stream_error() {
if (err == -EPIPE)
err = sock_error(sk) ? : -EPIPE;
if (err == -EPIPE && !(flags & MSG_NOSIGNAL))
send_sig(SIGPIPE, current, 0);
}
Before this patch the same send() returned -ENOTCONN and no signal. Is
that behaviour change intended, and could it be mentioned in the
changelog?
> diff --git a/net/vmw_vsock/virtio_transport.c
> b/net/vmw_vsock/virtio_transport.c
> index 4f9aa9c4c3aa5..4d6991321699e 100644
> --- a/net/vmw_vsock/virtio_transport.c
> +++ b/net/vmw_vsock/virtio_transport.c
> @@ -413,11 +413,14 @@ static void virtio_vsock_event_fill(struct virtio_vsock
> *vsock)
>
> static void virtio_vsock_reset_sock(struct sock *sk)
> {
> + struct vsock_sock *vsk = vsock_sk(sk);
> +
> /* vmci_transport.c doesn't take sk_lock here either. At least we're
> * under vsock_table_lock so the sock cannot disappear while we're
> * executing.
> */
>
> + vsk->peer_shutdown = SHUTDOWN_MASK;
[Severity: Medium]
Should this store be a WRITE_ONCE()? As the comment right above it says,
no socket lock is held here, only vsock_table_lock via
vsock_for_each_connected_socket(). peer_shutdown is sampled locklessly
by vsock_poll():
net/vmw_vsock/af_vsock.c:vsock_poll() {
u32 peer_shutdown = READ_ONCE(vsk->peer_shutdown);
/* DGRAM sockets do not take lock_sock() in poll(), so use one
* lockless snapshot for all shutdown-derived mask bits.
*/
...
peer_shutdown = READ_ONCE(vsk->peer_shutdown);
mask |= vsock_poll_shutdown(sk, peer_shutdown);
}
and the equivalent assignment in the same transport is annotated:
net/vmw_vsock/virtio_transport_common.c:virtio_transport_do_close() {
WRITE_ONCE(vsk->peer_shutdown, SHUTDOWN_MASK);
}
The other writers in af_vsock.c, hyperv_transport.c and vmci_transport.c
use WRITE_ONCE() too, so this looks like an unmarked/marked pair that
KCSAN would flag. The last patch of the series also reads this field
with READ_ONCE() from vsock_connect() with the comment "Virtio/PM events
are serviced locklessly.", which makes the pairing here relevant.
[Severity: High]
Separately from the annotation: can this store be lost against the
read-modify-write in the rx path?
virtio_transport_recv_connected() handles OP_SHUTDOWN under lock_sock():
net/vmw_vsock/virtio_transport_common.c:virtio_transport_recv_connected() {
case VIRTIO_VSOCK_OP_SHUTDOWN: {
u32 peer_shutdown = READ_ONCE(vsk->peer_shutdown);
if (le32_to_cpu(hdr->flags) & VIRTIO_VSOCK_SHUTDOWN_RCV)
peer_shutdown |= RCV_SHUTDOWN;
if (le32_to_cpu(hdr->flags) & VIRTIO_VSOCK_SHUTDOWN_SEND)
peer_shutdown |= SEND_SHUTDOWN;
WRITE_ONCE(vsk->peer_shutdown, peer_shutdown);
}
while the reset path holds only vsock_table_lock:
net/vmw_vsock/af_vsock.c:vsock_for_each_connected_socket() {
spin_lock_bh(&vsock_table_lock);
...
fn(sk_vsock(vsk));
}
So the two lock sets are disjoint:
CPU0 (rx_work, lock_sock held)
virtio_transport_recv_connected()
peer_shutdown = READ_ONCE(vsk->peer_shutdown); /* reads 0 */
CPU1 (event_work or device removal)
virtio_vsock_reset_sock()
vsk->peer_shutdown = SHUTDOWN_MASK;
CPU0 continues
WRITE_ONCE(vsk->peer_shutdown, peer_shutdown); /* stale value wins */
Are these two contexts actually serialized anywhere? virtio_vsock_vqs_del()
runs the reset loop before rx_run is cleared and rx_work drained:
net/vmw_vsock/virtio_transport.c:virtio_vsock_vqs_del() {
/* Reset all connected sockets when the VQs disappear */
vsock_for_each_connected_socket(&virtio_transport.transport,
virtio_vsock_reset_sock);
/* Stop all work handlers ... */
mutex_lock(&vsock->rx_lock);
vsock->rx_run = false;
}
and the VIRTIO_VSOCK_EVENT_TRANSPORT_RESET path runs from event_work,
which shares the non-ordered virtio_vsock_workqueue with rx_work.
Since hdr->flags is host supplied and is not validated as non-zero, the
value written back by the rx path can be the stale snapshot itself, i.e.
0, which would clear the reset state entirely. In that case
vsock_poll_shutdown() reports no EPOLLRDHUP/EPOLLHUP, the sendmsg/recvmsg
peer_shutdown tests see a stale mask, and the guard added by the last
patch of the series:
net/vmw_vsock/af_vsock.c:vsock_connect() {
/* Virtio/PM events are serviced locklessly. */
if (READ_ONCE(vsk->peer_shutdown)) {
err = -ECONNRESET;
}
silently passes. A WRITE_ONCE() alone would not close this; would an
atomic update (atomic OR / cmpxchg), or performing the reset from a
context that can take lock_sock(), be more appropriate here?
> sk->sk_state = TCP_CLOSE;
> sk->sk_err = ECONNRESET;
> sk_error_report(sk);
Both of the above were still present in the same form at the end of the
series, so no later patch appears to change them.
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915-vsock-connect-reset-closing-v2-0-a1d9abb472f7%40rbox.co
