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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 2 Jun 2020 13:37:01 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Victor Julien <victor@...iniac.net>
Cc:     Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        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 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.

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