Re: [PATCH net 3/3] vsock/virtio: queue at most one credit update per socket

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] vsock/virtio: premature credit_update clear in virtio_transport_send_skb()
--

commit 2cdb1a5b77890d0e5001ce1a907ca3477d945d5d
Author: Oleg Drokin <green@linuxhacker.ru>

vsock/virtio: queue at most one credit update per socket

This commit limits each vsock socket to at most one queued credit update
packet when its last queued packet is already an OP_CREDIT_UPDATE. When the
transport transmits the queued credit update and no newer packet has been
queued after it, virtio_transport_credit_update_xmit() refreshes its
fwd_cnt and buf_alloc and clears vvs->credit_update.

> diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
> index 4f9aa9c4c3aa5..82cd52249de3c 100644
> --- a/net/vmw_vsock/virtio_transport.c
> +++ b/net/vmw_vsock/virtio_transport.c
> @@ -104,6 +104,8 @@ static int virtio_transport_send_skb(struct sk_buff *skb, struct virtqueue *vq,
>  	int ret, in_sg = 0, out_sg = 0;
>  	struct scatterlist **sgs;
>  
> +	virtio_transport_credit_update_xmit(skb);
> +
>  	sgs = vsock->out_sgs;
>  	sg_init_one(sgs[out_sg], virtio_vsock_hdr(skb),
>  		    sizeof(*virtio_vsock_hdr(skb)));

[Severity: High]
Does calling virtio_transport_credit_update_xmit(skb) before
virtqueue_add_sgs() clear vvs->credit_update and advance vvs->last_fwd_cnt
even when adding the packet to the tx virtqueue fails?

In virtio_transport_send_skb(), virtqueue_add_sgs() can fail (for example
with -ENOSPC when VSOCK_VQ_TX is full, or -ENOMEM under GFP_ATOMIC on the
fast path):

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;
}

When that happens, virtio_transport_credit_update_xmit(skb) has already set
vvs->credit_update = NULL and vvs->last_fwd_cnt = vvs->fwd_cnt, and the
caller then queues or requeues skb in vsock->send_pkt_queue:

- via virtio_transport_send_pkt() -> virtio_transport_send_skb_fast_path(),
  which falls back to virtio_vsock_skb_queue_tail(&vsock->send_pkt_queue, skb)
  when vsock->send_pkt_queue was initially empty, or
- via virtio_transport_send_pkt_work(), which puts skb back with
  virtio_vsock_skb_queue_head(&vsock->send_pkt_queue, skb).

While skb sits in vsock->send_pkt_queue with vvs->credit_update cleared to
NULL, subsequent calls to virtio_transport_send_credit_update() (from
OP_CREDIT_REQUEST or local reads) will queue additional OP_CREDIT_UPDATE
skbs behind it. In addition, when skb is later sent by
virtio_transport_send_pkt_work(), virtio_transport_credit_update_xmit(skb)
sees vvs->credit_update != skb and will not refresh hdr->fwd_cnt or
hdr->buf_alloc with credits freed while it was queued.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105000141.3617241-1-green@linuxhacker.ru?part=3