netdev - Re: [PATCH net v4 4/4] virtio-net: correct hdr

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAF6piCLkv6kFqoq7OQfJ=Su9AVHSQ9J7DzaumOSf5xuf9w-kyA@mail.gmail.com>
Date: Wed, 29 Oct 2025 10:51:34 +0100
From: Paolo Abeni <pabeni@...hat.com>
To: Xuan Zhuo <xuanzhuo@...ux.alibaba.com>
Cc: netdev@...r.kernel.org, "Michael S. Tsirkin" <mst@...hat.com>, 
	Jason Wang <jasowang@...hat.com>, Eugenio Pérez <eperezma@...hat.com>, 
	Andrew Lunn <andrew+netdev@...n.ch>, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, 
	Heng Qi <hengqi@...ux.alibaba.com>, Willem de Bruijn <willemb@...gle.com>, 
	Jiri Pirko <jiri@...nulli.us>, Alvaro Karsz <alvaro.karsz@...id-run.com>, 
	virtualization@...ts.linux.dev
Subject: Re: [PATCH net v4 4/4] virtio-net: correct hdr_len handling for
 tunnel gso

On Wed, Oct 29, 2025 at 4:09 AM Xuan Zhuo <xuanzhuo@...ux.alibaba.com>
wrote:
> 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)
> +{
> +       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 receive
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);
> +               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.c
receive 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

Content of type "text/html" skipped

Download attachment "diffs" of type "application/octet-stream" (853 bytes)