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.
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().
