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]

Message-ID: <ZMAiYibjYzVTBjEF@corigine.com>
Date: Tue, 25 Jul 2023 21:28:34 +0200
From: Simon Horman <simon.horman@...igine.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,
	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 Mon, Jul 24, 2023 at 04:59:51PM -0700, Stanislav Fomichev wrote:

...

Hi Stan,

> diff --git a/include/uapi/linux/netdev.h b/include/uapi/linux/netdev.h
> index bf71698a1e82..cf1e11c76339 100644
> --- a/include/uapi/linux/netdev.h
> +++ b/include/uapi/linux/netdev.h
> @@ -37,11 +37,26 @@ enum netdev_xdp_act {
>  	NETDEV_XDP_ACT_MASK = 127,
>  };
>  
> +/**
> + * enum netdev_xsk_flags
> + * @NETDEV_XSK_FLAGS_TX_TIMESTAMP: HW timestamping egress packets is supported
> + *   by the driver.
> + * @NETDEV_XSK_FLAGS_TX_CHECKSUM: L3 checksum HW offload is supported by the
> + *   driver.
> + */
> +enum netdev_xsk_flags {
> +	NETDEV_XSK_FLAGS_TX_TIMESTAMP = 1,
> +	NETDEV_XSK_FLAGS_TX_CHECKSUM = 2,
> +

I know that it isn't the practice in this file.
but adding the following makes kernel-doc happier
about NETDEV_XSK_FLAGS_MASK not being documented.

	/* private: */


> +	NETDEV_XSK_FLAGS_MASK = 3,
> +};
> +
>  enum {
>  	NETDEV_A_DEV_IFINDEX = 1,
>  	NETDEV_A_DEV_PAD,
>  	NETDEV_A_DEV_XDP_FEATURES,
>  	NETDEV_A_DEV_XDP_ZC_MAX_SEGS,
> +	NETDEV_A_DEV_XSK_FEATURES,
>  
>  	__NETDEV_A_DEV_MAX,
>  	NETDEV_A_DEV_MAX = (__NETDEV_A_DEV_MAX - 1)

...

> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c

...

> @@ -626,6 +635,7 @@ static struct sk_buff *xsk_build_skb_zerocopy(struct xdp_sock *xs,
>  static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
>  				     struct xdp_desc *desc)
>  {
> +	struct xsk_tx_metadata *meta = NULL;
>  	struct net_device *dev = xs->dev;
>  	struct sk_buff *skb = xs->skb;
>  	int err;
> @@ -678,12 +688,40 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
>  
>  			skb_add_rx_frag(skb, nr_frags, page, 0, len, 0);
>  		}
> +
> +		if (desc->options & XDP_TX_METADATA) {
> +			if (unlikely(xs->tx_metadata_len == 0)) {
> +				err = -EINVAL;
> +				goto free_err;
> +			}
> +
> +			meta = buffer - xs->tx_metadata_len;
> +
> +			if (meta->flags & XDP_TX_METADATA_CHECKSUM) {
> +				if (unlikely(meta->csum_start + meta->csum_offset +
> +					     sizeof(__sum16) > len)) {
> +					err = -EINVAL;
> +					goto free_err;
> +				}
> +
> +				skb->csum_start = hr + meta->csum_start;

hr seems to only be set - by earlier, existing code in this function -
if skb is NULL. Is it always safe to use it here?

Smatch flags hr as being potentially used uninitialised,
the above is my understanding of why it thinks that is so.

> +				skb->csum_offset = meta->csum_offset;
> +				skb->ip_summed = CHECKSUM_PARTIAL;
> +
> +				if (unlikely(meta->flags & XDP_TX_METADATA_CHECKSUM_SW)) {
> +					err = skb_checksum_help(skb);
> +					if (err)
> +						goto free_err;
> +				}
> +			}
> +		}
>  	}
>  
>  	skb->dev = dev;
>  	skb->priority = xs->sk.sk_priority;
>  	skb->mark = xs->sk.sk_mark;
>  	skb->destructor = xsk_destruct_skb;
> +	skb_shinfo(skb)->xsk_meta = meta;
>  	xsk_set_destructor_arg(skb);
>  
>  	return skb;

...