Re: [PATCH net v4 4/4] virtio-net: correct hdr_len handling for tunnel gso
From: Paolo Abeni <pabeni@redhat.com>
Date: 2025-10-29 09:51:50
Also in:
virtualization
On Wed, Oct 29, 2025 at 4:09 AM Xuan Zhuo [off-list ref] wrote:
quoted hunk ↗ jump to hunk
diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h index 6ef0b737d548..46b04816d333 100644 --- a/include/linux/virtio_net.h +++ b/include/linux/virtio_net.h@@ -207,6 +207,14 @@ 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 int virtio_net_tcp_hdrlen(const struct sk_buff *skb, bool
tnl)
quoted hunk ↗ jump to hunk
+{ + if (tnl) + return inner_tcp_hdrlen(skb); + + return tcp_hdrlen(skb); +} + static inline int virtio_net_hdr_from_skb(const struct sk_buff *skb, struct virtio_net_hdr *hdr, bool little_endian,@@ -217,25 +225,33 @@ static inline int virtio_net_hdr_from_skb(const
struct sk_buff *skb,
if (skb_is_gso(skb)) {
struct skb_shared_info *sinfo = skb_shinfo(skb);
+ bool tnl = false;
u16 hdr_len = 0;
- /* In certain code paths (such as the af_packet.c receivepath),
- * this function may be called without a transport header.
- * In this case, we do not need to set the hdr_len.
- */
- if (skb_transport_header_was_set(skb))
- hdr_len = skb_transport_offset(skb);
+ if (sinfo->gso_type & (SKB_GSO_UDP_TUNNEL |
+ SKB_GSO_UDP_TUNNEL_CSUM)) {
+ tnl = true;
+ hdr_len = skb_inner_transport_offset(skb);
+
+ } else {
+ /* In certain code paths (such as the af_packet.creceive path),
+ * this function may be called without a
transport header.
+ * In this case, we do not need to set the
hdr_len.
+ */
+ if (skb_transport_header_was_set(skb))
+ hdr_len = skb_transport_offset(skb);
+ }
hdr->gso_size = __cpu_to_virtio16(little_endian,
sinfo->gso_size);
if (sinfo->gso_type & SKB_GSO_TCPV4) {
hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
if (hdr_len)
- hdr_len += tcp_hdrlen(skb);
+ hdr_len += virtio_net_tcp_hdrlen(skb,tnl);
} else if (sinfo->gso_type & SKB_GSO_TCPV6) {
hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
if (hdr_len)
- hdr_len += tcp_hdrlen(skb);
+ hdr_len += virtio_net_tcp_hdrlen(skb,tnl);
} else if (sinfo->gso_type & SKB_GSO_UDP_L4) {
hdr->gso_type = VIRTIO_NET_HDR_GSO_UDP_L4;
if (hdr_len)I think it's a bit of a pity that the non-UDP tunnel path had to do all the additional conditionals. The (completely untested) alternative attached here would reduce them a bit. Still I'm a bit concerned by all the 'if (hdr_len)' unconditionally sprinkled around by the previous patch. The virtio spec says that hdr_len is valid if and only if the VIRTIO_NET_F_GUEST_HDRLEN feature has been negotiated. What about moving hdr->hdr_len initialization in a separate helper and calling it only when the relevant feature has been negotiated? Thanks, Paolo
Attachments
- diffs [application/octet-stream] 853 bytes