Re: [PATCH 6.12] vsock/virtio: fix zerocopy completion for multi-skb sends

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Failure during zerocopy skb allocation advances the user's `iov_iter` without reverting it, causing subsequent loop iterations to skip bytes and silently corrupt the message stream.
- [High] MSG_ZEROCOPY violates its API contract by generating multiple completion IDs for a single sendmsg() syscall when the message is fragmented by the socket's wait loop, which can cause Use-After-Free in userspace.
--

--- Patch [1]: [PATCH 6.12] vsock/virtio: fix zerocopy completion for multi-skb sends ---
commit 60570416b19b3acf053340573a6882df58212cc4
Author: Stefano Garzarella <sgarzare@redhat.com>

vsock/virtio: fix zerocopy completion for multi-skb sends

This commit fixes an issue where non-final skbs in a fragmented message do
not carry zerocopy completion tracking. It resolves this by pre-allocating
the zerocopy uarg before the send loop and attaching it to every skb via
skb_zcopy_set. Notification only fires after the last skb is freed.

Link: https://patch.msgid.link/20260514092948.268720-1-sgarzare@redhat.com

> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> index 9acc13ab3f822..62d753e3df358 100644
> --- a/net/vmw_vsock/virtio_transport_common.c
> +++ b/net/vmw_vsock/virtio_transport_common.c

[ ... ]

> @@ -372,6 +345,25 @@ static int virtio_transport_send_pkt_info(struct vsock_sock *vsk,
>  		if (can_zcopy)
>  			max_skb_len = min_t(u32, VIRTIO_VSOCK_MAX_PKT_BUF_SIZE,
>  					    (MAX_SKB_FRAGS * PAGE_SIZE));
> +
> +		if (info->msg->msg_flags & MSG_ZEROCOPY &&
> +		    info->op == VIRTIO_VSOCK_OP_RW) {
> +			uarg = info->msg->msg_ubuf;
> +
> +			if (!uarg) {
> +				uarg = msg_zerocopy_realloc(sk_vsock(vsk),
> +							    pkt_len, NULL);

[Severity: High]
This is a pre-existing issue, but does MSG_ZEROCOPY generate multiple 
completion IDs for a single sendmsg() syscall here? 

If the peer lacks sufficient credit, stream_enqueue() in af_vsock.c can
return a short write, causing vsock_connectible_sendmsg() to loop and wait. 
In the next iteration, info->msg->msg_ubuf remains NULL because it was 
never updated with the previously allocated uarg.

When virtio_transport_send_pkt_info() is called again, does it allocate a 
new uarg with msg_zerocopy_realloc(..., NULL) and increment the socket's 
sk_zckey? This would return multiple completion IDs for one syscall, which 
might cause userspace to desynchronize its tracking and prematurely free 
buffers.

> +				if (!uarg) {
> +					virtio_transport_put_credit(vvs, pkt_len);
> +					return -ENOMEM;
> +				}
> +
> +				if (!can_zcopy)
> +					uarg_to_msgzc(uarg)->zerocopy = 0;
> +
> +				have_uref = true;
> +			}
> +		}
>  	}
>  
>  	rest_len = pkt_len;
> @@ -390,21 +382,7 @@ static int virtio_transport_send_pkt_info(struct vsock_sock *vsk,
>  			break;
>  		}

[Severity: Critical]
This isn't a bug introduced by this patch, but what happens to the user's
iov_iter if virtio_transport_alloc_skb() (called just above this snippet)
fails and the loop breaks?

If virtio_transport_alloc_skb() fails because __zerocopy_sg_from_iter() 
hits an error after successfully pinning some pages, the iter has already 
been advanced. Since rest_len != pkt_len, virtio_transport_send_pkt_info()
returns the partial byte count without reverting msg_iter.

Will the next iteration of vsock_connectible_sendmsg() read from the 
advanced iter, permanently skipping the user bytes that were pinned in the
failed skb and silently corrupting the stream?

> -		/* We process buffer part by part, allocating skb on
> -		 * each iteration. If this is last skb for this buffer
> -		 * and MSG_ZEROCOPY mode is in use - we must allocate
> -		 * completion for the current syscall.
> -		 */
> -		if (info->msg && info->msg->msg_flags & MSG_ZEROCOPY &&
> -		    skb_len == rest_len && info->op == VIRTIO_VSOCK_OP_RW) {
> -			if (virtio_transport_init_zcopy_skb(vsk, skb,
> -							    info->msg,
> -							    can_zcopy)) {
> -				kfree_skb(skb);
> -				ret = -ENOMEM;
> -				break;
> -			}
> -		}
> +		skb_zcopy_set(skb, uarg, NULL);
>  
>  		virtio_transport_inc_tx_pkt(vvs, skb);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716163600.115458-1-alexevgmart@gmail.com?part=1