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

[Simon Horman <simon.horman@corigine.com>]

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;

...