Re: [PATCH net] xdp: fix zero-copy frame layout

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Double free of XSK buffer and memory leak of `xdp_frame` page on redirect enqueue error.
- [High] Silent truncation of fragmented AF_XDP (MEM_TYPE_XSK_BUFF_POOL) multi-buffer packets during redirect.
--

--- Patch [1]: [PATCH net] xdp: fix zero-copy frame layout ---
commit 300f3eb2043fb6dcf3006d467327612f861ccc98
Author: Weiming Shi <bestswngs@gmail.com>

xdp: fix zero-copy frame layout

xdp_convert_zc_to_xdp_frame() clones an XSK packet into an order-0 page
and advertises PAGE_SIZE as its frame size. It allows the copied frame
to occupy the page tail needed by skb_shared_info and records zero
headroom even when metadata separates the frame header from packet data.

> diff --git a/net/core/xdp.c b/net/core/xdp.c
> index 0194e69da339a..1d679e8fd6493 100644
> --- a/net/core/xdp.c
> +++ b/net/core/xdp.c
> @@ -585,7 +585,7 @@ struct xdp_frame *xdp_convert_zc_to_xdp_frame(struct xdp_buff *xdp)
>  		   xdp->data - xdp->data_meta;
>  	totsize = xdp->data_end - xdp->data + metasize;
>  
> -	if (sizeof(*xdpf) + totsize > PAGE_SIZE)
> +	if (sizeof(*xdpf) + totsize > SKB_WITH_OVERHEAD(PAGE_SIZE))
>  		return NULL;

[Severity: High]
This isn't a bug introduced by this patch, but does this function silently
drop fragments when an AF_XDP zero-copy buffer contains them?

Looking at xdp_convert_zc_to_xdp_frame() in net/core/xdp.c, totsize is
calculated using only xdp->data_end - xdp->data. It ignores
xdp_buff_has_frags(xdp), meaning it allocates an order-0 page and copies only
the linear data.

Any attached fragments in the tailroom's skb_shared_info seem to be ignored,
and while they are freed via xsk_buff_free(xdp), the resulting xdp_frame is
silently truncated.

>  
>  	page = dev_alloc_page();
[ ... ]
> @@ -602,7 +602,7 @@ struct xdp_frame *xdp_convert_zc_to_xdp_frame(struct xdp_buff *xdp)
>  
>  	xdpf->data = addr + metasize;
>  	xdpf->len = totsize - metasize;
> -	xdpf->headroom = 0;
> +	xdpf->headroom = metasize;
>  	xdpf->metasize = metasize;
>  	xdpf->frame_sz = PAGE_SIZE;
>  	xdpf->mem_type = MEM_TYPE_PAGE_ORDER0;

[Severity: Critical]
This is a pre-existing issue, but does freeing the zero-copy buffer at the end
of this function cause a double-free and memory leak if the subsequent enqueue
operation fails?

In net/core/xdp.c:xdp_convert_zc_to_xdp_frame(), the original buffer is
explicitly freed via xsk_buff_free(xdp) upon successful conversion. It then
returns the new xdp_frame to xdp_do_redirect():

net/core/filter.c:xdp_do_redirect() {
    ...
    return __xdp_do_redirect_frame(ri, dev, xdp_convert_buff_to_frame(xdp),
                                   xdp_prog);
}

If __xdp_do_redirect_frame() fails, it returns the error code without freeing
the xdp_frame parameter:

net/core/filter.c:__xdp_do_redirect_frame() {
    ...
err:
    _trace_xdp_redirect_map_err(dev, xdp_prog, fwd, map_type, map_id, ri->tgt_index, err);
    return err;
}

Since the pointer is not saved in xdp_do_redirect(), the newly allocated
page appears to be leaked.

Additionally, because the error propagates back to the calling driver, the
driver might assume the subsystem did not consume the packet and proceed to
free the original ZC buffer again, potentially causing a double-free in the
XSK pool.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818154516.793517-1-bestswngs@gmail.com?part=1