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

[Amaan Cheval]

On Tue, Nov 10, 2026 at 5:35 PM <netdev-bot+sashiko@xxxxxxxxxx> wrote:
>
> 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.

pw-bot: cr

v5 addresses this here:
  https://patchwork.kernel.org/series/1205920/

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