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

[Willem de Bruijn]

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.

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.