>
> Thank you for your contribution! Sashiko AI review found 4 potential
> issue(s) to consider.
>
> Critical: 0 · High: 1 · Medium: 1 (1) · Low: 2
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [High] The new attach-time handoff passes partially read skbs to
> read_skb, which ignores the read offset.
> - [Medium] Sockmap attach/detach now drains queued data on every vsock
> socket, and the commit message wrongly says non-batched sockets keep…
> - [Low] The commit message says "Sampling at batch finish takes
> sk_callback_lock once." That holds only on the native path.
> - [Low] vsock_rx_callback_handoff() calls sk_data_ready without the
> sk_rcvlowat/SOCK_DONE check that vsock_data_ready() and the per-packet…
>
> Pre-existing issues:
> - [Medium] vsock_rx_callback_handoff() reads sk_data_ready under
> read_lock_bh(&sk->sk_callback_lock), releases the lock, and only then…
>
> --
>
> vsock/virtio: defer RX readable notifications until batch unlock
>
> During virtio vsock RX batches, this commit holds back the native
> sock_def_readable() notification until the batch has dropped the socket
> lock. If sk_data_ready or sk_write_space has been replaced, for example
> by sockmap, a new vsock_rx_callback_handoff() passes queued skbs to the
> current callback one at a time. vsock_bpf_update_proto() now schedules
> that handoff on sockmap attach and detach.
>
> > diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> > index fe7f49da6c4bc..df6faee0bd37a 100644
> > --- a/net/vmw_vsock/af_vsock.c
> > +++ b/net/vmw_vsock/af_vsock.c
> > @@ -931,6 +931,107 @@ static int __vsock_bind(struct sock *sk, struct
> > sockaddr_vm *addr)
>
> [ ... ]
>
> > +/* Caller holds lock_sock(). */
> > +void vsock_rx_callback_handoff(struct sock *sk)
> > +{
> > + struct vsock_sock *vsk = vsock_sk(sk);
> > + void (*write_space)(struct sock *sk);
> > + void (*data_ready)(struct sock *sk);
> > + bool wrote = false;
> > + s64 before, after;
> > + int i;
> > +
> > + if (sock_flag(sk, SOCK_DEAD) || !vsk->transport)
> > + return;
> > +
> > + read_lock_bh(&sk->sk_callback_lock);
> > + data_ready = READ_ONCE(sk->sk_data_ready);
> > + if (READ_ONCE(sk->sk_prot) != sk->sk_prot_creator &&
> > + data_ready == vsk->default_data_ready) {
> > + read_unlock_bh(&sk->sk_callback_lock);
> > + /*
> > + * start_verdict publishes sk_data_ready after the proto swap.
> > + * Retry once before treating this as a stable non-RX setup.
> > + */
> > + if (!vsk->rx_cb_retried) {
> > + vsk->rx_cb_retried = true;
> > + vsock_rx_callback_queue(sk, 1);
> > + return;
> > + }
> > + vsk->rx_cb_retried = false;
> > + data_ready(sk);
> > + return;
> > + }
> > + read_unlock_bh(&sk->sk_callback_lock);
> > + vsk->rx_cb_retried = false;
> > +
> > + for (i = 0; i < VSOCK_RX_CB_HANDOFF_MAX; i++) {
> > + read_lock_bh(&sk->sk_callback_lock);
> > + data_ready = READ_ONCE(sk->sk_data_ready);
> > + write_space = READ_ONCE(sk->sk_write_space);
> > + read_unlock_bh(&sk->sk_callback_lock);
>
> [Severity: Medium]
> This is a pre-existing issue, but sk_callback_lock here only covers
> reading the pointers, not calling them. Can a concurrent sockmap delete
> restore sk_data_ready and clear the psock between this read_unlock_bh()
> and the data_ready(sk) call below?
>
> The delete path takes only write_lock_bh(&sk->sk_callback_lock), not
> lock_sock():
>
> __sock_map_delete()
> sock_map_unref()
> sock_map_del_link() / sk_psock_drop()
>
> In that case the stale sk_psock_verdict_data_ready() still dequeues an
> skb through virtio_transport_read_skb(). That lowers rx_bytes and sends a
> credit update. sk_psock_verdict_recv() then finds no psock:
>
> net/core/skmsg.c:sk_psock_verdict_recv() {
> ...
> if (unlikely(!psock)) {
> len = 0;
> tcp_eat_skb(sk, skb);
> sock_drop(sk, skb);
> goto out;
> }
> ...
> }
>
> So the skb is dropped after it has already been credited to the peer.
>
> The baseline vsock_data_ready() has the same window. It calls
> sk->sk_data_ready(sk) under lock_sock() without sk_callback_lock, and
> tcp_data_ready() has the same issue. This patch adds more call sites
> with the same pattern: the attach-kicked work item and the batch-finish
> handoff. Taking the read lock here can also make it look as if the call
> is protected when it is not.
>
> > +
> > + if (!wrote && write_space != vsk->default_write_space) {
> > + write_space(sk);
> > + wrote = true;
> > + }
> > + if (data_ready == vsk->default_data_ready) {
> > + data_ready(sk);
> > + break;
> > + }
> > + before = vsock_stream_has_data(vsk);
> > + if (before <= 0)
> > + break;
> > + data_ready(sk);
>
> [Severity: Low]
> Neither of these data_ready(sk) calls checks sk->sk_rcvlowat or
> SOCK_DONE, unlike vsock_data_ready() and the per-packet path in
> virtio_transport_recv_connected(). Is that intended?
>
> For a replaced callback such as the sockmap verdict, data_ready(sk) runs
> whenever vsock_stream_has_data() > 0. The verdict program can then see
> data below SO_RCVLOWAT that the per-packet path would hold back.
>
> For the default callback the call is unconditional, both here and in the
> retry branch above:
>
> vsk->rx_cb_retried = false;
> data_ready(sk);
> return;
>
> Doesn't this wake readers when no readable event was pending? That
> happens on every attach and restore kick from vsock_bpf_update_proto().
> It also happens when virtio_transport_rx_batch_finish() enters the
> handoff with only write_space_pending set.
>
> > + after = vsock_stream_has_data(vsk);
> > + if (after >= before)
> > + break;
>
> [Severity: High]
> Can this hand a partially read skb to read_skb?
>
> A native stream read can consume only part of the head skb. In that
> case virtio_transport_stream_do_dequeue() advances the private offset,
> leaves the skb queued, and lowers rx_bytes only by the bytes copied:
>
> VIRTIO_VSOCK_SKB_CB(skb)->offset += bytes;
> ...
> virtio_transport_dec_rx_pkt(vvs, bytes, dequeued);
>
> If the socket is then added to a sockmap with a verdict program, the new
> kick in vsock_bpf_update_proto() runs this loop:
>
> vsock_rx_callback_work()
> vsock_rx_callback_handoff()
> sk_psock_verdict_data_ready()
> vsock_read_skb()
> virtio_transport_read_skb()
>
> virtio_transport_read_skb() does not look at the offset:
>
> pkt_len = le32_to_cpu(hdr->len);
> virtio_transport_dec_rx_pkt(vvs, pkt_len, pkt_len);
> ...
> return recv_actor(sk, skb);
>
> Take a 1000 byte skb with 400 bytes already read, so rx_bytes is 600.
> The verdict program sees all 1000 bytes, 400 of them a second time. The
> u32 rx_bytes then wraps to 4294966896.
>
> The after >= before check above sees no progress and stops. But
> vsock_stream_has_data() now reports about 4GB, so poll stays readable.
> The next native read hits WARN_ONCE("rx_queue is empty, but rx_bytes is
> non-zero") and returns -EFAULT.
>
> The offset handling in virtio_transport_read_skb() predates this patch.
> In the baseline the same corruption needs a partial read, then an
> attach, then another packet from the peer. With this patch the extra
> packet is no longer needed, and the new attach-time drain depends on
> read_skb handling already-queued skbs correctly.
>
> > + if (i == VSOCK_RX_CB_HANDOFF_MAX - 1 && after > 0)
> > + vsock_rx_callback_queue(sk, 0);
> > + }
> > +}
>
> [ ... ]
>
> > diff --git a/net/vmw_vsock/virtio_transport_common.c
> > b/net/vmw_vsock/virtio_transport_common.c
> > index e5637291f3185..6b96d8ac0d598 100644
> > --- a/net/vmw_vsock/virtio_transport_common.c
> > +++ b/net/vmw_vsock/virtio_transport_common.c
>
> [ ... ]
>
> > @@ -1986,33 +2007,45 @@ EXPORT_SYMBOL_GPL(virtio_transport_recv_pkt);
>
> [ ... ]
>
> > - if (write_space_pending) {
> > - vsk = vsock_sk(sk);
> > + vsk = vsock_sk(sk);
> > + if (write_space_pending)
> > vsk->default_write_space(sk);
> > - write_space = READ_ONCE(sk->sk_write_space);
> > - if (write_space != vsk->default_write_space)
> > - write_space(sk);
> > +
> > + read_lock_bh(&sk->sk_callback_lock);
> > + native = vsock_rx_cb_is_native(sk);
> > + read_unlock_bh(&sk->sk_callback_lock);
> > +
> > + if (native) {
> > + release_sock(sk);
> > + if (data_ready_pending)
> > + vsk->default_data_ready(sk);
> > + sock_put(sk);
> > + return;
> > }
> >
> > + if (write_space_pending || data_ready_pending)
> > + vsock_rx_callback_handoff(sk);
>
> [Severity: Low]
> The commit message says:
>
> Sampling at batch finish takes sk_callback_lock once.
>
> Does that hold on the non-native path? sk_callback_lock is taken once
> here for vsock_rx_cb_is_native(). vsock_rx_callback_handoff() then takes
> it again at the top. It takes it once more on each loop iteration, up
> to VSOCK_RX_CB_HANDOFF_MAX (64) times, and the requeued work can do all
> of this again.
>
> [ ... ]
>
> > diff --git a/net/vmw_vsock/vsock_bpf.c b/net/vmw_vsock/vsock_bpf.c
> > index 9049d26486460..7770a6c51dbe5 100644
> > --- a/net/vmw_vsock/vsock_bpf.c
> > +++ b/net/vmw_vsock/vsock_bpf.c
> > @@ -154,6 +154,7 @@ int vsock_bpf_update_proto(struct sock *sk, struct
> > sk_psock *psock, bool restore
> > if (restore) {
> > sk->sk_write_space = psock->saved_write_space;
> > sock_replace_proto(sk, psock->sk_proto);
> > + vsock_rx_callback_kick(sk);
> > return 0;
> > }
> >
> > @@ -166,6 +167,7 @@ int vsock_bpf_update_proto(struct sock *sk, struct
> > sk_psock *psock, bool restore
> >
> > vsock_bpf_check_needs_rebuild(psock->sk_proto);
> > sock_replace_proto(sk, &vsock_bpf_prot);
> > + vsock_rx_callback_kick(sk);
> > return 0;
> > }
>
> [Severity: Medium]
> These kicks run for every AF_VSOCK socket added to or removed from a
> sockmap or sockhash, not only for sockets in a virtio RX batch. Is this
> change in behaviour intended?
>
> RX batching exists only in the guest virtio transport
> (virtio_transport_rx_work() -> virtio_transport_recv_pkt_batch()).
> vhost_vsock and vsock_loopback always use virtio_transport_recv_pkt().
> Both still set .read_skb = virtio_transport_read_skb, so their sockets
> reach this code too. The same is true of SOCK_SEQPACKET sockets, which
> virtio_transport_recv_pkt_batchable() rejects.
>
> Before this patch, sk_data_ready was not called by either path:
>
> sock_map_link() -> sock_map_init_proto() -> vsock_bpf_update_proto()
> sk_psock_start_verdict()
>
> Data queued before attach only reached the verdict program when a later
> packet arrived.
>
> Now the kick pushes that data through sk_psock_verdict_data_ready(), up
> to 64 skbs per pass, and requeues itself if data remains. The verdict
> program can redirect or drop data that arrived before attach, where it
> used to stay for the native reader.
>
> If only the prot was swapped, for example with a msg_parser-only map,
> the handoff calls the default callback unconditionally after one retry.
> That adds an extra wakeup on every attach and detach.
>
> The commit message says "non-batched sockets keep their existing
> behavior". It also says the handoff is scheduled "so an attach or detach
> outside the worker still drains skbs already queued".
>
> Neither statement seems to fit what the code does: this is a new sockmap
> behaviour for every vsock transport that has read_skb, and the subject
> line does not mention it. Could this be split out, or at least
> described in the commit message?
>
I will drop the attempt to preserve RX batching across sockmap cb
replacements, at least for this optimization series.
> --
> Sashiko AI review ·
> https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261010142247.99223-1-physicalmtea%40gmail.com
