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

[Victor Julien]

Hi Willem,

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

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.

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?

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