> > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) > to consider: > - [Low] vsock: unreachable callback replacement abort logic in batching > -- > > commit 811453fb266663fdded1db37d3b4cf3bde547c9f > Author: Jia Jia <[email protected]> > > vsock: defer RX readable notifications until batch unlock > > This commit defers vsock RX data-ready notifications until the batch finishes > and releases the socket lock. It records the initial callback to handle > mid-batch replacements. > > > diff --git a/net/vmw_vsock/virtio_transport_common.c > > b/net/vmw_vsock/virtio_transport_common.c > > --- a/net/vmw_vsock/virtio_transport_common.c > > +++ b/net/vmw_vsock/virtio_transport_common.c > > [ ... ] > > > @@ -2015,6 +2040,13 @@ void virtio_transport_recv_pkt_batch(struct > > virtio_transport *t, > > vsock_addr_equals_addr(&batch->dst, &dst) && > > virtio_transport_recv_pkt_batchable(t, batch->sk)) { > > sk = batch->sk; > > + defer_data_ready = READ_ONCE(sk->sk_data_ready) == > > + vsock_sk(sk)->default_data_ready; > > + if (batch->data_ready_pending && !defer_data_ready) { > > + virtio_transport_rx_batch_finish(batch); > > + goto lookup; > > + } > > + > > [Severity: Low] > Can this branch ever execute? >
Unless I'm mistaken, this branch can actually be reached. > The condition checks if a custom callback was installed (!defer_data_ready) > while a default notification is already pending. > > Since custom callback attachments (such as BPF sockmap insertion) require > acquiring the exact same socket lock, and the lock is held continuously while > the batch is active, is it possible for the callback to change from default > to custom mid-batch? > > Could this bailout branch be unreachable dead code due to the structural > mutual > exclusion provided by the socket lock? > The premise that sockmap updates always take lock_sock(sk) applies only to the syscall path. When a BPF program calls bpf_map_update_elem(), for example from SK_LOOKUP or BPF_TRACE_ITER, sock_map_update_elem() takes bh_lock_sock(sk), The batch reuse path checks sk->sk_prot in virtio_transport_recv_pkt_batchable() before reading sk->sk_data_ready. A concurrent BPF update can replace both callbacks between those reads while the RX worker owns the socket. > -- > Sashiko AI review ยท > https://sashiko.dev/#/patchset/[email protected]?part=5
