Re: [PATCH] hw/net/net_tx_pkt: Fix virtio header without checksum offloading

[Akihiko Odaki <akihiko.odaki@daynix.com>]

On 2024/03/27 12:24, Jason Wang wrote:
> On Wed, Mar 27, 2024 at 11:11 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> On 2024/03/27 12:06, Jason Wang wrote:
>>> On Wed, Mar 27, 2024 at 11:05 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>>
>>>> On 2024/03/27 11:59, Jason Wang wrote:
>>>>> On Wed, Mar 27, 2024 at 10:53 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>>>>
>>>>>> On 2024/03/27 11:50, Jason Wang wrote:
>>>>>>> On Tue, Mar 26, 2024 at 3:04 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>>>>>>
>>>>>>>> On 2024/03/26 15:51, Jason Wang wrote:
>>>>>>>>> On Sun, Mar 24, 2024 at 4:32 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>>>>>>>>
>>>>>>>>>> It is incorrect to have the VIRTIO_NET_HDR_F_NEEDS_CSUM set when
>>>>>>>>>> checksum offloading is disabled so clear the bit. Set the
>>>>>>>>>> VIRTIO_NET_HDR_F_DATA_VALID bit instead to tell the checksum is valid.
>>>>>>>>>>
>>>>>>>>>> TCP/UDP checksum is usually offloaded when the peer requires virtio
>>>>>>>>>> headers because they can instruct the peer to compute checksum. However,
>>>>>>>>>> igb disables TX checksum offloading when a VF is enabled whether the
>>>>>>>>>> peer requires virtio headers because a transmitted packet can be routed
>>>>>>>>>> to it and it expects the packet has a proper checksum. Therefore, it
>>>>>>>>>> is necessary to have a correct virtio header even when checksum
>>>>>>>>>> offloading is disabled.
>>>>>>>>>>
>>>>>>>>>> A real TCP/UDP checksum will be computed and saved in the buffer when
>>>>>>>>>> checksum offloading is disabled. The virtio specification requires to
>>>>>>>>>> set the packet checksum stored in the buffer to the TCP/UDP pseudo
>>>>>>>>>> header when the VIRTIO_NET_HDR_F_NEEDS_CSUM bit is set so the bit must
>>>>>>>>>> be cleared in that case.
>>>>>>>>>>
>>>>>>>>>> The VIRTIO_NET_HDR_F_NEEDS_CSUM bit also tells to skip checksum
>>>>>>>>>> validation. Even if checksum offloading is disabled, it is desirable to
>>>>>>>>>> skip checksum validation because the checksum is always correct. Use the
>>>>>>>>>> VIRTIO_NET_HDR_F_DATA_VALID bit to claim the validity of the checksum.
>>>>>>>>>>
>>>>>>>>>> Fixes: ffbd2dbd8e64 ("e1000e: Perform software segmentation for loopback")
>>>>>>>>>> Buglink: https://issues.redhat.com/browse/RHEL-23067
>>>>>>>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>>>>>>>>> ---
>>>>>>>>>>       hw/net/net_tx_pkt.c | 3 +++
>>>>>>>>>>       1 file changed, 3 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/hw/net/net_tx_pkt.c b/hw/net/net_tx_pkt.c
>>>>>>>>>> index 2e5f58b3c9cc..c225cf706513 100644
>>>>>>>>>> --- a/hw/net/net_tx_pkt.c
>>>>>>>>>> +++ b/hw/net/net_tx_pkt.c
>>>>>>>>>> @@ -833,6 +833,9 @@ bool net_tx_pkt_send_custom(struct NetTxPkt *pkt, bool offload,
>>>>>>>>>>
>>>>>>>>>>           if (offload || gso_type == VIRTIO_NET_HDR_GSO_NONE) {
>>>>>>>>>>               if (!offload && pkt->virt_hdr.flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) {
>>>>>>>>>> +            pkt->virt_hdr.flags =
>>>>>>>>>> +                (pkt->virt_hdr.flags & ~VIRTIO_NET_HDR_F_NEEDS_CSUM) |
>>>>>>>>>> +                VIRTIO_NET_HDR_F_DATA_VALID;
>>>>>>>>>
>>>>>>>>> Why VIRTIO_NET_HDR_F_DATA_VALID is used in TX path?
>>>>>>>>
>>>>>>>> On igb, a packet sent from a PCI function may be routed to another
>>>>>>>> function. The virtio header updated here will be directly provided to
>>>>>>>> the RX path in such a case.
>>>>>>>
>>>>>>> But I meant for example net_tx_pkt_send_custom() is used in
>>>>>>> e1000e_tx_pkt_send() which is the tx path on the host.
>>>>>>>
>>>>>>> VIRTIO_NET_HDR_F_DATA_VALID is not necessary in the tx path.
>>>>>>
>>>>>> igb passes igb_tx_pkt_vmdq_callback to net_tx_pkt_send_custom().
>>>>>> igb_tx_pkt_vmdq_callback() passes the packet to its rx path for loopback.
>>>>>>
>>>>>
>>>>> You are right, how about igb_tx_pkt_vmdq_callback()?
>>>>>
>>>>> We probably need to tweak the name if it is only used in rx path.
>>>>
>>>> igb_tx_pkt_vmdq_callback() itself is part of the tx path of a PCI
>>>> function, and invokes the rx path of another PCI function in case of
>>>> loopback, or triggers the transmission to the external peer.
>>>
>>> Right, so if it's an external TX, VIRTIO_NET_HDR_F_DATA_VALID may not
>>> work there.
>>
>> It should be fine since it's just a hint.
> 
> It is not defined in the spec AFAIK. So we should try our best to avoid that.
> 
> For example vnet header might be hardened by failing a TX packet with
> that by kernel
> 
> I would bother now than bother it in the future for safety if it's not too hard.

I checked the specification and found:
https://docs.oasis-open.org/virtio/virtio/v1.2/csd01/virtio-v1.2-csd01.html#x1-2310001
 > 5.1.6.2.1 Driver Requirements: Packet Transmission

 > The driver MUST NOT set the VIRTIO_NET_HDR_F_DATA_VALID and
 > VIRTIO_NET_HDR_F_RSC_INFO bits in flags.

So it's explicitly prohibited. I'll send v2, which removes 
VIRTIO_NET_HDR_F_DATA_VALID.

Regards,
Akihiko Odaki

> 
> Thanks
> 
> 
> 
> Thanks
> 
>>
>> Regards,
>> Akihiko Odaki
>>
>>>
>>> Thanks
>>>
>>>>
>>>> Regards,
>>>> Akihiko Odaki
>>>>
>>>>>
>>>>> Thanks
>>>>>
>>>>>> Regards,
>>>>>> Akihiko Odaki
>>>>>>
>>>>>>>
>>>>>>> Thanks
>>>>>>>
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Akihiko Odaki
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks
>>>>>>>>>
>>>>>>>>>>                   net_tx_pkt_do_sw_csum(pkt, &pkt->vec[NET_TX_PKT_L2HDR_FRAG],
>>>>>>>>>>                                         pkt->payload_frags + NET_TX_PKT_PL_START_FRAG - 1,
>>>>>>>>>>                                         pkt->payload_len);
>>>>>>>>>>
>>>>>>>>>> ---
>>>>>>>>>> base-commit: ba49d760eb04630e7b15f423ebecf6c871b8f77b
>>>>>>>>>> change-id: 20240324-tx-c57d3c22ad73
>>>>>>>>>>
>>>>>>>>>> Best regards,
>>>>>>>>>> --
>>>>>>>>>> Akihiko Odaki <akihiko.odaki@daynix.com>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>