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

[Paolo Abeni <pabeni@redhat.com>]

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?

/P