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

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?

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

Ah that's true.

Thanks,
Bobby