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

From: Jason Wang <jasowang@redhat.com>
To: Akihiko Odaki <akihiko.odaki@daynix.com>
Cc: Dmitry Fleytman <dmitry.fleytman@gmail.com>, qemu-devel@nongnu.org
Subject: Re: [PATCH] hw/net/net_tx_pkt: Fix virtio header without checksum offloading
Date: Wed, 27 Mar 2024 11:24:29 +0800	[thread overview]
Message-ID: <CACGkMEuR=Q1cUitrsxQwhV16jJnZiV5bzs5PHXYt=mpmBKaddA@mail.gmail.com> (raw)
In-Reply-To: <72ccd5e9-8e9a-4d01-bbdc-94c2dfaf6468@daynix.com>

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.

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>
> >>>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>>
> >>
> >
>