On Thu, Oct 29, 2026 at 10:39:50PM +0100, Michal Luczaj wrote:
> On 10/22/26 12:47, Stefano Garzarella wrote:
> >>>> +/* Move @vsk to TCP_ESTABLISHED and into the connected table, unless
> >>>> the device
> >>>> + * has moved to a namespace @vsk cannot reach. Returns false without
> >>>> doing
> >>>> + * either in that case.
> >>>> + *
> >>>> + * The reset sweep walks the same table under the same lock, so an
> >>>> assign
> >>>> + * cannot land between the check and the insert: either the sweep finds
> >>>> @vsk
> >>>> + * and closes it, or @vsk is never added.
> >>>> + */
> >>>> +bool vsock_maybe_set_connected(struct vsock_sock *vsk)
> >>>> +{
> >>>> + struct list_head *list;
> >>>> + bool reachable;
> >>>> +
> >>>> + list = vsock_connected_sockets(&vsk->remote_addr,
> >>>> &vsk->local_addr);
> >>>> +
> >>>> + spin_lock_bh(&vsock_table_lock);
> >>>> + reachable = vsock_g2h_reachable_sk(vsk);
> >>>> + if (reachable) {
> >>>> + sk_vsock(vsk)->sk_state = TCP_ESTABLISHED;
> >>>> + __vsock_insert_connected(list, vsk);
> >>>> + }
> >>>> + spin_unlock_bh(&vsock_table_lock);
> >>>> +
> >>>> + return reachable;
> >>>> +}
> >>>> +EXPORT_SYMBOL_GPL(vsock_maybe_set_connected);
> >>>> +
> >>>> +static bool vsock_g2h_should_reset(struct vsock_sock *vsk,
> >>>> + const struct vsock_transport *t)
> >>>> +{
> >>>> + return vsk->transport == t &&
> >>>> + READ_ONCE(sk_vsock(vsk)->sk_state) != TCP_CLOSE &&
> >>>> + !READ_ONCE(vsk->close_work_scheduled) &&
> >>>> + !vsock_g2h_reachable_sk(vsk);
> >>>> +}
> >>>> +
> >>>> +static struct vsock_sock *
> >>>> +vsock_g2h_next_unreachable(const struct vsock_transport *t, unsigned
> >>>> int *i)
> >>>> +{
> >>>> + struct vsock_sock *vsk;
> >>>> +
> >>>> + spin_lock_bh(&vsock_table_lock);
> >>>> +
> >>>> + for (; *i < ARRAY_SIZE(vsock_connected_table); (*i)++) {
> >>>> + list_for_each_entry(vsk, &vsock_connected_table[*i],
> >>>> + connected_table) {
> >>>> + if (vsock_g2h_should_reset(vsk, t)) {
> >>>> + sock_hold(sk_vsock(vsk));
> >>>> + spin_unlock_bh(&vsock_table_lock);
> >>>> + return vsk;
> >>>> + }
> >>>> + }
> >>>> + }
> >>>> +
> >>>> + spin_unlock_bh(&vsock_table_lock);
> >>>> +
> >>>> + return NULL;
> >>>> +}
> >>>> +
> >>>> +/* 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))) {
> >>>
> >>> can we reuse vsock_for_each_connected_socket() ?
> >>>
> >>> If not, maybe a comment here will be nice and maybe I'll try to merge
> >>> vsock_g2h_next_unreachable() here if it's not used somewhere else.
> >>
> >> I think we would need a variant that returns the socket, since we need
> >> to be able to acquire the lock_sock after releasing the
> >> vsock_table_lock.
> >
> > That would be nice if it's not too complicated.
> >
> >>
> >> Maybe the callback could return a bool, if true then return socket
> >> otherwise skip. Then the cb could also grab the sock_hold ref under the
> >> table lock.
> >
> > Yeah, looks promising to me!
> >
> >>
> >>>
> >>> I find these functions a bit hard to read.
> >>
> >> Yeah its all kind of a pain. The problem is that we need to hold
> >> sock_lock to set sk_state and sk_err, but can't acquire it under the
> >> table lock. Instead, we grab the socket ref sock_hold under the table
> >> lock to guarantee it stays alive between releasing the table lock and
> >> grabbing the socket lock.
> >>
> >> I was really tempted to not do this lock_sock dance though. The race
> >> condition I'm avoiding here is that if this reset races with an RST
> >> receive, then the socket could end up with TCP_CLOSING + sk_err
> >> ECONNRESET... but I'm not sure this actually propagates up into a real
> >> problem for any potential user.
>
> Right, vsock_connect() is fragile. Two things to be cautious about:
> 1. Don't let it reset the already connected socket. Because that would
> allow user to "re-connect()" and make mess in connected_table.
> 2. Don't let it re-assign transport of an already connected socket. Because
> that breaks the contract in few places.
>
> State 1 is not an issue: vsock_g2h_reset_unreachable() calls
> __vsock_remove_connected().
>
> State 2 may be problematic: if vsock_g2h_reset_unreachable() sets TCP_CLOSE
> when connect() is scheduled out, then after the wake-up sock may get reset
> to SS_UNCONNECTED. Then user re-connect()s and hits the transport
> re-assignment logic. I'm worried that, as we re-assign, someone may be
> holding a reference to such "connected" (even if not in connected_table
> anymore) vsock.
I see path you're looking at... after vsock_connect() gets scheduled
back in we set TCP_CLOSE/SS_UNCONNECTED if sock_error returns non-zero.
Could you elaborate on your last concern there about someone holding a
reference? I'm not sure I fully understood.
>
> So I don't know, maybe it's vsock_connect() that should be fixed/hardened
> instead?
>
> Or at least disallow transport *re*-assignment? Does the "re-" part have a
> use case?
>
> Hope it's not too off-topic.
>
> >> It also avoided some racing with the tx path, but now that send_pkt()
> >> also checks reachability I think that path is safe.
> >>
> >> What do you think about doing the below hunk (TCP_CLOSE, ECONNRESET,
> >> sk_error_report) with no lock_sock?
> >
> > Looking at the code, it seems that in all the places where we close all
> > connections (virtio_vsock_reset_sock, vhost_vsock_reset_orphans) we do
> > with no lock_sock but with vsock_table_lock.
> >
> > So, makes sense to me.
>
> virtio_vsock_reset_sock() and vhost_vsock_reset_orphans() are my pet
> peeves: state switcheroo without taking lock_sock(). Which, again, races
> vsock_connect().
True, I don't love that either, but I feel the impact is at least
constrained by the fact that it only happens when the module or device
is already going away.
Best,
Bobby
