Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [High] Concurrent non-atomic read-modify-write on vsk->peer_shutdown without synchronization leads to lost shutdown flags, potentially hanging host processes. -- commit d7feaefbab86483ded481345d096a59fb79a2b29 Author: Ziyu Zhang <[email protected]> vsock: keep poll shutdown state consistent This patch updates vsock polling to use a consistent snapshot of the peer_shutdown state to avoid race conditions between checking for HUP/RDHUP and EOF readability. It introduces lockless READ_ONCE and WRITE_ONCE accessors for peer_shutdown in datagram and stream sockets. > diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c > --- a/net/vmw_vsock/vmci_transport.c > +++ b/net/vmw_vsock/vmci_transport.c [ ... ] > @@ -1542,7 +1542,9 @@ static int vmci_transport_recv_connected(struct sock > *sk, > if (pkt->u.mode) { > vsk = vsock_sk(sk); > > - vsk->peer_shutdown |= pkt->u.mode; > + WRITE_ONCE(vsk->peer_shutdown, > + READ_ONCE(vsk->peer_shutdown) | > + pkt->u.mode); [Severity: High] This is a pre-existing issue, but does this non-atomic read-modify-write sequence risk losing concurrent updates to the shutdown mask? While vmci_transport_recv_connected() executes under lock_sock(), vmci_transport_handle_detach() can be called from softirq context via the queue pair detach event. The detach event uses bh_lock_sock() without checking sock_owned_by_user() to defer execution to the socket backlog. Because lock_sock() releases the inner spinlock while setting sk_lock.owned=1, bh_lock_sock() can successfully acquire the lock and execute concurrently. If the softirq executes exactly between the READ_ONCE() and WRITE_ONCE() calls here: CPU 1 (vmci_transport_recv_connected): read = READ_ONCE(vsk->peer_shutdown); CPU 2 (softirq detach callback): vmci_transport_handle_detach() WRITE_ONCE(vsk->peer_shutdown, SHUTDOWN_MASK); CPU 1 (vmci_transport_recv_connected): WRITE_ONCE(vsk->peer_shutdown, read | pkt->u.mode); Could CPU 1 overwrite the SHUTDOWN_MASK written by CPU 2? If the SHUTDOWN_MASK is lost, it appears host processes blocking in poll() could be left sleeping indefinitely because the connection closure flags won't be fully recognized. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
