> bpf_xdp_shrink_data() frees a released frag via __xdp_return() using > xdp->rxq->mem.type, but that type is wrong for skb-backed XDP: the skb is > cow'd into page_pool memory while the rxq still says MEM_TYPE_PAGE_SHARED, > so the page_pool page is freed with page_frag_free() and we hit > "Bad page state ... page_pool leak". > > Both generic XDP and veth are affected. A non-linear skb is cow'd into > page_pool memory (skb_cow_data_for_xdp() -> skb_pp_cow_data() for generic > XDP, veth_convert_skb_to_xdp_buff() for veth), so its frags become > page_pool pages while the rxq keeps MEM_TYPE_PAGE_SHARED. > > We can't just fix rxq->mem.type in place: > - generic XDP: xdp->rxq is dev->_rx[queue].xdp_rxq (see > bpf_prog_run_generic_xdp()), a shared rxq that other CPUs may access in > parallel, so we must not write to it. > - veth: rq->xdp_rxq.mem is shared per-queue state that veth resets on XDP > teardown, and with GRO that reset runs without stopping in-flight NAPI, > so a type stashed there can be clobbered under a packet still in flight. > > Adding a check in __xdp_return() or bpf_xdp_shrink_data() itself is not an > option either: without recording it somewhere, both can only guess the > frag's memory type, which quickly gets confusing. > > So record it in the xdp_buff. A dedicated mem_type field (mirroring > xdp_frame->mem_type) would read more naturally, but it grows xdp_buff, and > struct xdp_page_head embeds two of them, which shifts the layout the > xdp_do_redirect live-frames selftest hard-codes (MAX_PKT_SIZE). So use a > flag bit: add XDP_FLAGS_FRAGS_PAGE_POOL, the two skb-cow sites set it, and > bpf_xdp_shrink_data() frees the frag to the page_pool when it is set, > otherwise it keeps falling back to xdp->rxq->mem.type unchanged.
I have not looked into the details yet, but I am wondering if it is possible
to just move xdp_mem_info type (or ever the full xdp_mem_info) in xdp_buff and
remove it from xdp_rxq_info struct. What do you think?
>
> The flag describes the buff, so the buff <-> frame conversions have to keep
> it consistent:
> - buff -> frame: xdp_update_frame_from_buff() copies xdp->flags into the
> frame. veth XDP_TX/XDP_REDIRECT hand the frame to the peer, and cpumap
> and devmap run a second program on it, so the peer would inherit a stale
> tag; strip it, an xdp_frame describes its memory with ::mem_type.
> - frame -> buff: xdp_convert_frame_to_buff() rebuilds a buff, and veth and
> devmap then run a program whose rxq says MEM_TYPE_PAGE_SHARED even for a
> page_pool frame (only cpumap copies mem_type into the rxq). A shrink
> there leaks the same way (this predates the series: bpf_xdp_adjust_tail()
> frees frags by rxq->mem.type), so re-derive the flag from the frame's
> ::mem_type.
>
> Fixes: e6d5dbdd20aa ("xdp: add multi-buff support for xdp running in generic
> mode")
> Fixes: 0ebab78cbcbf ("net: veth: add page_pool for page recycling")
> Fixes: bf25146a5595 ("bpf: add frags support to the bpf_xdp_adjust_tail()
> API")
> Reported-by: [email protected]
> Closes: https://syzkaller.appspot.com/bug?extid=237bbeed8dfe0699b7f5
> Signed-off-by: Jiayuan Chen <[email protected]>
> ---
> drivers/net/veth.c | 6 ++++++
> include/net/xdp.h | 30 +++++++++++++++++++++++++++++-
> net/core/dev.c | 6 ++++++
> net/core/filter.c | 7 +++++++
> 4 files changed, 48 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/veth.c b/drivers/net/veth.c
> index 6ed3ee81153fb..a3fdf1959b76c 100644
> --- a/drivers/net/veth.c
> +++ b/drivers/net/veth.c
> @@ -775,6 +775,12 @@ 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);
Even if it is not a real problem at the moment, I still think we should
set it not just if we have a non-liner skb, but for all skb returned by
skb_pp_cow_data().
> } else {
> xdp_buff_clear_frags_flag(xdp);
> }
> 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
> @@ -81,6 +81,14 @@ enum xdp_buff_flags {
> * XDP program is not attached.
> */
> XDP_FLAGS_FRAGS_UNREADABLE = BIT(2),
> + /*
> + * frags are page_pool memory even though rxq->mem.type is not: a
> + * skb-backed XDP buff (generic XDP, veth) is cow'd into a page_pool.
> + * xdp_buff only: an xdp_frame describes its memory with ::mem_type, so
> + * this is stripped in xdp_update_frame_from_buff() and re-derived from
> + * ::mem_type in xdp_convert_frame_to_buff().
> + */
> + XDP_FLAGS_FRAGS_PAGE_POOL = BIT(3),
> };
>
> struct xdp_buff {
> @@ -131,6 +139,16 @@ static __always_inline void
> xdp_buff_set_frag_unreadable(struct xdp_buff *xdp)
> xdp->flags |= XDP_FLAGS_FRAGS_UNREADABLE;
> }
>
> +static __always_inline void xdp_buff_set_frag_pp(struct xdp_buff *xdp)
> +{
> + xdp->flags |= XDP_FLAGS_FRAGS_PAGE_POOL;
> +}
> +
> +static __always_inline bool xdp_buff_is_frag_pp(const struct xdp_buff *xdp)
> +{
> + return !!(xdp->flags & XDP_FLAGS_FRAGS_PAGE_POOL);
> +}
> +
> static __always_inline u32 xdp_buff_get_skb_flags(const struct xdp_buff *xdp)
> {
> return xdp->flags;
> @@ -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);
> }
>
> static inline
> @@ -420,7 +447,8 @@ int xdp_update_frame_from_buff(const struct xdp_buff *xdp,
> xdp_frame->headroom = headroom - sizeof(*xdp_frame);
> xdp_frame->metasize = metasize;
> xdp_frame->frame_sz = xdp->frame_sz;
> - xdp_frame->flags = xdp->flags;
> + /* XDP_FLAGS_FRAGS_PAGE_POOL is xdp_buff only, don't carry it over */
> + xdp_frame->flags = xdp->flags & ~XDP_FLAGS_FRAGS_PAGE_POOL;
IIUC this is only necessary for the veth case, right? If so, I would suggest to
move it in veth driver (or to have a helper function called just in veth).
Regards,
Lorenzo
>
> return 0;
> }
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 290e0f099e6bf..94268f4d3c5f7 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -5532,6 +5532,12 @@ 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);
> }
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 61940e7535523..d34ba56d79d8f 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -4378,6 +4378,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) {
> --
> 2.43.0
>
signature.asc
Description: PGP signature
