Re: [PATCH net-next v2] af-packet: new flag to indicate all csums are good

[Victor Julien]

On 02-06-2020 21:38, Willem de Bruijn wrote:
> On Tue, Jun 2, 2020 at 3:22 PM Victor Julien <victor@xxxxxxxxxxxx> wrote:
>>
>> On 02-06-2020 21:03, Willem de Bruijn wrote:
>>> On Tue, Jun 2, 2020 at 2:31 PM Victor Julien <victor@xxxxxxxxxxxx> wrote:
>>>> On 02-06-2020 19:37, Willem de Bruijn wrote:
>>>>> On Tue, Jun 2, 2020 at 1:03 PM Victor Julien <victor@xxxxxxxxxxxx> wrote:
>>>>>>
>>>>>> On 02-06-2020 16:29, Willem de Bruijn wrote:
>>>>>>> On Tue, Jun 2, 2020 at 4:05 AM Victor Julien <victor@xxxxxxxxxxxx> 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.

So if I understand you correctly the key may be in the call to
`skb_csum_unnecessary`:

That reads:

static inline int skb_csum_unnecessary(const struct sk_buff *skb)
{
        return ((skb->ip_summed == CHECKSUM_UNNECESSARY) ||
                skb->csum_valid ||
                (skb->ip_summed == CHECKSUM_PARTIAL &&
                 skb_checksum_start_offset(skb) >= 0));
}

But really only the first 2 conditions are reachable, as we already know
skb->ip_summed is not CHECKSUM_PARTIAL when we call it.

So our unmodified check is:

        else if (skb->pkt_type != PACKET_OUTGOING &&
                (skb->ip_summed == CHECKSUM_COMPLETE ||
		 skb->ip_summed == CHECKSUM_UNNECESSARY ||
		 skb->csum_valid))

Should this become something like:

        else if (skb->pkt_type != PACKET_OUTGOING &&
                (skb->ip_summed == CHECKSUM_COMPLETE &&
                 skb->csum_valid) ||
		 skb->ip_summed == CHECKSUM_UNNECESSARY)

Is this what you had in mind?

-- 
---------------------------------------------
Victor Julien
http://www.inliniac.net/
PGP: http://www.inliniac.net/victorjulien.asc
---------------------------------------------