Re:Re: [PATCH] vhost/net: Fill virtio_net_hdr GSO/csum metadata on RX

["Xiong Weimin" <15927021679@163.com>]

Hi Michael,


Thanks for the review.


On why VHOST_NET_F_VIRTIO_NET_HDR:


We hit this with vhost-user backends that do not expose IFF_VNET_HDR
(e.g. DPDK/custom socket backends). When the guest negotiates
VHOST_NET_F_VIRTIO_NET_HDR, vhost is responsible for supplying
virtio_net_hdr on RX. The current code always zeroes the header
(GSO_NONE), so guests that negotiated GUEST_TSO*/GUEST_CSUM never
receive correct offload metadata even when the socket skb has it.


The goal is to make RX offload metadata correct for that configuration.
TX TSO is intentionally left for a follow-up series.


On the race you pointed out:


You're right — peeking the skb under sk_receive_queue.lock and then
building the header after dropping the lock is racy if another context
can dequeue the skb before recvmsg() runs. I see why the header filling
ended up in tun, where the backend owns the skb lifecycle.


I can think of a few options:
  a) Drop this approach and not use VHOST_NET_F_VIRTIO_NET_HDR for
     these backends (keep zeroed headers / no guest offload).
  b) Move the metadata extraction to the backend (similar to tun).
  c) Hold the receive-queue lock across peek + recvmsg if that is
     acceptable for this path (I need to check whether recvmsg can
     be called under that lock).


Could you suggest which direction you'd prefer? I'm happy to respin
once we agree on the right integration point.


I'll also fix the multiline comment to follow the net convention:


/* When VHOST_NET_F_VIRTIO_NET_HDR is set, vhost supplies virtio_net_hdr.
 * Populate GSO/checksum metadata from the socket skb ...
 */


Thanks,
Weimin

