Message-ID: <CA+FuTSci29=W89CLweZcW=RTKwEXpUdPjsLGTB95iSNcnpU_Lw@mail.gmail.com>
Date:   Tue, 2 Jun 2020 15:03:55 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Victor Julien <victor@...iniac.net>
Cc:     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 2:31 PM Victor Julien <victor@...iniac.net> wrote:
>
> Hi Willem,
>
> 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.

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

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

>
> Thanks,
> Victor
>
>
> >> (happy to follow up with a patch to clarify the doc when I understand
> >> things better)
> >>
> >>> But as I understand it drivers set CHECKSUM_COMPLETE if they fill in
> >>> skb->csum over the full length of the packet. This does not
> >>> necessarily imply that any of the checksum fields in the packet are
> >>> valid yet (see also skb->csum_valid). Protocol code later compares
> >>> checksum fields against this using __skb_checksum_validate_complete and friends.
> >>>
> >>> But packet sockets may be called before any of this, however. So I wonder
> >>> how valid the checksum really is right now when setting TP_STATUS_CSUM_VALID.
> >>> I assume it's correct, but don't fully understand where the validation
> >>> has taken place..
> >>
> >> I guess I'm more confused now about what TP_STATUS_CSUM_VALID actually
> >> means. It sounds almost like the opposite of TP_STATUS_CSUMNOTREADY, but
> >> I'm not sure I understand what the value would be.
> >>
> >> It would be great if someone could help clear this up. Everything I
> >> thought I knew/understood so far has been proven wrong, so I'm not too
> >> confident about my patch anymore...
> >
> > Agreed that we should clear this up.
> >
> >>> Similar to commit 682f048bd494 ("af_packet: pass checksum validation
> >>> status to the user"), please update tpacket_rcv and packet_rcv.
> >>
> >> Ah yes, good catch. Will add it there as well.
> >>
> >>> Note also that net-next is currently closed.
> >>
> >> Should I hold off on sending a v3 until it reopens?
> >
> > Yep, thanks. You can always check
> > http://vger.kernel.org/~davem/net-next.html when unsure.
> >
>
>
> --
> ---------------------------------------------
> Victor Julien
> http://www.inliniac.net/
> PGP: http://www.inliniac.net/victorjulien.asc
> ---------------------------------------------
>

Powered by blists - more mailing lists