Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] vsock: unprotected socket state modification in vsock_g2h_reset_unreachable() --
commit 22f9613466fe3520fb04ad3d32779638f9afe30e Author: Bobby Eshleman <[email protected]> vsock: add a netlink command to assign the g2h device to a netns This commit introduces the VSOCK_CMD_DEV_NETNS_SET netlink command to move the guest-to-host vsock device to a different network namespace. It resets established connections that are no longer able to reach the device from their respective namespaces. The commit also updates CID discovery and socket creation paths to honour the new namespace assignments. 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 95a435aef512b..9938dd5010192 100644 > --- a/net/vmw_vsock/af_vsock.c > +++ b/net/vmw_vsock/af_vsock.c [ ... ] > @@ -742,6 +812,173 @@ bool vsock_find_cid(unsigned int cid) [ ... ] > +/* Reset every connected socket of @t that can no longer reach the g2h > device, > + * and let the transport tell each peer. > + */ > +static void vsock_g2h_reset_unreachable(const struct vsock_transport *t) > +{ > + struct vsock_sock *vsk, *tmp; > + LIST_HEAD(reset_list); > + struct sock *sk; > + int i; > + > + /* The calling context must hold vsock_register_mutex, which serializes > + * concurrent netns assignments' use of vsk->pending_reset. > + */ > + lockdep_assert_held(&vsock_register_mutex); > + > + spin_lock_bh(&vsock_table_lock); > + > + for (i = 0; i < ARRAY_SIZE(vsock_connected_table); i++) { > + list_for_each_entry(vsk, &vsock_connected_table[i], > + connected_table) { > + sk = sk_vsock(vsk); > + > + if (vsk->transport != t || > + sk->sk_state == TCP_CLOSE || > + vsock_g2h_reachable_sk(vsk)) > + continue; > + > + sk->sk_state = TCP_CLOSE; > + sk->sk_err = ECONNRESET; > + sk_error_report(sk); [Severity: High] Does this code modify the socket state locklessly? sk->sk_state and sk->sk_err are modified here while holding only the vsock_table_lock spinlock, without acquiring the socket lock (lock_sock). Core socket operations, such as vsock_connectible_sendmsg(), rely on lock_sock() to protect state transitions and evaluate sk->sk_state to determine if data can be enqueued. Modifying the state here without the socket lock introduces a race condition. For example, a thread holding lock_sock() might read an established state and proceed to enqueue data, while this function concurrently sets the state to TCP_CLOSE. Should the state modification be deferred to the second loop where lock_sock() can be acquired? > + > + sock_hold(sk); > + list_add_tail(&vsk->pending_reset, &reset_list); > + } > + } > + > + spin_unlock_bh(&vsock_table_lock); > + > + /* Reset outside of spinlock because the transport may sleep > + * (e.g., GFP_KERNEL alloc). > + */ > + list_for_each_entry_safe(vsk, tmp, &reset_list, pending_reset) { > + list_del_init(&vsk->pending_reset); > + t->reset(vsk, NULL); > + sock_put(sk_vsock(vsk)); > + } > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
