Re: [PATCH net] 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 (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).

[ ... ]

> 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?

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