netdev - Re: [RFC net-next v4 2/8] xsk: add TX timestamp and TX checksum offload support

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, 25 Jul 2023 16:48:20 -0700
From: Stanislav Fomichev <sdf@...gle.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: bpf@...r.kernel.org, ast@...nel.org, daniel@...earbox.net, 
	andrii@...nel.org, martin.lau@...ux.dev, song@...nel.org, yhs@...com, 
	john.fastabend@...il.com, kpsingh@...nel.org, haoluo@...gle.com, 
	jolsa@...nel.org, kuba@...nel.org, toke@...nel.org, willemb@...gle.com, 
	dsahern@...nel.org, magnus.karlsson@...el.com, bjorn@...nel.org, 
	maciej.fijalkowski@...el.com, hawk@...nel.org, netdev@...r.kernel.org, 
	xdp-hints@...-project.net
Subject: Re: [RFC net-next v4 2/8] xsk: add TX timestamp and TX checksum
 offload support

On 07/25, Willem de Bruijn wrote:
> Stanislav Fomichev wrote:
> > On 07/25, Willem de Bruijn wrote:
> > > Stanislav Fomichev wrote:
> > > > This change actually defines the (initial) metadata layout
> > > > that should be used by AF_XDP userspace (xsk_tx_metadata).
> > > > The first field is flags which requests appropriate offloads,
> > > > followed by the offload-specific fields. The supported per-device
> > > > offloads are exported via netlink (new xsk-flags).
> > > > 
> > > > The offloads themselves are still implemented in a bit of a
> > > > framework-y fashion that's left from my initial kfunc attempt.
> > > > I'm introducing new xsk_tx_metadata_ops which drivers are
> > > > supposed to implement. The drivers are also supposed
> > > > to call xsk_tx_metadata_request/xsk_tx_metadata_complete in
> > > > the right places. Since xsk_tx_metadata_{request,_complete}
> > > > are static inline, we don't incur any extra overhead doing
> > > > indirect calls.
> > > > 
> > > > The benefit of this scheme is as follows:
> > > > - keeps all metadata layout parsing away from driver code
> > > > - makes it easy to grep and see which drivers implement what
> > > > - don't need any extra flags to maintain to keep track of that
> > > >   offloads are implemented; if the callback is implemented - the offload
> > > >   is supported (used by netlink reporting code)
> > > > 
> > > > Two offloads are defined right now:
> > > > 1. XDP_TX_METADATA_CHECKSUM: skb-style csum_start+csum_offset
> > > > 2. XDP_TX_METADATA_TIMESTAMP: writes TX timestamp back into metadata
> > > >    area upon completion (tx_timestamp field)
> > > > 
> > > > The offloads are also implemented for copy mode:
> > > > 1. Extra XDP_TX_METADATA_CHECKSUM_SW to trigger skb_checksum_help; this
> > > >    might be useful as a reference implementation and for testing
> > > > 2. XDP_TX_METADATA_TIMESTAMP writes SW timestamp from the skb
> > > >    destructor (note I'm reusing hwtstamps to pass metadata pointer)
> > > > 
> > > > The struct is forward-compatible and can be extended in the future
> > > > by appending more fields.
> > > > 
> > > > Signed-off-by: Stanislav Fomichev <sdf@...gle.com>
> 
> > > > +/* Request transmit checksum offload. Checksum start position and offset
> > > > + * are communicated via csum_start and csum_offset fields of struct
> > > > + * xsk_tx_metadata.
> > > > + */
> > > > +#define XDP_TX_METADATA_CHECKSUM		(1 << 1)
> > > > +
> > > > +/* Force checksum calculation in software. Can be used for testing or
> > > > + * working around potential HW issues. This option causes performance
> > > > + * degradation and only works in XDP_COPY mode.
> > > > + */
> > > > +#define XDP_TX_METADATA_CHECKSUM_SW		(1 << 2)
> > > 
> > > Not sure how useful this is, especially if only for copy mode.
> > 
> > Seems useful at least as a reference implementation? But I'm happy
> > to drop. It's used only in the tests for now. I was using it to
> > verify csum_offset/start field values.
> 
> If testing over veth, does anything even look at the checksum?

My receiver in the xdp_metadata test looks at it and compares to the
fixed (verified) value:

	ASSERT_EQ(udph->check, 0x1c72, "csum");

The packet is always the same (and macs are fixed), so we are able
to do that.
 
> > > > +struct xsk_tx_metadata {
> > > > +	__u32 flags;
> > > > +
> > > > +	/* XDP_TX_METADATA_CHECKSUM */
> > > > +
> > > > +	/* Offset from desc->addr where checksumming should start. */
> > > > +	__u16 csum_start;
> > > > +	/* Offset from csum_start where checksum should be stored. */
> > > > +	__u16 csum_offset;
> > > > +
> > > > +	/* XDP_TX_METADATA_TIMESTAMP */
> > > > +
> > > > +	__u64 tx_timestamp;
> > > > +};
> > > 
> > > Is this structure easily extensible for future offloads,
> > > such as USO?
> > 
> > We can append more field. What do we need for USO? Something akin
> > to gso_size/gso_segs/gso_type ?
> 
> Yes, a bit to set the feature (gso_type) and a field to store the
> segment size (gso_size).
> 
> Pacing offload is the other feature that comes to mind. That could
> conceivably use the tx_timestamp field.

Right, so we can append to this struct and add more XDP_TX_METADATA_$(FLAG)s
to signal various features. Jakub mentioned that it might be handy
to pass l2_offset/l3_offset/l4_offset, but I'm not sure whether
he was talking about xSO offloads or something else.