Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 1 · Low: 0
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).
- [Medium] Both the guest and vhost transports call
virtio_transport_credit_update_xmit() before the packet is actually…
--
vsock/virtio: queue at most one credit update per socket
This patch limits how many credit update packets each virtio vsock socket
can have queued. It tracks the last queued credit update skb in
virtio_vsock_sock and suppresses new ones while that skb is pending.
Transports call a new virtio_transport_credit_update_xmit() hook to
refresh fwd_cnt/buf_alloc on the tracked skb and release tracking when it
is sent. A tracked skb holds a refcount on virtio_vsock_sock to keep it
alive.
[ ... ]
> diff --git a/net/vmw_vsock/virtio_transport_common.c
> b/net/vmw_vsock/virtio_transport_common.c
> index 74a8fdf825d50..65d6e0790e724 100644
> --- a/net/vmw_vsock/virtio_transport_common.c
> +++ b/net/vmw_vsock/virtio_transport_common.c
> @@ -604,18 +604,68 @@ static void virtio_transport_dec_rx_pkt(struct
> virtio_vsock_sock *vvs,
[ ... ]
> +/* Called by the transports when @skb is put on the wire. */
> +void virtio_transport_credit_update_xmit(struct sk_buff *skb)
> +{
> + struct virtio_vsock_sock *vvs = VIRTIO_VSOCK_SKB_CB(skb)->vvs;
> + struct virtio_vsock_hdr *hdr = virtio_vsock_hdr(skb);
> +
> + if (!vvs)
> + return;
> +
> + spin_lock_bh(&vvs->rx_lock);
> + /* Still the socket's last packet: carry what was suppressed. */
> + if (vvs->credit_update == skb) {
> + vvs->credit_update = NULL;
> + vvs->last_fwd_cnt = vvs->fwd_cnt;
> + hdr->fwd_cnt = cpu_to_le32(vvs->fwd_cnt);
> + hdr->buf_alloc = cpu_to_le32(vvs->buf_alloc);
> + }
> + spin_unlock_bh(&vvs->rx_lock);
> +}
> +EXPORT_SYMBOL_GPL(virtio_transport_credit_update_xmit);
[Severity: Medium]
This hook can't be undone once it has run. It clears vvs->credit_update,
advances vvs->last_fwd_cnt and rewrites the header. Do the guest and vhost
transports call it before the packet is actually committed?
In the guest driver, virtio_transport_send_skb() calls it before
virtqueue_add_sgs(), and that call fails when the tx vq is full:
net/vmw_vsock/virtio_transport.c:virtio_transport_send_skb() {
virtio_transport_credit_update_xmit(skb);
...
ret = virtqueue_add_sgs(vq, sgs, out_sg, in_sg, skb, gfp);
if (ret < 0)
return ret;
...
}
virtio_transport_send_pkt_work() then puts the skb back at the head:
ret = virtio_transport_send_skb(skb, vq, vsock, GFP_KERNEL);
if (ret < 0) {
virtio_vsock_skb_queue_head(&vsock->send_pkt_queue, skb);
break;
}
When the fast path fails, virtio_transport_send_pkt() queues it at the
tail instead.
The requeued skb is no longer tracked. So the next
virtio_transport_send_credit_update() is not suppressed, and it queues a
second, tracked credit update behind the first.
Doesn't that make the per-socket bound two, rather than the "at most one
credit update" the commit message describes? Any later updates are still
suppressed by the second tracked one, so the unbounded queuing stays
fixed.
In vhost, vhost_transport_do_send_pkt() calls the hook right after it
gets a descriptor, before the checks that drop the skb:
virtio_transport_credit_update_xmit(skb);
if (out) {
restart_tx |= vhost_vsock_drop_skb(vsock, skb);
...
}
iov_len = iov_length(&vq->iov[out], in);
if (iov_len < sizeof(*hdr)) {
restart_tx |= vhost_vsock_drop_skb(vsock, skb);
...
The copy_to_iter() and skb_copy_datagram_iter() fault paths drop the skb
in the same way.
Suppose updates were suppressed while this one was queued. Their fwd_cnt
has been merged into this skb and recorded in last_fwd_cnt, but the
guest never receives this skb.
Before this patch, the suppressed update would have been a separate skb,
delivered on the next descriptor. Now nothing resends it until more local
reads happen. Could the guest-side sender then stall waiting for credit?
Moving the hook after these checks would not be enough by itself, since
the suppressed state is lost whenever a tracked update is dropped.
The commit message says:
So a suppressed update always reaches the
peer, either in the queued one or in a later packet, and the fwd_cnt
the peer sees never goes back.
Is that still true on these drop paths?
[ ... ]
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261105000141.3617241-1-green%40linuxhacker.ru
