Re: [PATCH net v10 1/2] virtio-net: correct hdr_len handling for VIRTIO_NET_F_GUEST_HDRLEN

[Jason Wang <jasowang@redhat.com>]

On Thu, Mar 19, 2026 at 4:15 PM Paolo Abeni <pabeni@redhat.com> wrote:
>
> Hi,
>
> On 3/18/26 5:11 AM, Jason Wang wrote:
> > On Wed, Mar 18, 2026 at 12:07 PM Jason Wang <jasowang@redhat.com> wrote:
> >> On Fri, Mar 13, 2026 at 3:59 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> >>> 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().
>
> It looks like that the above leads to more complex code with a bunch of
> additional conditionals, which possibly the compiler can't optimize out,
> and IMHO not nice layering violation, see the already mentioned previous
> iteration:
>
> https://lore.kernel.org/all/cf62a942-e1e8-4b37-837a-b0bd76297656@redhat.com/
>
> What about instead simply rename virtio_net_set_hdrlen() to
> __virtio_net_set_hdrlen(), virtio_net_set_tnl_hdrlen() to
> __virtio_net_set_tnl_hdrlen(), and add explicitly mention in a comment
> that such functions must be invoked only after virtio_net_hdr_from_skb()
> validation?

Works for me.

Thanks

>
> /P
>