On Wed, 18 Mar 2026 12:11:15 +0800, Jason Wang <[email protected]> wrote: > On Wed, Mar 18, 2026 at 12:07 PM Jason Wang <[email protected]> wrote: > > > > On Fri, Mar 13, 2026 at 3:59 PM Xuan Zhuo <[email protected]> > > wrote: > > > > > > The commit be50da3e9d4a ("net: virtio_net: implement exact header length > > > guest feature") introduces support for the VIRTIO_NET_F_GUEST_HDRLEN > > > feature in virtio-net. > > > > > > This feature requires virtio-net to set hdr_len to the actual header > > > length of the packet when transmitting, the number of > > > bytes from the start of the packet to the beginning of the > > > transport-layer payload. > > > > > > However, in practice, hdr_len was being set using skb_headlen(skb), > > > which is clearly incorrect. This commit fixes that issue. > > > > > > Fixes: be50da3e9d4a ("net: virtio_net: implement exact header length > > > guest feature") > > > Signed-off-by: Xuan Zhuo <[email protected]> > > > --- > > > drivers/net/tun_vnet.h | 2 +- > > > drivers/net/virtio_net.c | 6 +++++- > > > include/linux/virtio_net.h | 33 +++++++++++++++++++++++++++++---- > > > 3 files changed, 35 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/net/tun_vnet.h b/drivers/net/tun_vnet.h > > > index a5f93b6c4482..fa5cab9d3e55 100644 > > > --- a/drivers/net/tun_vnet.h > > > +++ b/drivers/net/tun_vnet.h > > > @@ -244,7 +244,7 @@ tun_vnet_hdr_tnl_from_skb(unsigned int flags, > > > > > > if (virtio_net_hdr_tnl_from_skb(skb, tnl_hdr, has_tnl_offload, > > > tun_vnet_is_little_endian(flags), > > > - vlan_hlen, true)) { > > > + vlan_hlen, true, false)) { > > > struct virtio_net_hdr_v1 *hdr = &tnl_hdr->hash_hdr.hdr; > > > struct skb_shared_info *sinfo = skb_shinfo(skb); > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > > index 72d6a9c6a5a2..7106333ef904 100644 > > > --- a/drivers/net/virtio_net.c > > > +++ b/drivers/net/virtio_net.c > > > @@ -3267,8 +3267,12 @@ static int xmit_skb(struct send_queue *sq, struct > > > sk_buff *skb, bool orphan) > > > struct virtio_net_hdr_v1_hash_tunnel *hdr; > > > int num_sg; > > > unsigned hdr_len = vi->hdr_len; > > > + bool feature_hdrlen; > > > bool can_push; > > > > > > + feature_hdrlen = virtio_has_feature(vi->vdev, > > > + VIRTIO_NET_F_GUEST_HDRLEN); > > > + > > > pr_debug("%s: xmit %p %pM\n", vi->dev->name, skb, dest); > > > > > > /* Make sure it's safe to cast between formats */ > > > @@ -3288,7 +3292,7 @@ static int xmit_skb(struct send_queue *sq, struct > > > sk_buff *skb, bool orphan) > > > > > > if (virtio_net_hdr_tnl_from_skb(skb, hdr, vi->tx_tnl, > > > > > > virtio_is_little_endian(vi->vdev), 0, > > > - false)) > > > + false, feature_hdrlen)) > > > return -EPROTO; > > > > > > if (vi->mergeable_rx_bufs) > > > diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h > > > index 75dabb763c65..48de4a16a96a 100644 > > > --- a/include/linux/virtio_net.h > > > +++ b/include/linux/virtio_net.h > > > @@ -207,6 +207,22 @@ static inline int virtio_net_hdr_to_skb(struct > > > sk_buff *skb, > > > return __virtio_net_hdr_to_skb(skb, hdr, little_endian, > > > hdr->gso_type); > > > } > > > > > > +static inline void virtio_net_set_hdrlen(const struct sk_buff *skb, > > > + struct virtio_net_hdr *hdr, > > > + bool little_endian) > > > +{ > > > + u16 hdr_len; > > > + > > > + hdr_len = skb_transport_offset(skb); > > > + > > > + if (hdr->gso_type == VIRTIO_NET_HDR_GSO_UDP_L4) > > > + hdr_len += sizeof(struct udphdr); > > > + else > > > + hdr_len += tcp_hdrlen(skb); > > > > Ok, I think this depends on the logic inside virtio_net_hdr_from_skb() > > > > if (sinfo->gso_type & SKB_GSO_TCPV4) > > hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV4; > > else if (sinfo->gso_type & SKB_GSO_TCPV6) > > hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV6; > > else if (sinfo->gso_type & SKB_GSO_UDP_L4) > > hdr->gso_type = VIRTIO_NET_HDR_GSO_UDP_L4; > > else > > return -EINVAL; > > > > To be more robust, I'd suggest moving it there. > > > > But I have another question, the logic above depends on the headlen is > > correctly set: > > > > /* This is a hint as to how much should be linear. */ > > hdr->hdr_len = __cpu_to_virtio16(little_endian, > > skb_headlen(skb)); > > > > Is the headlen guaranteed to be correct in all cases (e.g for nested > > setups or dodgy packets?) > > Speak too fast, I miss > > hdr_len = skb_transport_offset(skb); > > This is probably another call to > > 1) Move virtio_net_set_hdrlen() inside virtio_net_hdr_from_skb() or > 2) call virtio_net_set_hdrlen() inside virtio_net_hdr_from_skb().
Do you more like v8? If so, please reivew this: http://lore.kernel.org/all/[email protected] Thanks > > Thanks > > > > > Thanks > > > > > + > > > + hdr->hdr_len = __cpu_to_virtio16(little_endian, hdr_len); > > > +} > > > + > > > static inline int virtio_net_hdr_from_skb(const struct sk_buff *skb, > > > struct virtio_net_hdr *hdr, > > > bool little_endian, > > > @@ -385,7 +401,8 @@ virtio_net_hdr_tnl_from_skb(const struct sk_buff *skb, > > > bool tnl_hdr_negotiated, > > > bool little_endian, > > > int vlan_hlen, > > > - bool has_data_valid) > > > + bool has_data_valid, > > > + bool feature_hdrlen) > > > { > > > struct virtio_net_hdr *hdr = (struct virtio_net_hdr *)vhdr; > > > unsigned int inner_nh, outer_th; > > > @@ -394,9 +411,17 @@ virtio_net_hdr_tnl_from_skb(const struct sk_buff > > > *skb, > > > > > > tnl_gso_type = skb_shinfo(skb)->gso_type & (SKB_GSO_UDP_TUNNEL | > > > > > > SKB_GSO_UDP_TUNNEL_CSUM); > > > - if (!tnl_gso_type) > > > - return virtio_net_hdr_from_skb(skb, hdr, little_endian, > > > - has_data_valid, vlan_hlen); > > > + if (!tnl_gso_type) { > > > + ret = virtio_net_hdr_from_skb(skb, hdr, little_endian, > > > + has_data_valid, vlan_hlen); > > > + if (ret) > > > + return ret; > > > + > > > + if (feature_hdrlen && hdr->hdr_len) > > > + virtio_net_set_hdrlen(skb, hdr, little_endian); > > > + > > > + return ret; > > > + } > > > > > > /* Tunnel support not negotiated but skb ask for it. */ > > > if (!tnl_hdr_negotiated) > > > -- > > > 2.32.0.3.g01195cf9f > > > >
