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

[Victor Julien]

On 02-06-2020 22:18, Willem de Bruijn wrote:
> On Tue, Jun 2, 2020 at 4:05 PM Victor Julien <victor@xxxxxxxxxxxx> wrote:
>>
>> 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
> 
> .. from this codepath. That function is called in other codepaths as well.
> 
>> , 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?
> 
> I don't suggest modifying skb_csum_unnecessary probably. Certainly not
> until I've looked at all other callers of it.

Yeah didn't read it that way. Just wanted to make clear what the actual
check for afpacket is (for my own understanding mostly)

> But in case of packet sockets, yes, adding that csum_valid check is my
> first rough approximation.
> 
> That said, first let's give others more familiar with
> TP_STATUS_CSUM_VALID some time to comment.
> 

Thanks a lot for your comments.

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