Message-ID: <CA+FuTSdmtC4+0cnC2K1gwRLksXgb4hffUpyRbHjjGZbOJOfL0w@mail.gmail.com>
Date:   Tue, 2 Jun 2020 15:38:23 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Victor Julien <victor@...iniac.net>
Cc:     Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        Network Development <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Jonathan Corbet <corbet@....net>,
        Eric Dumazet <edumazet@...gle.com>,
        Mao Wenan <maowenan@...wei.com>, Arnd Bergmann <arnd@...db.de>,
        Neil Horman <nhorman@...driver.com>, linux-doc@...r.kernel.org,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Alexander Drozdov <al.drozdov@...il.com>,
        Tom Herbert <tom@...bertland.com>
Subject: Re: [PATCH net-next v2] af-packet: new flag to indicate all csums are good

On Tue, Jun 2, 2020 at 3:22 PM Victor Julien <victor@...iniac.net> wrote:
>
> On 02-06-2020 21:03, Willem de Bruijn wrote:
> > On Tue, Jun 2, 2020 at 2:31 PM Victor Julien <victor@...iniac.net> wrote:
> >> On 02-06-2020 19:37, Willem de Bruijn wrote:
> >>> On Tue, Jun 2, 2020 at 1:03 PM Victor Julien <victor@...iniac.net> wrote:
> >>>>
> >>>> On 02-06-2020 16:29, Willem de Bruijn wrote:
> >>>>> On Tue, Jun 2, 2020 at 4:05 AM Victor Julien <victor@...iniac.net> wrote:
> >>>>>>
> >>>>>> Introduce a new flag (TP_STATUS_CSUM_UNNECESSARY) to indicate
> >>>>>> that the driver has completely validated the checksums in the packet.
> >>>>>>
> >>>>>> The TP_STATUS_CSUM_UNNECESSARY flag differs from TP_STATUS_CSUM_VALID
> >>>>>> in that the new flag will only be set if all the layers are valid,
> >>>>>> while TP_STATUS_CSUM_VALID is set as well if only the IP layer is valid.
> >>>>>
> >>>>> transport, not ip checksum.
> >>>>
> >>>> Allow me a n00b question: what does transport refer to here? Things like
> >>>> ethernet? It isn't clear to me from the doc.
> >>>
> >>> The TCP/UDP/.. transport protocol checksum.
> >>
> >> Hmm that is what I thought originally, but then it didn't seem to work.
> >> Hence my patch.
> >>
> >> However I just redid my testing. I took the example tpacketv3 program
> >> and added the status flag checks to the 'display()' func:
> >>
> >>                 if (ppd->tp_status & TP_STATUS_CSUM_VALID) {
> >>                         printf("TP_STATUS_CSUM_VALID, ");
> >>                 }
> >>                 if (ppd->tp_status & (1<<8)) {
> >>                         printf("TP_STATUS_CSUM_UNNECESSARY, ");
> >>
> >>                 }
> >>
> >> Then using scapy sent some packets in 2 variants:
> >> - default (good csums)
> >> - deliberately bad csums
> >> (then also added a few things like ip6 over ip)
> >>
> >>
> >> srp1(Ether()/IP(src="1.2.3.4", dst="5.6.7.8")/IPv6()/TCP(),
> >> iface="enp1s0") // good csums
> >>
> >> srp1(Ether()/IP(src="1.2.3.4", dst="5.6.7.8")/IPv6()/TCP(chksum=1),
> >> iface="enp1s0") //bad tcp
> >
> > Is this a test between two machines? What is the device driver of the
> > machine receiving and printing the packet? It would be helpful to know
> > whether this uses CHECKSUM_COMPLETE or CHECKSUM_UNNECESSARY.
>
> Yes 2 machines, or actually 2 machines and a VM. The receiving Linux
> sits in a kvm vm with network pass through and uses the virtio driver
> (host uses e1000e). Based on a quick 'git grep CHECKSUM_UNNECESSARY'
> virtio seems to support that.
>
> I've done some more tests. In a pcap replay that I know contains packet
> with bad TCP csums (but good IP csums for those pkts), to a physical
> host running Ubuntu Linux kernel 5.3:
>
> - receiver uses nfp (netronome) driver: TP_STATUS_CSUM_VALID set for
> every packet, including the bad TCP ones
> - receiver uses ixgbe driver: TP_STATUS_CSUM_VALID not set for the bad
> packets.

Great. Thanks a lot for running all these experiments.

We might have to drop the TP_STATUS_CSUM_VALID with CHECKSUM_COMPLETE
unless skb->csum_valid.

For packets with multiple transport layer checksums,
CHECKSUM_UNNECESSARY should mean that all have been verified.

I believe that in the case of multiple transport headers, csum_valid
similarly ensures all checksums up to csum_start are valid. Will need
to double check.

If so, there probably is no need for a separate new TP_STATUS.
TP_STATUS_CSUM_VALID is reported only when all checksums are valid.

> Again purely based on 'git grep' it seems nfp does not support
> UNNECESSARY, while ixgbe does.
>
> (my original testing was with the nfp only, so now I at least understand
> my original thinking)
>
>
> >>
> >> 1.2.3.4 -> 5.6.7.8, TP_STATUS_CSUM_VALID, TP_STATUS_CSUM_UNNECESSARY,
> >> rxhash: 0x81ad5744
> >> 1.2.3.4 -> 5.6.7.8, rxhash: 0x81ad5744
> >>
> >> So this suggests that what you're saying is correct, that it sets
> >> TP_STATUS_CSUM_VALID if the TCP/UDP csum (and IPv4 csum) is valid, and
> >> does not set it when either of them are invalid.
> >
> > That's not exactly what I said. It looks to me that a device that sets
> > CHECKSUM_COMPLETE will return TP_STATUS_CSUM_VALID from
> > __netif_receive_skb_core even if the TCP checksum turns out to be bad.
> > If a driver would insert such packets into the stack, that is.
>
> Ok, this might be confirmed by my nfp vs virtio/ixgbe observations
> mentioned above.
>
>
> >> I'll also re-evaluate things in Suricata.
> >>
> >>
> >> One thing I wonder if what this "at least" from the 682f048bd494 commit
> >> message means:
> >>
> >> "Introduce TP_STATUS_CSUM_VALID tp_status flag to tell the
> >>  af_packet user that at least the transport header checksum
> >>  has been already validated."
> >>
> >> For TCP/UDP there wouldn't be a higher layer with csums, right? And
> >> based on my testing it seems lower levels (at least IP) is also
> >> included. Or would that perhaps refer to something like VXLAN or Geneve
> >> over UDP? That the csums of packets on top of those layers aren't
> >> (necessarily) considered?
> >
> > The latter. All these checksums are about transport layer checksums
> > (the ip header checksum is cheap to compute). Multiple checksums
> > refers to packets encapsulated in other protocols with checksum, such
> > as GRE or UDP based like Geneve.
>
> If nothing else comes from this I'll at least propose doc patch to
> clarify this for ppl like myself.
>
> Thanks,
> Victor
>
> --
> ---------------------------------------------
> Victor Julien
> http://www.inliniac.net/
> PGP: http://www.inliniac.net/victorjulien.asc
> ---------------------------------------------
>

Powered by blists - more mailing lists