Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] vsock: incomplete socket state reset in vsock_g2h_reset_unreachable() --
commit fe6dcf2ff7ad3f974f6de58396eb4f397d8574af Author: Bobby Eshleman <[email protected]> vsock: add a netlink command to assign the g2h device to a netns This commit introduces a vsock generic netlink family with VSOCK_CMD_DEV_NETNS_SET and VSOCK_CMD_DEV_NETNS_GET to assign the guest-to-host vsock device to a network namespace and query its current assignment. When the device moves to a namespace that existing connected sockets cannot reach, those sockets are swept and closed with ECONNRESET. Link: https://lore.kernel.org/all/20200427142518.uwssa6dtasrp3bfc@steredhat/ > diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c > index c9b28e34b1b3c..e16e72471b4a3 100644 > --- a/net/vmw_vsock/af_vsock.c > +++ b/net/vmw_vsock/af_vsock.c [ ... ] > @@ -742,8 +823,303 @@ bool vsock_find_cid(unsigned int cid) [ ... ] > +/* Close every connected socket of @t that can no longer reach the g2h > device. > + */ > +static void vsock_g2h_reset_unreachable(const struct vsock_transport *t) > +{ > + struct vsock_sock *vsk; > + unsigned int i = 0; > + struct sock *sk; > + bool reset; > + > + lockdep_assert_held(&vsock_g2h_assign_mutex); > + lockdep_assert_not_held(&vsock_register_mutex); > + > + while ((vsk = vsock_g2h_next_unreachable(t, &i))) { > + sk = sk_vsock(vsk); > + > + lock_sock(sk); > + > + reset = false; > + spin_lock_bh(&vsock_table_lock); > + if (__vsock_in_connected_table(vsk) && > + vsock_g2h_should_reset(vsk, t)) { > + __vsock_remove_connected(vsk); > + reset = true; > + } > + spin_unlock_bh(&vsock_table_lock); > + > + if (reset) { > + sk->sk_state = TCP_CLOSE; > + sk->sk_err = ECONNRESET; > + sk_error_report(sk); > + } > + > + release_sock(sk); > + sock_put(sk); > + } > +} [Severity: Medium] When vsock_g2h_reset_unreachable() resets an established socket, it sets sk->sk_state = TCP_CLOSE and sk->sk_err = ECONNRESET, but leaves SOCK_DONE unset, vsk->peer_shutdown unchanged, and sk->sk_socket->state as SS_CONNECTED. On a subsequent read, __vsock_connectible_recvmsg() checks sk->sk_state before reaching the stream or seqpacket receive paths that check sock_error(sk): net/vmw_vsock/af_vsock.c:__vsock_connectible_recvmsg() if (!transport || sk->sk_state != TCP_ESTABLISHED) { if (sock_flag(sk, SOCK_DONE)) err = 0; else err = -ENOTCONN; goto out; } And on a subsequent write, vsock_connectible_sendmsg() skips the -EPIPE branch because vsk->peer_shutdown does not have RCV_SHUTDOWN set, and instead takes the -ENOTCONN branch: net/vmw_vsock/af_vsock.c:vsock_connectible_sendmsg() if (sk->sk_shutdown & SEND_SHUTDOWN || vsk->peer_shutdown & RCV_SHUTDOWN) { err = -EPIPE; goto out; } if (!transport || sk->sk_state != TCP_ESTABLISHED || !vsock_addr_bound(&vsk->local_addr)) { err = -ENOTCONN; goto out; } ... out: if (sk->sk_type == SOCK_STREAM) err = sk_stream_error(sk, msg->msg_flags, err); Because sk_stream_error() only replaces err with sock_error(sk) when err == -EPIPE (and is not called for SOCK_SEQPACKET), both recvmsg() and sendmsg() return -ENOTCONN instead of -ECONNRESET without clearing sk->sk_err, leaving vsock_poll() permanently reporting EPOLLERR | EPOLLOUT. In addition, because sk->sk_socket->state remains SS_CONNECTED, a retried vsock_connect() fails with -EISCONN: net/vmw_vsock/af_vsock.c:vsock_connect() switch (sock->state) { case SS_CONNECTED: err = -EISCONN; goto out; While vsock_shutdown() sees sock->state != SS_UNCONNECTED and still calls vsock_send_shutdown() on the disallowed transport: net/vmw_vsock/af_vsock.c:vsock_shutdown() if (sock->state == SS_UNCONNECTED) { err = -ENOTCONN; if (sock_type_connectible(sk->sk_type)) goto out; } else { sock->state = SS_DISCONNECTING; err = 0; } /* Receive and send shutdowns are treated alike. */ mode = mode & (RCV_SHUTDOWN | SEND_SHUTDOWN); if (mode) { sk->sk_shutdown |= mode; sk->sk_state_change(sk); if (sock_type_connectible(sk->sk_type)) { sock_reset_flag(sk, SOCK_DONE); vsock_send_shutdown(sk, mode); } } Should vsock_g2h_reset_unreachable() also update vsk->peer_shutdown, SOCK_DONE, and sk->sk_socket->state (when sk->sk_socket is present), or ensure recvmsg() and sendmsg() consume sk->sk_err so callers observe ECONNRESET and can reconnect as intended? [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
