Re: [PATCH net-next v4 4/7] vsock: add a netlink command to assign the g2h device to a netns

[Michal Luczaj <mhal@rbox.co>]

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