netdev - Re: [PATCH bpf-next v4 12/21] xdp: Add checksum hint

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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAADnVQ+vn0=1UT5_c628ovq+LzfrNFf0MxmZn++NqeUFJ-ykQw@mail.gmail.com>
Date: Sat, 29 Jul 2023 11:04:16 -0700
From: Alexei Starovoitov <alexei.starovoitov@...il.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: Larysa Zaremba <larysa.zaremba@...el.com>, bpf <bpf@...r.kernel.org>, 
	Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>, 
	Andrii Nakryiko <andrii@...nel.org>, Martin KaFai Lau <martin.lau@...ux.dev>, Song Liu <song@...nel.org>, 
	Yonghong Song <yhs@...com>, John Fastabend <john.fastabend@...il.com>, KP Singh <kpsingh@...nel.org>, 
	Stanislav Fomichev <sdf@...gle.com>, Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>, 
	David Ahern <dsahern@...il.com>, Jakub Kicinski <kuba@...nel.org>, 
	Willem de Bruijn <willemb@...gle.com>, Jesper Dangaard Brouer <brouer@...hat.com>, 
	Anatoly Burakov <anatoly.burakov@...el.com>, Alexander Lobakin <alexandr.lobakin@...el.com>, 
	Magnus Karlsson <magnus.karlsson@...il.com>, Maryam Tahhan <mtahhan@...hat.com>, 
	xdp-hints@...-project.net, Network Development <netdev@...r.kernel.org>, 
	Simon Horman <simon.horman@...igine.com>
Subject: Re: [PATCH bpf-next v4 12/21] xdp: Add checksum hint

On Sat, Jul 29, 2023 at 9:15 AM Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
>
> Alexei Starovoitov wrote:
> > On Fri, Jul 28, 2023 at 07:39:14PM +0200, Larysa Zaremba wrote:
> > >
> > > +union xdp_csum_info {
> > > +   /* Checksum referred to by ``csum_start + csum_offset`` is considered
> > > +    * valid, but was never calculated, TX device has to do this,
> > > +    * starting from csum_start packet byte.
> > > +    * Any preceding checksums are also considered valid.
> > > +    * Available, if ``status == XDP_CHECKSUM_PARTIAL``.
> > > +    */
> > > +   struct {
> > > +           u16 csum_start;
> > > +           u16 csum_offset;
> > > +   };
> > > +
> >
> > CHECKSUM_PARTIAL makes sense on TX, but this RX. I don't see in the above.
>
> It can be observed on RX when packets are looped.
>
> This may be observed even in XDP on veth.

veth and XDP is a broken combination. GSO packets coming out of containers
cannot be parsed properly by XDP.
It was added mainly for testing. Just like "generic XDP".
bpf progs at skb layer is much better fit for veth.

> > > +   /* Checksum, calculated over the whole packet.
> > > +    * Available, if ``status & XDP_CHECKSUM_COMPLETE``.
> > > +    */
> > > +   u32 checksum;
> >
> > imo XDP RX should only support XDP_CHECKSUM_COMPLETE with u32 checksum
> > or XDP_CHECKSUM_UNNECESSARY.
> >
> > > +};
> > > +
> > > +enum xdp_csum_status {
> > > +   /* HW had parsed several transport headers and validated their
> > > +    * checksums, same as ``CHECKSUM_UNNECESSARY`` in ``sk_buff``.
> > > +    * 3 least significant bytes contain number of consecutive checksums,
> > > +    * starting with the outermost, reported by hardware as valid.
> > > +    * ``sk_buff`` checksum level (``csum_level``) notation is provided
> > > +    * for driver developers.
> > > +    */
> > > +   XDP_CHECKSUM_VALID_LVL0         = 1,    /* 1 outermost checksum */
> > > +   XDP_CHECKSUM_VALID_LVL1         = 2,    /* 2 outermost checksums */
> > > +   XDP_CHECKSUM_VALID_LVL2         = 3,    /* 3 outermost checksums */
> > > +   XDP_CHECKSUM_VALID_LVL3         = 4,    /* 4 outermost checksums */
> > > +   XDP_CHECKSUM_VALID_NUM_MASK     = GENMASK(2, 0),
> > > +   XDP_CHECKSUM_VALID              = XDP_CHECKSUM_VALID_NUM_MASK,
> >
> > I don't see what bpf prog suppose to do with these levels.
> > The driver should pick between 3:
> > XDP_CHECKSUM_UNNECESSARY, XDP_CHECKSUM_COMPLETE, XDP_CHECKSUM_NONE.
> >
> > No levels and no anything partial. please.
>
> This levels business is an unfortunate side effect of
> CHECKSUM_UNNECESSARY. For a packet with multiple checksum fields, what
> does the boolean actually mean? With these levels, at least that is
> well defined: the first N checksum fields.

If I understand this correctly this is intel specific feature that
other NICs don't have. skb layer also doesn't have such concept.
The driver should say CHECKSUM_UNNECESSARY when it's sure
or don't pretend that it checks the checksum and just say NONE.