Message-ID: <6a3dcce9-4635-28e9-d78e-1c7f1f7874da@inliniac.net>
Date:   Tue, 2 Jun 2020 21:22:11 +0200
From:   Victor Julien <victor@...iniac.net>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>
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 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.

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