Re: [PATCH bpf] bpf: free page_pool frags via the page_pool path in bpf_xdp_shrink_data

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The netmem_is_pp() check fails on compound tail pages, leading to page_pool leaks and 'Bad page state' kernel panics.
- [Low] The multi-line comment introduced in bpf_xdp_shrink_data() does not follow the required BPF subsystem comment style.
--

--- Patch [1]: [PATCH bpf] bpf: free page_pool frags via the page_pool path in bpf_xdp_shrink_data ---
commit 28e0b86b119236ed1263628b895946efb16e99cf
Author: Jiayuan Chen <[email protected]>

bpf: free page_pool frags via the page_pool path in bpf_xdp_shrink_data

This commit fixes a page_pool leak reported by syzbot when generic XDP
programs shrink skbs into fragments. It inspects the page itself using
netmem_is_pp() rather than relying on the rxq memory model.

Closes: https://syzkaller.appspot.com/bug?extid=237bbeed8dfe0699b7f5

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 3423734124a5b..4536bd147c253 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c

[ ... ]

> @@ -4293,6 +4294,14 @@ static bool bpf_xdp_shrink_data(struct xdp_buff *xdp, skb_frag_t *frag,
>  	if (mem_type == MEM_TYPE_XSK_BUFF_POOL) {
>  		netmem = 0;
>  		zc_frag = bpf_xdp_shrink_data_zc(xdp, shrink, tail, release);
> +	} else if (netmem_is_pp(netmem)) {

[Severity: High]
Does this check handle compound tail pages correctly?

For fragments that span into tail pages of a compound page (such as those
from an order > 0 page_pool allocation), calling netmem_is_pp() directly on
the tail page might fail to find the page_pool signature. Tail pages store
compound_info in the memory location overlapping with pp_magic.

Would it be safer to check the compound head page here, perhaps using
netmem_compound_head(netmem) before checking the signature? Otherwise, tail
pages might still route to the wrong destructor and trigger the Bad page
state panic this patch aims to fix.

> +		/* The rxq mem model does not always describe how a released
> +		 * frag must be freed: the generic-XDP and veth paths run the

[Severity: Low]
This isn't a bug, but does this follow the required BPF subsystem multi-line
comment style?

The style guide requires the opening /* to be on its own line:

/*
 * The rxq mem model does not always describe...

> +		 * program on a shared rxq while the frag has been cow'd into
> +		 * page_pool memory. Trust the page itself and return a
> +		 * page_pool frag to its pool regardless of rxq->mem.type.
> +		 */
> +		mem_type = MEM_TYPE_PAGE_POOL;
>  	}
>  
>  	if (release) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1