Re: [virtio-comment] Re: [PATCH v5] virtio-net: device does not deliver partially checksummed packet and may validate the checksum

[Jason Wang <jasowang@redhat.com>]

On Wed, Dec 20, 2023 at 2:41âAM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Dec 19, 2023 at 03:53:14PM +0800, Jason Wang wrote:
> > On Mon, Dec 18, 2023 at 12:54âPM Heng Qi <hengqi@linux.alibaba.com> wrote:
> > >
> > >
> > >
> > > å 2023/12/18 äå11:10, Jason Wang åé:
> > > > On Fri, Dec 15, 2023 at 5:51âPM Heng Qi <hengqi@linux.alibaba.com> wrote:
> > > >> Hi all!
> > > >>
> > > >> I would like to ask if anyone has any comments on this version, if so
> > > >> please let me know!
> > > >> If not, I will collect Michael's comments and publish a new version next
> > > >> Monday.
> > > > I have a dumb question. (And sorry if I asked it before)
> > > >
> > > > Looking at the spec and code. It looks to me DATA_VALID could be set
> > > > without GUEST_CSUM.
> > >
> > > I don't see that in the spec.
> > > Am I missing something? [1][2]
> > >
> > > [1] If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated, the
> > > VIRTIO_NET_HDR_F_DATA_VALID bit in flags can be set: if so, device has
> > > validated the packet checksum. In case of multiple encapsulated
> > > protocols, one level of checksums has been validated.
> > > Additionally, VIRTIO_NET_F_GUEST_CSUM, TSO4, TSO6, UDP and ECN features
> > > *enable receive checksum*, large receive offload and ECN support which
> > > are the input equivalents of the transmit checksum, transmit
> > > segmentation *offloading* and ECN features, as described in 5.1.6.2.
> > >
> > > [2] If VIRTIO_NET_F_GUEST_CSUM is not negotiated, the device *MUST set
> > > flags to zero* and SHOULD supply a fully checksummed packet to the driver.
> >
> > So this is kind of ambiguous and seems not what I wanted when I wrote
> > the code for DATA_VALID in 2011.
> >
> > NEEDS_CSUM maps to CHECKSUM_PARTIAL which means the packet checksum is
> > correct. So spec had
> >
> > """
> > If neither VIRTIO_NET_HDR_F_NEEDS_CSUM nor VIRTIO_NET_HDR_F_DATA_VALID
> > is set, the driver MUST NOT rely on the packet checksum being correct.
> > """
> >
> > For DATA_VALID, it maps to CHECKSUM_UNNECESSARY which is mutually
> > exclusive with CHECKSUM_PARTAIL. And this is what Linux did right now:
> >
> > For tun_put_user():
> >
> >         if (skb->ip_summed == CHECKSUM_PARTIAL) {
> >                 ...
> >         } else if (has_data_valid &&
> >                    skb->ip_summed == CHECKSUM_UNNECESSARY) {
> >                    hdr->flags = VIRTIO_NET_HDR_F_DATA_VALID;
> >         } /* else everything is zero */
> >
> > This CHECKSUM_UNNECESSARY will work even if GUEST_CSUM is disabled if
> > I was not wrong.
> >
> > And in receive_buf():
> >
> >         if (hdr->hdr.flags & VIRTIO_NET_HDR_F_DATA_VALID)
> >                 skb->ip_summed = CHECKSUM_UNNECESSARY;
> >
> > I think we can fix this by safely removing "*MUST set flags to zero*"
> > in [2] from the spec.
> >
> > Thanks
>
> I don't get why you want to remove this. I think this text refers to
> when no offloads are negotiated. Why would device then set any flags?

Current Linux can produce DATA_VALID without GUEST_CSUM. So it just
wants to align with that.

Driver can just not set CHECKSUM_UNNECESSARY if the rx csum is disabled.

Thanks