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 11/2/26 20:43, Bobby Eshleman wrote:
> On Mon, Nov 02, 2026 at 02:21:27AM +0100, Michal Luczaj wrote:
>> 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?
> 
> Ah you're right. I think that your approach of making vsock_connect()
> more robust is probably the right way to go...
> 
> I think this is the new race...
> 
> 1) vsock_connect(cid=VMADDR_CID_HOST)
>    -> enqueue pkt
>    -> sleep
> 
> 2) rx worker receives OP_RESPONSE
>    ->  sets TCP_ESTABLISHED/SS_CONNECTED
> 
>    (vsock_connect() still asleep)
> 
> 2) sendmsg()
>    -> sees SS_CONNECTED
>    -> enqueues skb
> 
> 3) netns reset sets TCP_CLOSE and sk_err=ECONNRESET
> 
> 4) vsock_connect()
>    -> scheduled back in
>    -> sees sk_err != 0
>    -> sets SS_UNCONNECTED
> 
> 5) vsock_connect(CID=VMADDR_CID_LOCAL) call #2
>    -> sees SS_UNCONNECTED
>    -> frees old vsk->trans
> 
> 6) sendmsg's skb tx completes
>    -> tx worker dereferences vsk->trans for the UAF

Yes, that's exactly what I've meant.

> If connect() checks for SS_CONNECTING before setting SS_UNCONNECTED,
> then in step 4 sock->state stays as SS_CONNECTING and the second
> connect() call will hit -EISCONN instead of freeing vsk->trans.
> 
> Seems to work for a racing vsock_shutdown() too, where sock->state would
> become SS_DISCONNECTING, and the second connect() would hit -EINVAL
> instead of freeing the vsk->trans.
> 
> I guess the rule would be that vsock_connect() sets SS_CONNECTING before
> sleeping and only changes it to SS_UNCONNECTED after waking if and only
> if it is still SS_CONNECTING.
> 
> WDYT?

I think it's a great idea. Does seem to solve quite a lot.

thanks,
Michal