Re: [PATCH net v4 1/2] gve: block XDP on the DQO-QPL queue format

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · 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).

- [High] The patch is incomplete for GVE_GQI_RDA_FORMAT.

--

gve: block XDP on the DQO-QPL queue format

This patch adds a queue_format check to gve_verify_xdp_configuration()
that refuses XDP program setup on DQO-QPL. That check was lost when
d8a8ca14c937 enabled XDP on DQO-RDA. The DQO-QPL RX page accounting can
free a page that is still DMA-mapped when a redirect target releases the
buffer synchronously.

> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> index 2437cd8ba6331..4e9e692a0ed0a 100644
> --- a/drivers/net/ethernet/google/gve/gve_main.c
> +++ b/drivers/net/ethernet/google/gve/gve_main.c
> @@ -1723,6 +1723,12 @@ static int gve_verify_xdp_configuration(struct net_device *dev,
>  	struct gve_priv *priv = netdev_priv(dev);
>  	u16 max_xdp_mtu;
>  
> +	if (priv->queue_format == GVE_DQO_QPL_FORMAT) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "XDP is not supported in DQO QPL queue format.");
> +		return -EOPNOTSUPP;
> +	}
> +

[Severity: High]
Does this leave GVE_GQI_RDA_FORMAT uncovered? The commit message says the
check removed by d8a8ca14c937 "refused every format but GQI-QPL", so it
blocked GQI-RDA as well as DQO-QPL. Only DQO-QPL is added back here.

GQI-RDA also falls into the else branch of gve_set_netdev_xdp_features():

	} else {
		xdp_features = 0;
	}

and gve_xdp_xmit() rejects it too:

	if (priv->queue_format == GVE_GQI_QPL_FORMAT)
		return gve_xdp_xmit_gqi(dev, n, frames, flags);
	else if (priv->queue_format == GVE_DQO_RDA_FORMAT)
		return gve_xdp_xmit_dqo(dev, n, frames, flags);

	return -EOPNOTSUPP;

On GQI-RDA, though, every remaining check in this function can pass:

  - GQI has no NETIF_F_GRO_HW.
  - Header split is DQO-RDA only.
  - packet_buffer_size defaults to 2K.
  - GVE_RX_PAD is only subtracted for GQI-QPL.
  - The rx/tx queue count condition can be met with ethtool -L.

Once verification passes on a running interface, the path is:

gve_xdp()->gve_set_xdp()->gve_configure_rings_xdp()->gve_adjust_config()
  ->gve_queues_start()->gve_reg_xdp_info()

On GQI-RDA, gve_is_qpl() is false, so gve_reg_xdp_info() takes this
branch:

		else
			err = xdp_rxq_info_reg_mem_model(&rx->xdp_rxq,
							 MEM_TYPE_PAGE_POOL,
							 rx->dqo.page_pool);

On a GQI ring, rx->dqo.page_pool is NULL because gve_rx_alloc_ring_gqi()
memsets the ring. __xdp_reg_mem_model() therefore returns -EINVAL. By
then gve_adjust_config() has already closed the old queues, so it logs
"!!! DISABLING ALL QUEUES !!!" and turns the device down.

Before that failure, gve_queues_start() has already set:

	priv->tx_cfg.num_xdp_queues = tx_alloc_cfg->num_xdp_rings;

and nothing rolls it back. A later ifdown/ifup goes through
gve_open()->gve_get_curr_alloc_cfgs(), rebuilds the XDP rings, and fails
in gve_reg_xdp_info() again. Does the NIC then stay down until a device
reset? If the interface is already down at attach time, gve_set_xdp()
installs the program, and every later gve_open() fails with -EINVAL.

There is also a NULL dereference that only this failure hides.
gve_tx_fill_xdp() uses tx->tx_fifo.base and
tx->tx_fifo.qpl->page_buses. On raw-addressing GQI TX rings these are
never set up: gve_tx_alloc_ring_gqi() skips the FIFO/QPL setup, and
gve_can_tx() skips the FIFO check. That dereference would become
reachable if GQI-RDA mem model registration ever succeeded.

Would an allowlist (accept only GQI_QPL and DQO_RDA) be a closer match to
"matching the features the driver advertises"? Checking for
NETDEV_XDP_ACT_BASIC in dev->xdp_features would also work.

With the Fixes: d8a8ca14c937 and Cc: stable tags, backporters may assume
that regression is fully closed. The later patch in the series ("gve: fix
RX page refcount overflow from the pagecnt bias") does not touch
gve_verify_xdp_configuration().

>  	if (dev->features & NETIF_F_GRO_HW) {
>  		NL_SET_ERR_MSG_MOD(extack,
>  				   "XDP is not supported when HW-GRO is on.");

[ ... ]

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