netdev - Re: [PATCH bpf-next v4 02/11] 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]

Message-ID: <20231020174940.gjubehkouns2hmgz@MacBook-Pro-49.local>
Date: Fri, 20 Oct 2023 10:49:40 -0700
From: Alexei Starovoitov <alexei.starovoitov@...il.com>
To: Stanislav Fomichev <sdf@...gle.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,
	yoong.siang.song@...el.com, netdev@...r.kernel.org,
	xdp-hints@...-project.net
Subject: Re: [PATCH bpf-next v4 02/11] xsk: Add TX timestamp and TX checksum
 offload support

On Thu, Oct 19, 2023 at 10:49:35AM -0700, Stanislav Fomichev wrote:
> diff --git a/include/uapi/linux/if_xdp.h b/include/uapi/linux/if_xdp.h
> index 2ecf79282c26..ecfd67988283 100644
> --- a/include/uapi/linux/if_xdp.h
> +++ b/include/uapi/linux/if_xdp.h
> @@ -106,6 +106,43 @@ struct xdp_options {
>  #define XSK_UNALIGNED_BUF_ADDR_MASK \
>  	((1ULL << XSK_UNALIGNED_BUF_OFFSET_SHIFT) - 1)
>  
> +/* Request transmit timestamp. Upon completion, put it into tx_timestamp
> + * field of struct xsk_tx_metadata.
> + */
> +#define XDP_TX_METADATA_TIMESTAMP		(1 << 0)
> +
> +/* 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)
> +
> +struct xsk_tx_metadata {
> +	union {
> +		struct {
> +			__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;
> +		};
> +
> +		struct {
> +			/* XDP_TX_METADATA_TIMESTAMP */
> +			__u64 tx_timestamp;
> +		} completion;
> +	};
> +};

Could you add a comment to above union that csum fields are consumed by the driver
before it xmits the packet while timestamp is filled during xmit, so union
doesn't prevent using both features simultaneously.
It's clear from the example, but not obvious from uapi and the doc in patch 11
doesn't clarify it either.

Also please add a name to csum part of the union like you did for completion.
We've learned it the hard way with bpf_attr. All anon structs better have field name
within a union. Helps extensibility (avoid conflicts) in the long term.

Other than this the patch set looks great to me.
With Saeed and Magnus acks we can take it in.