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

[Amaan Cheval]

On Thu, Nov 5, 2026 at 2:39 AM <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 · 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 refills the bias only after
>   gve_dec_pagecnt_bias() reaches 0, and it lowers the per-page bias from…
>
> --
>
> gve: leave refcount headroom in the RX page bias
>
> This patch lowers the gve RX page refcount bias from INT_MAX to USHRT_MAX
> and stores it in a u16. This leaves page_count() room for references the
> stack takes with get_page(), such as splice() and tee(). When the bias
> reaches 0, it is refilled with a fixed batch of GVE_PAGECNT_BIAS.
>
> Link: https://github.com/amaanc/kernel-bias
>
> > 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 free a DQO-QPL RX page that is still registered with the device?
>
> Because the refill happens only at 0, the 1 -> 0 transition hands the
> driver's last reference to the consumer. With INT_MAX a page needed about
> 2^31 handoffs to get there. With GVE_PAGECNT_BIAS it happens every 65534
> handoffs.
>
> On DQO-QPL, gve_xdp_done_dqo() redirects the buffer before it touches 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 QPL rings as MEM_TYPE_PAGE_SHARED. If the
> redirect target is an AF_XDP socket in copy mode, the page is released
> synchronously:
>
> xdp_do_redirect()
>   xsk_rcv()
>     xdp_return_buff()
>       __xdp_return()
>         page_frag_free()
>           put_page_testzero()
>           free_frozen_pages()
>
> Suppose the page is at pagecnt_bias == 1 and page_count == 1. That is the
> normal recyclable state once all earlier consumers have released their
> references. The QPL page then goes back to the buddy allocator while it is
> still DMA mapped, registered in the QPL and owned by the buf_state.
>
> After that, gve_reuse_buffer()->gve_dec_pagecnt_bias() calls
> page_ref_add(page, GVE_PAGECNT_BIAS) on the freed page.
> gve_try_recycle_buf() then sees page_count - bias == 0 and posts the page
> back to the NIC. Could the device then DMA packet data into memory that
> has already been handed to another user?
>
> The v3 notes say XDP on DQO-QPL "will be blocked explicitly in a separate
> patch". In this tree, gve_xdp(), gve_verify_xdp_configuration() and
> gve_set_xdp() do not check queue_format.
>
> gve_set_netdev_xdp_features() sets xdp_features = 0 for DQO-QPL, but
> nothing on the attach path enforces it. xp_assign_dev() also falls back to
> copy mode when NETDEV_XDP_ACT_XSK is missing.
>
> That note is below the --- line, so it will not be in the applied commit.
> The patch is also tagged for stable without naming the prerequisite.
>
> The commit message also says:
>
>   Use USHRT_MAX in a u16, as igb, igc, ixgbe, i40e and hns3 do
>
> Those drivers refill at bias == 1, so they always keep a reference. For
> example, igb_can_reuse_rx_page() does:
>
>         if (unlikely(pagecnt_bias == 1)) {
>                 page_ref_add(page, USHRT_MAX - 1);
>                 rx_buffer->pagecnt_bias = USHRT_MAX;
>         }
>
> Would it be safer to refill at 1 here, as v2 did, so the driver never
> gives up its last reference on a QPL page?

The scenario is real, but it is reachable only because XDP can be
attached on DQO-QPL at all, which the driver does not support:
commit d8a8ca14c937 ("gve: add XDP_TX and XDP_REDIRECT support for DQ
RDA") dropped the queue_format check that used to refuse it, and the
core only checks that ndo_bpf exists. Harshitha indicated on v2 that the
right fix is to block XDP on DQO-QPL rather than to change the refill
point.

Harshitha, I know you mentioned Google would send that patch:

  https://lore.kernel.org/netdev/CAEAWyHfCM_ZesnChP71AQvxjM4MASwukesgzuWCYECts7jiNhA@xxxxxxxxxxxxxx/

Since this fix depends on it, I'd like to post v4 as a two-patch
series with the DQO-QPL block as 1/2, so that the fix and its
prerequisite reach stable together. If you already have one in
flight, or would rather send your own, please let me know and
I'll drop 1/2 and rebase on yours instead.

pw-bot: cr

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