At 2026-07-13 15:03:39, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>On Mon, Jul 13, 2026 at 09:04:42AM +0800, weimin xiong wrote:
>> From: xiongweimin <xiongweimin@kylinos.cn>
>> 
>> When VHOST_NET_F_VIRTIO_NET_HDR is set, vhost supplies virtio_net_hdr to
>> the guest but previously always wrote a zeroed header (GSO_NONE). Guests
>> that rely on GUEST_TSO*/GUEST_CSUM therefore never saw offload metadata.
>
>Right. Question is why are you using VHOST_NET_F_VIRTIO_NET_HDR?
>
>> 
>> Peek the socket skb before recvmsg and populate the header with
>> virtio_net_hdr_from_skb(). Also advertise the corresponding guest offload
>> feature bits from VHOST_GET_FEATURES.
>> 
>> TX TSO toward backends without IFF_VNET_HDR is intentionally left for a
>> follow-up series.
>> 
>> Signed-off-by: xiongweimin <xiongweimin@kylinos.cn>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Cc: Jason Wang <jasowang@redhat.com>
>> Cc: virtualization@vger.kernel.org
>> Cc: netdev@vger.kernel.org
>> ---
>> 
>> --- a/drivers/vhost/net.c
>> +++ b/drivers/vhost/net.c
>> @@ -73,6 +73,10 @@
>>  	VHOST_FEATURES,
>>  	VHOST_NET_F_VIRTIO_NET_HDR,
>>  	VIRTIO_NET_F_MRG_RXBUF,
>> +	VIRTIO_NET_F_GUEST_CSUM,
>> +	VIRTIO_NET_F_GUEST_TSO4,
>> +	VIRTIO_NET_F_GUEST_TSO6,
>> +	VIRTIO_NET_F_GUEST_ECN,
>>  	VIRTIO_F_ACCESS_PLATFORM,
>>  	VIRTIO_F_RING_RESET,
>>  	VIRTIO_F_IN_ORDER,
>> @@ -644,7 +648,7 @@
>>  static size_t init_iov_iter(struct vhost_virtqueue *vq, struct iov_iter *iter,
>>  			    size_t hdr_size, int out)
>>  {
>> -	/* Skip header. TODO: support TSO. */
>> +	/* Skip guest virtio_net_hdr; TX TSO handled in a follow-up. */
>>  	size_t len = iov_length(vq->iov, out);
>>  
>>  	iov_iter_init(iter, ITER_SOURCE, vq->iov, out, len);
>> @@ -1025,6 +1029,35 @@
>>  	return len;
>>  }
>>  
>> +/*
>> + * When VHOST_NET_F_VIRTIO_NET_HDR is set, vhost supplies virtio_net_hdr.
>> + * Populate GSO/checksum metadata from the socket skb so guests that
>> + * negotiated GUEST_TSO*/GUEST_CSUM receive correct offload information.
>> + */
>
>this is a wrong type of multiline comment. this file follows net
>convention:
>
>/* AAA
> * BBB
> */
>
>> +static int vhost_net_hdr_from_sock(struct vhost_virtqueue *vq, struct sock *sk,
>> +				   struct virtio_net_hdr *hdr)
>> +{
>> +	struct sk_buff *skb;
>> +	unsigned long flags;
>> +	int vlan_hlen = 0;
>> +	int ret;
>> +
>> +	spin_lock_irqsave(&sk->sk_receive_queue.lock, flags);
>> +	skb = skb_peek(&sk->sk_receive_queue);
>> +	if (!skb) {
>> +		spin_unlock_irqrestore(&sk->sk_receive_queue.lock, flags);
>> +		memset(hdr, 0, sizeof(*hdr));
>> +		hdr->gso_type = VIRTIO_NET_HDR_GSO_NONE;
>> +		return 0;
>> +	}
>> +	if (skb_vlan_tag_present(skb))
>> +		vlan_hlen = VLAN_HLEN;
>> +	ret = virtio_net_hdr_from_skb(skb, hdr, vhost_is_little_endian(vq),
>> +				      true, vlan_hlen);
>> +	spin_unlock_irqrestore(&sk->sk_receive_queue.lock, flags);
>> +	return ret;
>> +}
>
>
>This means the header will be wrong if something consumes
>the skb after we drop the lock, no?
>
>That's why in the end we put the header filling logic
>in tun, it can avoid races there.
>
>
>> +
>>  static int vhost_net_rx_peek_head_len(struct vhost_net *net, struct sock *sk,
>>  				      bool *busyloop_intr, unsigned int *count)
>>  {
>> @@ -1239,10 +1272,18 @@
>>  		/* We don't need to be notified again. */
>>  		iov_iter_init(&msg.msg_iter, ITER_DEST, vq->iov, in, vhost_len);
>>  		fixup = msg.msg_iter;
>> -		if (unlikely((vhost_hlen))) {
>> -			/* We will supply the header ourselves
>> -			 * TODO: support TSO.
>> +		if (unlikely(vhost_hlen)) {
>> +			/*
>> +			 * Build virtio_net_hdr from the socket skb before
>> +			 * recvmsg consumes it. Skip for ptr_ring backends
>> +			 * where the skb is not on sk_receive_queue.
>>  			 */
>> +			if (!nvq->rx_ring &&
>> +			    vhost_net_hdr_from_sock(vq, sock->sk, &hdr)) {
>> +				vq_err(vq, "Failed to build vnet_hdr from skb\n");
>> +				vhost_discard_vq_desc(vq, headcount, ndesc);
>> +				continue;
>> +			}
>>  			iov_iter_advance(&msg.msg_iter, vhost_hlen);
>>  		}
>>  		err = sock->ops->recvmsg(sock, &msg,
>> @@ -1270,7 +1311,6 @@
>>  			 */
>>  			iov_iter_advance(&fixup, sizeof(hdr));
>>  		}
>> -		/* TODO: Should check and handle checksum. */
>>  
>>  		num_buffers = cpu_to_vhost16(vq, headcount);
>>  		if (likely(set_num_buffers) &&