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

[netdev-bot+sashiko@kernel.org]

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?

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