This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible. --- bpf, veth: xdp: fix page_pool page leak on skb-backed XDP
This adds an XDP_FLAGS_FRAGS_PAGE_POOL xdp_buff flag that the two skb-cow sites (generic XDP and veth) set, so bpf_xdp_shrink_data() frees a released frag to the page_pool instead of using xdp->rxq->mem.type. The flag is stripped on buff -> frame conversion and re-derived from frame->mem_type on frame -> buff conversion. Closes: https://syzkaller.appspot.com/bug?extid=237bbeed8dfe0699b7f5 > diff --git a/include/net/xdp.h b/include/net/xdp.h > index aa742f413c358..8fd4139e9cbd3 100644 > --- a/include/net/xdp.h > +++ b/include/net/xdp.h [ ... ] > @@ -394,6 +412,15 @@ void xdp_convert_frame_to_buff(const struct xdp_frame > *frame, > xdp->data_meta = frame->data - frame->metasize; > xdp->frame_sz = frame->frame_sz; > xdp->flags = frame->flags; > + /* > + * frame->flags never carries XDP_FLAGS_FRAGS_PAGE_POOL (it is stripped > + * in xdp_update_frame_from_buff()); re-derive it from the frame's own > + * memory type. veth and devmap rebuild a buff here and run a program > + * whose rxq says MEM_TYPE_PAGE_SHARED, so without this a shrink would > + * free a page_pool frag through page_frag_free(). > + */ > + if (frame->mem_type == MEM_TYPE_PAGE_POOL) > + xdp_buff_set_frag_pp(xdp); > } [ ... ] > diff --git a/net/core/filter.c b/net/core/filter.c > index 532405988fd98..861c878d0a6d6 100644 > --- a/net/core/filter.c > +++ b/net/core/filter.c > @@ -4377,6 +4377,13 @@ 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 (xdp_buff_is_frag_pp(xdp)) { > + /* > + * Skb-backed XDP (generic XDP, veth) cow's the frags into a > + * page_pool while the rxq stays MEM_TYPE_PAGE_SHARED, so free > + * the frag to the pool, not via page_frag_free(). > + */ > + mem_type = MEM_TYPE_PAGE_POOL; > } > > if (release) { [Severity: High] The new flag only overrides mem_type in one direction (towards MEM_TYPE_PAGE_POOL), while mem_type is still initialized from the shared xdp->rxq->mem.type: net/core/filter.c:bpf_xdp_shrink_data() { enum xdp_mem_type mem_type = xdp->rxq->mem.type; ... } The commit message says veth's rq->xdp_rxq.mem "is shared per-queue state" that "can be clobbered under a packet still in flight". veth also writes a frame's memory type into it and never restores it: drivers/net/veth.c:veth_xdp_rcv_one() { case XDP_TX: orig_frame = *frame; xdp->rxq->mem.type = frame->mem_type; ... case XDP_REDIRECT: orig_frame = *frame; xdp->rxq->mem.type = frame->mem_type; } Only veth_disable_xdp_range() puts rq->xdp_rxq.mem back to rq->xdp_mem. So can the opposite direction still go wrong on a veth queue? 1) A multi-buff frame with frame->mem_type == MEM_TYPE_PAGE_POOL (e.g. redirected in from a page_pool NIC) takes XDP_TX or XDP_REDIRECT, and rq->xdp_rxq.mem.type stays MEM_TYPE_PAGE_POOL for every later packet on that queue. 2) A later multi-buff frame whose frame->mem_type is MEM_TYPE_PAGE_SHARED (the peer's veth_xdp_rcv_skb() XDP_TX/XDP_REDIRECT path stamps xdp->rxq->mem = rq->xdp_mem into the frame) or MEM_TYPE_PAGE_ORDER0 arrives. xdp_convert_frame_to_buff() does not set XDP_FLAGS_FRAGS_PAGE_POOL for it, and it does not clear the flag either, so bpf_xdp_shrink_data() picks up the stale MEM_TYPE_PAGE_POOL from the rxq. 3) If the program then calls bpf_xdp_adjust_tail() with a negative offset that consumes a whole frag, the release path ends in: net/core/xdp.c:__xdp_return() { case MEM_TYPE_PAGE_POOL: netmem = netmem_compound_head(netmem); ... /* No need to check netmem_is_pp() as mem->type knows this a * page_pool page */ page_pool_put_full_netmem(netmem_get_pp(netmem), netmem, napi_direct); } There is no netmem_is_pp() validation here, so netmem_get_pp() on a plain page returns whatever occupies the overlaid field, and NULL for a page that veth already released from its pool. Can that dereference a garbage or NULL struct page_pool *, or hand a foreign page to an unrelated pool's cache? Would it be more robust for bpf_xdp_shrink_data() to derive the frag memory type entirely from the buff (i.e. also express "not page_pool" instead of falling back to rxq->mem.type), so the shared and clobberable veth rxq type is not consulted at all? Note this residual path is not addressed later in the series: the diff of net/, drivers/net/veth.c and include/net/xdp.h between this commit and 6c9c432f is empty, the following patch only adds a selftest.
