Re: [PATCH net v5 1/2] gve: refuse XDP on queue formats without XDP support

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] The commit message misstates which queue formats the new check
  affects.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch
  extends it.

--

gve: refuse XDP on queue formats without XDP support

This patch adds a check to gve_verify_xdp_configuration() that refuses XDP
setup when dev->xdp_features lacks NETDEV_XDP_ACT_BASIC. It replaces the
queue format gate dropped by commit d8a8ca14c937. The aim is to keep XDP
off DQO-QPL, whose RX page accounting can free a page that is still
DMA-mapped and registered in the QPL.

> Refuse XDP setup in gve_verify_xdp_configuration() whenever the driver
> advertises no XDP features for the current queue format, which today
> means DQO-QPL.

[Severity: Medium]
Is "which today means DQO-QPL" accurate? gve_set_netdev_xdp_features() only
sets NETDEV_XDP_ACT_BASIC for GQI-QPL and DQO-RDA:

drivers/net/ethernet/google/gve/gve_main.c:gve_set_netdev_xdp_features() {
	if (priv->queue_format == GVE_GQI_QPL_FORMAT) {
		xdp_features = NETDEV_XDP_ACT_BASIC;
		...
	} else if (priv->queue_format == GVE_DQO_RDA_FORMAT) {
		xdp_features = NETDEV_XDP_ACT_BASIC;
		...
	} else {
		xdp_features = 0;
	}
	...
}

GQI-RDA also falls into the else branch. gve_adminq.c can still select
GVE_GQI_RDA_FORMAT from the GQI_RDA and GQI_RAW_ADDRESSING device options,
so the new check refuses GQI-RDA as well.

The check that d8a8ca14c937 removed (queue_format != GVE_GQI_QPL_FORMAT)
also refused GQI-RDA. Since that commit, XDP could be attached on GQI-RDA,
and XDP_TX there takes this path:

gve_rx()
  gve_xdp_done()
    gve_xdp_xmit_one()
      gve_tx_fill_xdp()

gve_tx_fill_xdp() always uses tx->tx_fifo: it calls gve_tx_alloc_fifo(),
memcpy()s into tx->tx_fifo.base and reads tx->tx_fifo.qpl->page_buses. A
raw-addressing TX ring never allocates that FIFO. Isn't that a NULL
dereference that this patch also fixes without saying so?

Could the commit message mention GQI-RDA? Two things are worth stating:
GQI-RDA now gets -EOPNOTSUPP, and this closes the XDP_TX crash there. That
would help stable backporters.

The follow-up patch "gve: fix RX page refcount overflow from INT_MAX
pagecnt_bias" uses the same wording ("The previous patch keeps it that way
by blocking XDP on DQO-QPL"). Its safety argument also depends on XDP being
blocked on GQI-RDA.

> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> index 2437cd8ba6331..fe560f5c5408c 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 (!(dev->xdp_features & NETDEV_XDP_ACT_BASIC)) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "XDP is not supported on this queue format.");
> +		return -EOPNOTSUPP;
> +	}
> +

[Severity: High]
This is a pre-existing issue that this patch did not introduce, but the
patch adds another exit to the same gate. gve_xdp() calls
gve_verify_xdp_configuration() before it looks at the command, so the gate
also runs for teardown:

drivers/net/ethernet/google/gve/gve_main.c:gve_xdp() {
	err = gve_verify_xdp_configuration(dev, xdp->extack);
	if (err)
		return err;
	switch (xdp->command) {
	case XDP_SETUP_PROG:
		return gve_set_xdp(priv, xdp->prog, xdp->extack);
	case XDP_SETUP_XSK_POOL:
		if (xdp->xsk.pool)
			...
		else
			return gve_xsk_pool_disable(dev, xdp->xsk.queue_id);
	...
}

Several of the gate's conditions can become true after a successful bind:

- gve has no ndo_change_mtu, so the MTU can be raised above max_xdp_mtu
  while a zero-copy pool is bound.
- gve_set_features() refuses HW-GRO only when priv->xdp_prog is set, so
  HW-GRO can be turned on while only a pool is bound.
- The queue counts can be changed the same way.

When the AF_XDP socket is then closed:

xp_release_deferred()
  xp_clear_dev()
    __xp_clear_dev()
      xp_disable_drv_zc()
        gve_xdp()
          gve_verify_xdp_configuration()   /* returns -EOPNOTSUPP */

On failure, xp_disable_drv_zc() only does WARN(1, "Failed to disable
zero-copy!\n"). The core then goes on to xsk_clear_pool_at_qid(),
xdp_put_umem() and xp_destroy().

Because gve_xsk_pool_disable() never runs, clear_bit(qid, priv->xsk_pools),
xsk_pool_dma_unmap() and gve_unreg_xsk_pool() are all skipped. Won't
rx->xsk_pool and tx->xsk_pool still point at the freed pool when the RX
refill and XSK TX NAPI paths use them? The UMEM DMA mappings would also be
leaked.

dev_xdp_uninstall() follows the same pattern for programs. It does WARN_ON()
on the failed removal and then calls bpf_prog_put(), which leaves
priv->xdp_prog dangling.

The new xdp_features check cannot change between setup and teardown by
itself, since gve_setup_device() computes it only from queue_format. Even
so, would it be better to run the verification only for XDP_SETUP_PROG with
a non-NULL prog and for pool enable, so that teardown is never refused?

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