Re: [RFC PATCH v2 22/25] net/gve: Use PISA memory for RX buffers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] net/gve: wrong GFP flags in gve_rx_alloc_buffer() via
  gve_get_napi_enabled()
- [Medium] net/gve: uninitialized new_info.can_flip copied in
  gve_rx_refill_buffers()
--

commit 58fd34b0315f121e3fcc6437dac09650326d6a54
Author: Luigi Rizzo <lrizzo@google.com>

net/gve: Use PISA memory for RX buffers

This commit allocates GQI-RDA RX packet buffers with pisa_rx_gfp(dev) when
PISA RX buffers are enabled, and replaces plain pages when free PISA RX
blocks are available.

> diff --git a/drivers/net/ethernet/google/gve/gve_rx.c b/drivers/net/ethernet/google/gve/gve_rx.c
> index 89f287b678bf0..85c243bbdeb33 100644
> --- a/drivers/net/ethernet/google/gve/gve_rx.c
> +++ b/drivers/net/ethernet/google/gve/gve_rx.c
> @@ -4,6 +4,7 @@
>   * Copyright (C) 2015-2021 Google, Inc.
>   */
>  
> +#include <linux/pisa.h>
>  #include "gve.h"
>  #include "gve_adminq.h"
>  #include "gve_utils.h"
> @@ -170,6 +171,8 @@ static int gve_rx_alloc_buffer(struct gve_priv *priv, struct device *dev,
>  	dma_addr_t dma;
>  	int err;
>  
> +	if (rx->data.raw_addressing)
> +		gfp |= pisa_rx_gfp(dev);
>  	err = gve_alloc_page(priv, dev, &page, &dma, DMA_FROM_DEVICE, gfp);
>  	if (err) {
>  		u64_stats_update_begin(&rx->statss);
> @@ -499,13 +502,19 @@ static void gve_rx_flip_buff(struct gve_rx_slot_page_info *page_info, __be64 *sl
>  	*(slot_addr) ^= offset;
>  }
>  
> -static int gve_rx_can_recycle_buffer(struct gve_rx_slot_page_info *page_info)
> +static int gve_rx_can_recycle_buffer(struct gve_rx_ring *rx,
> +				     struct gve_rx_slot_page_info *page_info)
>  {
>  	int pagecount = page_count(page_info->page);
>  
>  	/* This page is not being used by any SKBs - reuse */
> -	if (pagecount == page_info->pagecnt_bias)
> +	if (pagecount == page_info->pagecnt_bias) {
> +		if (rx && rx->data.raw_addressing &&
> +		    !is_pisa_page(page_info->page) &&
> +		    pisa_has_free_blocks_rx(&rx->gve->pdev->dev, 0, rx->gve->numa_node))
> +			return 0;
>  		return 1;
> +	}

[Severity: High]
Can gve_rx_alloc_buffer() sleep in softirq context or pass GFP_ATOMIC in
process context when selecting GFP flags via gve_get_napi_enabled(priv)?

In gve_turnup(), NAPI is enabled, IRQs are unmasked, and napi_schedule()
is called on all RX queues before gve_set_napi_enabled(priv) runs:

drivers/net/ethernet/google/gve/gve_main.c:gve_turnup() {
    ...
		napi_enable_locked(&block->napi);
		netif_queue_set_napi(priv->dev, idx, NETDEV_QUEUE_TYPE_RX,
				     &block->napi);
    ...
		mb();
		napi_schedule(&block->napi);
	}

	if (priv->tx_cfg.num_xdp_queues && gve_supports_xdp_xmit(priv))
		xdp_features_set_redirect_target_locked(priv->dev, false);

	gve_set_napi_enabled(priv);
}

With gve_rx_can_recycle_buffer() returning 0 to evict non-PISA pages even
when pagecount == pagecnt_bias, a NAPI poll running before
gve_set_napi_enabled(priv) reaches gve_rx_refill_buffers() ->
gve_rx_alloc_buffer() while gve_get_napi_enabled(priv) is still false.
That passes GFP_KERNEL | __GFP_PISA_RW in softirq context, allowing the
page allocator or PISA metadata initialization and page decryption to
sleep in atomic context.

Conversely, gve_adjust_config() -> gve_queues_mem_alloc() and
gve_rx_queue_mem_alloc() pre-fill new RX rings in process context while
the interface is up and gve_get_napi_enabled(priv) is true, passing
GFP_ATOMIC | __GFP_PISA_RW and preventing PISA from blocking to initialize
metadata or decrypt pages.

Passing an explicit gfp_t parameter from gve_rx_prefill_pages()
(GFP_KERNEL) and gve_rx_refill_buffers() (GFP_ATOMIC) into
gve_rx_alloc_buffer() would avoid relying on gve_get_napi_enabled(priv).

[ ... ]

> @@ -987,12 +996,15 @@ static bool gve_rx_refill_buffers(struct gve_priv *priv, struct gve_rx_ring *rx)
>  				union gve_rx_data_slot *data_slot =
>  						&rx->data.data_ring[idx];
>  				struct device *dev = &priv->pdev->dev;
> -				gve_rx_free_buffer(dev, page_info, data_slot);
> -				page_info->page = NULL;
> -				if (gve_rx_alloc_buffer(priv, dev, page_info,
> -							data_slot, rx)) {
> +				struct gve_rx_slot_page_info new_info;
> +				union gve_rx_data_slot new_slot;
> +
> +				if (gve_rx_alloc_buffer(priv, dev, &new_info,
> +							&new_slot, rx))
>  					break;
> -				}
> +				gve_rx_free_buffer(dev, page_info, data_slot);
> +				*page_info = new_info;
> +				*data_slot = new_slot;
>  			}
>  		}
>  		fill_cnt++;

[Severity: Medium]
Does copying new_info into *page_info overwrite page_info->can_flip with
uninitialized stack memory?

new_info is declared on the stack without initialization, and
gve_rx_alloc_buffer() -> gve_setup_rx_buffer() only initializes page,
page_offset, page_address, buf_size, and pagecnt_bias, leaving can_flip
(and pad) untouched.  Previously, page_info was zero-allocated and this
branch was only entered when page_info->can_flip was 0.

When the next packet received into this slot takes the copybreak path in
gve_rx_skb(), a non-XDP_PASS verdict, or a drop path in gve_rx(),
page_info->can_flip is not updated, so the following
gve_rx_refill_buffers() pass checks an uninitialized page_info->can_flip
and can spuriously flip the buffer instead of checking whether it can be
recycled.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109182533.3594984-1-lrizzo@google.com?part=22