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

[Bobby Eshleman <bobbyeshleman@meta.com>]

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