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

[Michal Luczaj]

On 10/30/26 21:30, Bobby Eshleman wrote:
>>>> 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.

Ah, sorry, let me explain. This case reminded me of the vsock's TCP_CLOSING
bug[1]: reporter mentioned vsk->trans getting freed when skb was still in
flight. I did not reproduce the exact scenario, but it did sound plausible
and I thought it applies here: virtio_transport_destruct() frees vsk->trans
the moment vsock_loopback_work() invokes virtio_transport_consume_skb_sent().

Now, to get there let's start with vsock_connect() setting SS_UNCONNECTED
(on a connected) socket after vsock_g2h_reset_unreachable(). Then user is
able to re-assign the transport, which involves
virtio_transport_destruct(). But since we're dealing with a connected
socket, (before the reset) some send() might have triggered
vsock_loopback_work(). Does this make sense to you?

Another thing is this feature of vsock_assign_transport(): in some
re-assign failure paths you may end up with vsk->transport = NULL. In the
past this caused issues with sockmap: see WARN_ON_ONCE in
vsock_bpf_recvmsg(). Problem here is that normally vsock gets removed from
sockmap on close(), but in our (reset+reassign) case close() is never invoked.

[1]: https://lore.kernel.org/netdev/anzT1fREOSyHT99k@v4bel/

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

Doesn't it happen on suspend as well? Is so, with a poorly timed connect()
things can go south on resume.