Re: [PATCH net] gve: leave refcount headroom in the RX page bias

[Amaan Cheval]

On Sun, Nov 1, 2026 at 11:49 PM <netdev-bot+sashiko@xxxxxxxxxx> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
>
> Critical: 1 · High: 0 · Medium: 0 (1) · Low: 0
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [Critical] This patch moves gve's RX page bias from INT_MAX to
>   USHRT_MAX, but keeps the old refill rule in gve_dec_pagecnt_bias()…
>
> Pre-existing issues:
> - [Medium] This problem wasn't introduced by this patch, but in
>   gve_rx_copy_to_pool() (drivers/net/ethernet/google/gve/gve_rx.c) the…
>
> --
>
> gve: leave refcount headroom in the RX page bias
>
> This patch changes the GQI and DQO-QPL RX page bias from INT_MAX to
> USHRT_MAX and stores pagecnt_bias in a u16. A get_page() from the stack
> therefore no longer wraps the page refcount negative.
> gve_dec_pagecnt_bias() now adds a fixed batch of GVE_PAGECNT_BIAS
> references when the bias reaches zero.
>
> Link: https://github.com/amaanc/kernel-bias
>
> > diff --git a/drivers/net/ethernet/google/gve/gve_rx.c b/drivers/net/ethernet/google/gve/gve_rx.c
> > index 81ea800e66e94..6533fff061555 100644
> > --- a/drivers/net/ethernet/google/gve/gve_rx.c
> > +++ b/drivers/net/ethernet/google/gve/gve_rx.c
> > @@ -152,8 +152,8 @@ static void gve_setup_rx_buffer(struct gve_rx_ring *rx,
> >       page_info->buf_size = rx->packet_buffer_size;
> >       *slot_addr = cpu_to_be64(addr);
> >       /* The page already has 1 ref */
> > -     page_ref_add(page, INT_MAX - 1);
> > -     page_info->pagecnt_bias = INT_MAX;
> > +     page_ref_add(page, GVE_PAGECNT_BIAS - 1);
> > +     page_info->pagecnt_bias = GVE_PAGECNT_BIAS;
> >  }
>
> [Severity: Medium]
> This is a pre-existing issue and was not introduced by this patch. While
> looking at the GQI-QPL copy pool, does gve_rx_copy_to_pool() leak the
> fallback page?
>
> drivers/net/ethernet/google/gve/gve_rx.c:gve_rx_copy_to_pool() {
>     ...
>                 page = alloc_page(GFP_ATOMIC);
>                 if (!page)
>                         return NULL;
>
>                 alloc_page_info.page = page;
>     ...
>                 skb = gve_rx_add_frags(napi, &alloc_page_info,
>                                        PAGE_SIZE,
>                                        len, ctx);
>     ...
>                 return skb;
>     ...
> }
>
> gve_rx_add_frags() can return NULL before skb_add_rx_frag() attaches
> the page:
>
>                 skb = napi_get_frags(napi);
>                 if (unlikely(!skb))
>                         return NULL;
>     ...
>                         skb = napi_alloc_skb(napi, 0);
>                         if (!skb)
>                                 return NULL;
>
> In that case the only pointer to the page is in the stack-local
> alloc_page_info. gve_rx_copy_to_pool() then returns NULL without calling
> put_page(page).
>

I can investigate this separately and prepare another patch later.

> [ ... ]
>
> > diff --git a/drivers/net/ethernet/google/gve/gve_utils.c b/drivers/net/ethernet/google/gve/gve_utils.c
> > index caed38cd5f2b3..cb6fba3122d5b 100644
> > --- a/drivers/net/ethernet/google/gve/gve_utils.c
> > +++ b/drivers/net/ethernet/google/gve/gve_utils.c
> > @@ -93,15 +93,9 @@ void gve_dec_pagecnt_bias(struct gve_rx_slot_page_info *page_info)
> >  {
> >       page_info->pagecnt_bias--;
> >       if (page_info->pagecnt_bias == 0) {
> > -             int pagecount = page_count(page_info->page);
> > -
> > -             /* If we have run out of bias - set it back up to INT_MAX
> > -              * minus the existing refs.
> > -              */
> > -             page_info->pagecnt_bias = INT_MAX - pagecount;
> > -
> > -             /* Set pagecount back up to max. */
> > -             page_ref_add(page_info->page, INT_MAX - pagecount);
> > +             /* Out of bias: take another batch of references. */
> > +             page_info->pagecnt_bias = GVE_PAGECNT_BIAS;
> > +             page_ref_add(page_info->page, GVE_PAGECNT_BIAS);
> >       }
> >  }
>
> [Severity: Critical]
> Can this page_ref_add() run on a QPL page that has already been freed?
>
> The top-up only happens once the bias has reached zero, which is after
> the last driver-held reference was handed off. igb, one of the drivers
> the commit message cites, refills while the bias is still 1, before the
> handoff:
>
> drivers/net/ethernet/intel/igb/igb_main.c:igb_can_reuse_rx_page() {
>     ...
>         if (unlikely(pagecnt_bias == 1)) {
>                 page_ref_add(page, USHRT_MAX - 1);
>                 rx_buffer->pagecnt_bias = USHRT_MAX;
>         }
>     ...
> }
>
> On DQO-QPL, gve_xdp_done_dqo() calls xdp_do_redirect() first and only
> then drops the bias:
>
>         case XDP_REDIRECT:
>                 err = xdp_do_redirect(priv->dev, xdp, xprog);
>                 if (unlikely(err))
>                         goto err;
>                 gve_reuse_buffer(rx, buf_state);
>
> gve_reg_xdp_info() registers MEM_TYPE_PAGE_SHARED for QPL queues. With
> an AF_XDP socket in copy mode, the redirect target returns the buffer
> before xdp_do_redirect() comes back to the driver:
>
> gve_xdp_done_dqo()
>   xdp_do_redirect()
>     __xsk_map_redirect()
>       xsk_rcv()
>         xdp_return_buff()        /* MEM_TYPE_PAGE_SHARED */
>           page_frag_free()
>             put_page_testzero() -> free_frozen_pages()
>   gve_reuse_buffer()
>     gve_dec_pagecnt_bias()
>       page_ref_add()             /* page already freed */
>
> Each redirect lowers page_count and pagecnt_bias by one together. After
> 65534 redirects from one QPL page, both are 1, and the remaining
> reference is the QPL's own base reference. The next redirect frees the
> page in page_frag_free(), even though the page is still registered with
> the NIC and still listed in qpl->pages[].
>
> gve_dec_pagecnt_bias() then sets the bias to 65535 and adds references
> to the freed page. The device keeps DMA-ing received packets into it,
> and the driver keeps handing it to XDP and to skbs.
>
> Nothing appears to block this setup. gve_verify_xdp_configuration() does
> not reject DQO-QPL, and dev_xdp_attach() does not check xdp_features.
>
> The same ordering problem existed with INT_MAX, but then it took about
> 2^31 handoffs per page. With USHRT_MAX it takes 65535, which sustained
> traffic can reach quickly.
>
> Should the refill happen at pagecnt_bias == 1, before the buffer is
> handed off, as igb does?

pw-bot: cr
Good catch, bot, thank you! I'll address this and prepare a v2 patch.
I don't have access to DQO-QPL hardware, so I'll need someone to
test the v2 PATCH on hardware, please.

>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261031173449.2439047-1-amaan.cheval%40gmail.com