xsk: refactors around generic xmit side

[kernel-patches-daemon-bpf-rc]

Pull request for series with
subject: xsk: refactors around generic xmit side
version: 2
url: https://patchwork.kernel.org/project/netdevbpf/list/?series=1006276

[kernel-patches-daemon-bpf-rc]

Upstream branch: dd948aa
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1006276
version: 2

[kernel-patches-daemon-bpf-rc]

Upstream branch: 5730dac
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1006276
version: 2

[kernel-patches-daemon-bpf-rc]

Upstream branch: 5730dac
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1006276
version: 2

[kernel-patches-daemon-bpf-rc]

Upstream branch: 5730dac
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1006276
version: 2

[kernel-patches-daemon-bpf-rc]

Upstream branch: 9b5c111
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1006276
version: 2

[kernel-patches-daemon-bpf-rc]

Upstream branch: 1193c46
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1006276
version: 2

[kernel-patches-daemon-bpf-rc]

Upstream branch: 105eb5d
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1006276
version: 2

[kernel-patches-daemon-bpf-rc]

Upstream branch: d43029f
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1006276
version: 2

[kernel-patches-daemon-bpf-rc]

Upstream branch: d43029f
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1006276
version: 2

[kernel-patches-daemon-bpf-rc]

Upstream branch: d43029f
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1006276
version: 2

[theihor]

This is a test message

Inline review:

commit 44a8057d9daec9219b38510f080f9ef22aca001d
Author: Maciej Fijalkowski <[email protected]>

xsk: wrap generic metadata handling onto separate function

This commit refactors xsk_build_skb() by extracting metadata handling
into a new xsk_skb_metadata() function. It also adds metadata support
for devices with IFF_TX_SKB_NO_LINEAR flag.

> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index f7e0d254a..7b0c68a70 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c

[ ... ]

> +static int xsk_skb_metadata(struct sk_buff *skb, void *buffer,
> +			    struct xdp_desc *desc, struct xsk_buff_pool *pool,
> +			    u32 hr)
> +{
> +	struct xsk_tx_metadata *meta = NULL;
> +
> +	if (unlikely(pool->tx_metadata_len == 0))
> +		return -EINVAL;
> +
> +	meta = buffer - pool->tx_metadata_len;
> +	if (unlikely(!xsk_buff_valid_tx_metadata(meta)))
> +		return -EINVAL;
> +
> +	if (meta->flags & XDP_TXMD_FLAGS_CHECKSUM) {
> +		if (unlikely(meta->request.csum_start +
> +			     meta->request.csum_offset +
> +			     sizeof(__sum16) > desc->len))
> +			return -EINVAL;
> +
> +		skb->csum_start = hr + meta->request.csum_start;
> +		skb->csum_offset = meta->request.csum_offset;
> +		skb->ip_summed = CHECKSUM_PARTIAL;
> +
> +		if (unlikely(pool->tx_sw_csum)) {
> +			int err;
> +
> +			err = skb_checksum_help(skb);
> +			if (err)
> +				return err;
> +		}
> +	}
> +
> +	if (meta->flags & XDP_TXMD_FLAGS_LAUNCH_TIME)
> +		skb->skb_mstamp_ns = meta->request.launch_time;
> +	xsk_tx_metadata_to_compl(meta, &skb_shinfo(skb)->xsk_meta);
> +
> +	return 0;
> +}
> +
>  static struct sk_buff *xsk_build_skb_zerocopy(struct xdp_sock *xs,
>  					      struct xdp_desc *desc)
>  {
> @@ -669,6 +708,9 @@ static struct sk_buff *xsk_build_skb_zerocopy(struct xdp_sock *xs,
>  	int err, i;
>  	u64 addr;
>
> +	addr = desc->addr;
> +	buffer = xsk_buff_raw_get_data(pool, addr);
> +
>  	if (!skb) {
>  		hr = max(NET_SKB_PAD, L1_CACHE_ALIGN(xs->dev->needed_headroom));
>
> @@ -679,6 +721,11 @@ static struct sk_buff *xsk_build_skb_zerocopy(struct xdp_sock *xs,
>  		skb_reserve(skb, hr);
>
>  		xsk_skb_init_misc(skb, xs, desc->addr);
> +		if (desc->options & XDP_TX_METADATA) {
> +			err = xsk_skb_metadata(skb, buffer, desc, pool, hr);
> +			if (unlikely(err))
> +				return ERR_PTR(err);

Does this leak the skb allocated by sock_alloc_send_skb()?

When xsk_skb_metadata() fails here, the function returns ERR_PTR(err)
directly without freeing the skb. The caller xsk_build_skb() sets
skb = NULL after getting the error, so the free_err path won't clean
it up either since it checks if (skb && !skb_shinfo(skb)->nr_frags).

> +		}
>  	} else {
>  		xsk_addr = kmem_cache_zalloc(xsk_tx_generic_cache, GFP_KERNEL);
>  		if (!xsk_addr)

[ ... ]

--

This review is generated by an AI bot. It may be wrong.
AI review job summary: https://github.com/kernel-patches/bpf/actions/runs/18018331618
In-Reply-To-Subject: xsk: wrap generic metadata handling onto separate function