Re: [PATCH bpf v2 1/2] bpf, veth: xdp: fix page_pool page leak on skb-backed XDP

> diff --git a/drivers/net/veth.c b/drivers/net/veth.c
> index 6ed3ee81153f..0afa0661ada1 100644
> --- a/drivers/net/veth.c
> +++ b/drivers/net/veth.c
> @@ -775,6 +775,11 @@ static int veth_convert_skb_to_xdp_buff(struct veth_rq *rq,
>  	if (skb_shinfo(skb)->nr_frags) {
>  		skb_shinfo(skb)->xdp_frags_size = skb->data_len;
>  		xdp_buff_set_frags_flag(xdp);
> +		/* A nonlinear skb was cow'd into rq->page_pool above, so the
> +		 * frags must be freed to that pool, not via the rxq's
> +		 * MEM_TYPE_PAGE_SHARED.
> +		 */
> +		xdp_buff_set_frag_pp(xdp);
>  	} else {
>  		xdp_buff_clear_frags_flag(xdp);
>  	}

[ ... ]

> diff --git a/net/core/dev.c b/net/core/dev.c
> index 38336858c168..be36020484b6 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -5532,6 +5532,11 @@ u32 bpf_prog_run_generic_xdp(struct sk_buff *skb, struct xdp_buff *xdp,
>  	if (skb_is_nonlinear(skb)) {
>  		skb_shinfo(skb)->xdp_frags_size = skb->data_len;
>  		xdp_buff_set_frags_flag(xdp);
> +		/* A nonlinear skb was cow'd into page_pool memory by
> +		 * skb_cow_data_for_xdp() before we got here, so the frags must
> +		 * be freed to that pool, not via the rxq's MEM_TYPE_PAGE_SHARED.
> +		 */
> +		xdp_buff_set_frag_pp(xdp);
>  	} else {
>  		xdp_buff_clear_frags_flag(xdp);
>  	}

This isn't a bug, but is the comment completely accurate?

The comment states the skb was cow'd "before we got here" by
skb_cow_data_for_xdp(). This is correct for the netif_receive_generic_xdp()
caller, where netif_skb_check_for_xdp() calls skb_cow_data_for_xdp() before
bpf_prog_run_generic_xdp().

However, bpf_prog_run_generic_xdp() has three other callers:

cpu_map_bpf_prog_run_skb():
    kernel/bpf/cpumap.c calls bpf_prog_run_generic_xdp() directly

dev_map_bpf_prog_run_skb():
    kernel/bpf/devmap.c calls bpf_prog_run_generic_xdp() directly

For these callers, skb_cow_data_for_xdp() is not called immediately before
this function. Instead, the page_pool invariant is inherited from an earlier
ingress generic-XDP pass where the skb went through
netif_receive_generic_xdp() -> netif_skb_check_for_xdp().

Should the comment acknowledge that the cow may have happened earlier in the
packet's path for some callers, rather than stating it always happens
"before we got here"?

[ ... ]

>     So record it in the xdp_buff. Add a XDP_FLAGS_FRAGS_PAGE_POOL flag; the two
>     skb-cow sites set it, and bpf_xdp_shrink_data() frees the frag to the

This isn't a bug, but is "the two skb-cow sites" the right description?

The commit message states "the two skb-cow sites set it", referring to
drivers/net/veth.c and net/core/dev.c where xdp_buff_set_frag_pp() is
called.

Looking at the code:

drivers/net/veth.c:782 is inside veth_convert_skb_to_xdp_buff(), which does
call skb_pp_cow_data() a few lines earlier to cow the nonlinear skb into
rq->page_pool. This is a cow site.

net/core/dev.c:5539 is inside bpf_prog_run_generic_xdp(), which does not
itself call skb_cow_data_for_xdp(). The cow happens one level up in
netif_receive_generic_xdp() -> netif_skb_check_for_xdp().

Would it be more precise to say "one cow site and one post-cow site" or "the
two sites where skb frags are known to be page_pool memory", since
bpf_prog_run_generic_xdp() is not itself performing the cow?

Also, bpf_prog_run_generic_xdp() has three callers that never cow at all:
cpu_map_bpf_prog_run_skb() and dev_map_bpf_prog_run_skb(). Their skbs
originate from xdp_do_generic_redirect_map() after going through
netif_receive_generic_xdp() where the cow already happened. So the flag is
set even though no cow occurs in the current call chain, which makes "two
skb-cow sites" a bit ambiguous.


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/32686145995