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.
