| 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
| ||
|
Message-ID: <ZLo6Stj4HofGOcGO@lincoln>
Date: Fri, 21 Jul 2023 08:01:18 +0000
From: "Zaremba, Larysa" <larysa.zaremba@...el.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>
CC: "bpf@...r.kernel.org" <bpf@...r.kernel.org>, "ast@...nel.org"
<ast@...nel.org>, "daniel@...earbox.net" <daniel@...earbox.net>,
"andrii@...nel.org" <andrii@...nel.org>, "martin.lau@...ux.dev"
<martin.lau@...ux.dev>, "song@...nel.org" <song@...nel.org>, "yhs@...com"
<yhs@...com>, "john.fastabend@...il.com" <john.fastabend@...il.com>,
"kpsingh@...nel.org" <kpsingh@...nel.org>, "sdf@...gle.com" <sdf@...gle.com>,
"haoluo@...gle.com" <haoluo@...gle.com>, "jolsa@...nel.org"
<jolsa@...nel.org>, David Ahern <dsahern@...il.com>, Jakub Kicinski
<kuba@...nel.org>, Willem de Bruijn <willemb@...gle.com>, "Brouer, Jesper"
<brouer@...hat.com>, "Burakov, Anatoly" <anatoly.burakov@...el.com>,
"Lobakin, Aleksander" <aleksander.lobakin@...el.com>, Magnus Karlsson
<magnus.karlsson@...il.com>, "Tahhan, Maryam" <mtahhan@...hat.com>,
"xdp-hints@...-project.net" <xdp-hints@...-project.net>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH bpf-next v3 12/21] xdp: Add checksum hint
On Thu, Jul 20, 2023 at 06:27:41PM -0400, Willem de Bruijn wrote:
> Zaremba, Larysa wrote:
> > On Thu, Jul 20, 2023 at 09:55:16AM -0400, Willem de Bruijn wrote:
> > > Zaremba, Larysa wrote:
> > > > On Thu, Jul 20, 2023 at 09:57:05AM +0000, Zaremba, Larysa wrote:
> > > > > On Wed, Jul 19, 2023 at 05:42:04PM -0400, Willem de Bruijn wrote:
> > > > > > Larysa Zaremba wrote:
> > > > > > > Implement functionality that enables drivers to expose to XDP code checksum
> > > > > > > information that consists of:
> > > > > > >
> > > > > > > - Checksum status - bitfield that consists of
> > > > > > > - number of consecutive validated checksums. This is almost the same as
> > > > > > > csum_level in skb, but starts with 1. Enum names for those bits still
> > > > > > > use checksum level concept, so it is less confusing for driver
> > > > > > > developers.
> > > > > > > - Is checksum partial? This bit cannot coexist with any other
> > > > > > > - Is there a complete checksum available?
> > > > > > > - Additional checksum data, a union of:
> > > > > > > - checksum start and offset, if checksum is partial
> > > > > > > - complete checksum, if available
> > > > > > >
> > > > > > > Signed-off-by: Larysa Zaremba <larysa.zaremba@...el.com>
> > > > > > > ---
> > > > > > > Documentation/networking/xdp-rx-metadata.rst | 3 ++
> > > > > > > include/linux/netdevice.h | 3 ++
> > > > > > > include/net/xdp.h | 46 ++++++++++++++++++++
> > > > > > > kernel/bpf/offload.c | 2 +
> > > > > > > net/core/xdp.c | 23 ++++++++++
> > > > > > > 5 files changed, 77 insertions(+)
> > > > > > >
> > > > > > > diff --git a/Documentation/networking/xdp-rx-metadata.rst b/Documentation/networking/xdp-rx-metadata.rst
> > > > > > > index ea6dd79a21d3..7f056a44f682 100644
> > > > > > > --- a/Documentation/networking/xdp-rx-metadata.rst
> > > > > > > +++ b/Documentation/networking/xdp-rx-metadata.rst
> > > > > > > @@ -26,6 +26,9 @@ metadata is supported, this set will grow:
> > > > > > > .. kernel-doc:: net/core/xdp.c
> > > > > > > :identifiers: bpf_xdp_metadata_rx_vlan_tag
> > > > > > >
> > > > > > > +.. kernel-doc:: net/core/xdp.c
> > > > > > > + :identifiers: bpf_xdp_metadata_rx_csum
> > > > > > > +
> > > > > > > An XDP program can use these kfuncs to read the metadata into stack
> > > > > > > variables for its own consumption. Or, to pass the metadata on to other
> > > > > > > consumers, an XDP program can store it into the metadata area carried
> > > > > > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > > > > > > index 1749f4f75c64..4f6da36ac123 100644
> > > > > > > --- a/include/linux/netdevice.h
> > > > > > > +++ b/include/linux/netdevice.h
> > > > > > > @@ -1660,6 +1660,9 @@ struct xdp_metadata_ops {
> > > > > > > enum xdp_rss_hash_type *rss_type);
> > > > > > > int (*xmo_rx_vlan_tag)(const struct xdp_md *ctx, u16 *vlan_tci,
> > > > > > > __be16 *vlan_proto);
> > > > > > > + int (*xmo_rx_csum)(const struct xdp_md *ctx,
> > > > > > > + enum xdp_csum_status *csum_status,
> > > > > > > + union xdp_csum_info *csum_info);
> > > > > > > };
> > > > > > >
> > > > > > > /**
> > > > > > > diff --git a/include/net/xdp.h b/include/net/xdp.h
> > > > > > > index 89c58f56ffc6..2b7a7d678ff4 100644
> > > > > > > --- a/include/net/xdp.h
> > > > > > > +++ b/include/net/xdp.h
> > > > > > > @@ -391,6 +391,8 @@ void xdp_attachment_setup(struct xdp_attachment_info *info,
> > > > > > > bpf_xdp_metadata_rx_hash) \
> > > > > > > XDP_METADATA_KFUNC(XDP_METADATA_KFUNC_RX_VLAN_TAG, \
> > > > > > > bpf_xdp_metadata_rx_vlan_tag) \
> > > > > > > + XDP_METADATA_KFUNC(XDP_METADATA_KFUNC_RX_CSUM, \
> > > > > > > + bpf_xdp_metadata_rx_csum) \
> > > > > > >
> > > > > > > enum {
> > > > > > > #define XDP_METADATA_KFUNC(name, _) name,
> > > > > > > @@ -448,6 +450,50 @@ enum xdp_rss_hash_type {
> > > > > > > XDP_RSS_TYPE_L4_IPV6_SCTP_EX = XDP_RSS_TYPE_L4_IPV6_SCTP | XDP_RSS_L3_DYNHDR,
> > > > > > > };
> > > > > > >
> > > > > > > +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, calculated over the whole packet.
> > > > > > > + * Available, if ``status & XDP_CHECKSUM_COMPLETE``.
> > > > > > > + */
> > > > > > > + u32 checksum;
> > > > > > > +};
> > > > > > > +
> > > > > > > +enum xdp_csum_status {
> > > > > > > + /* HW had parsed several transport headers and validated their
> > > > > > > + * checksums, same as ``CHECKSUM_UNNECESSARY`` in ``sk_buff``.
> > > > > > > + * 3 least significat bytes contain number of consecutive checksum,
> > > > > >
> > > > > > typo: significant
> > > > > >
> > > > > > (I did not scan for typos, just came across this when trying to understand
> > > > > > the skb->csum_level + 1 trick. Probably good to run a spell check).
> > > > > >
> > > >
> > > > Oh, and about skb->csum_level + 1, maybe this way it would be more
> > > > understandable: XDP_CHECKSUM_VALID_LVL0 + skb->csum_level?
> > >
> > > Agreed, that would help document the intent.
> > >
> > > > Using number of valid checksums (starts with 1) instead of checksum level
> > > > (starts with 0) is a debatable decision, but I have decided to go with it under
> > > > 2 assumptions:
> > > >
> > > > - the only reason checksum level in skb starts with 0 is to use less bits
> > > > - checksum number would be more intuitive for XDP/AF_XDP application developers
> > > >
> > > > I encourage everyone to share their opinion on that.
> > >
> > > I assumed this offset by one was because csum_status zero implicitly
> > > meant XDP_CHECKSUM_NONE. Is that not correct? That should probably
> > > get an explicit name.
> > >
> >
> > Well, I was not sure, whether I should add XDP_CHECKSUM_NONE, because it would
> > be equal to returning -ENODATA from kfunc, but after giving it some thought now,
> > it is worth to have XDP_CHECKSUM_NONE for packets that have no checksum to
> > check, like for hash there is XDP_RSS_TYPE_L2.
>
> On receive, CHECKSUM_NONE means that the packet has not been checked, not
> necessarily that it has no checksum. Perhaps the device was unable to
> parse the protocol.
>
> (on transmit, it conveys that a transmit checksum is not required.)
Oh, my bad, I have re-read the docs and for packets without checksum,
CHECKSUM_UNNECESSARY instead conveys "CRC in OK". In such case,
XDP_CHECKSUM_NONE becomes a full equivalent of returning -ENODATA from kfunc, so
I do not think XDP_CHECKSUM_NONE enum is worth including, because it coud lead
to new people writing programs in such way:
if (bpf_xdp_metadata_rx_csum(ctx, &csum_status, &rx_csum_info))
fallback();
if (csum_status == XDP_CHECKSUM_NONE)
fallback();
[...]
Powered by blists - more mailing lists