Re: [PATCH net-next v7 08/12] gve: split up notify block allocation and setup paths

[Joshua Washington]

On Fri, Sep 18, 2026 at 12:57 AM <netdev-bot+sashiko@xxxxxxxxxx> wrote:
>
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider:
>
> Critical: 0 · High: 1 · Medium: 0 · Low: 1
>
> - [High] Reset path now frees the notify-block MSI-X IRQs and zeroes
>   block->irq *before* the NAPI/queue teardown that consumes block->irq,…
> - [Low] Reported for completeness, but I could not confirm a functional
>   defect: the concern claims the new alloc/setup/teardown/free split…
>
> --
>
> gve: split up notify block allocation and setup paths
>
> This splits gve_alloc_notify_blocks() into an allocation half and a new
> gve_setup_notify_blocks()/gve_teardown_notify_blocks() pair so that MSI-X
> vectors can be requested after the IRQ doorbell offsets are known. New
> irq_requested and mgmt_irq_requested flags track which irqs were taken, and
> gve_reset_device() now tears the notify blocks down.
>
> > diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> > index 8cd82d92d3b09..7119b5a9ae41b 100644
> > --- a/drivers/net/ethernet/google/gve/gve_main.c
> > +++ b/drivers/net/ethernet/google/gve/gve_main.c
> > @@ -429,6 +429,24 @@ int gve_napi_poll_dqo(struct napi_struct *napi, int budget)
> >       return work_done;
> >  }
> >
> > +static void gve_free_notify_blocks(struct gve_priv *priv)
> > +{
> > +     pci_disable_msix(priv->pdev);
> > +     if (priv->irq_db_indices) {
> > +             dma_free_coherent(&priv->pdev->dev,
> > +                               priv->num_ntfy_blks *
> > +                               sizeof(*priv->irq_db_indices),
> > +                               priv->irq_db_indices,
> > +                               priv->irq_db_indices_bus);
> > +             priv->irq_db_indices = NULL;
> > +     }
> > +
> > +     kvfree(priv->ntfy_blocks);
> > +     priv->ntfy_blocks = NULL;
> > +     kvfree(priv->msix_vectors);
> > +     priv->msix_vectors = NULL;
> > +}
> > +
> >  static const struct cpumask *gve_get_node_mask(struct gve_priv *priv)
> >  {
> >       if (priv->numa_node == NUMA_NO_NODE)
>
> [ ... ]
>
> > @@ -497,15 +504,65 @@ static int gve_alloc_notify_blocks(struct gve_priv *priv)
>
> [ ... ]
>
> > +static void gve_teardown_notify_blocks(struct gve_priv *priv)
> > +{
> > +     int i;
> > +
> > +     if (!priv->ntfy_blocks)
> > +             return;
>
> [Severity: Low]
>
> This isn't a bug in the current tree, but the early return here keys on
> priv->ntfy_blocks while the management vector release further down depends
> only on priv->msix_vectors and priv->mgmt_irq_requested:
>
>         if (priv->mgmt_irq_requested) {
>                 free_irq(priv->msix_vectors[priv->mgmt_msix_idx].vector, priv);
>                 priv->mgmt_irq_requested = false;
>         }
>
> All three gve_free_notify_blocks() call sites currently run after
> gve_teardown_notify_blocks() (or before any request_irq()), so
> priv->ntfy_blocks and priv->msix_vectors are still valid whenever the
> teardown runs and nothing is leaked.
>
> Would it be worth guarding each resource with the flag that describes it,
> so the ordering contract is expressed in the code rather than left to the
> callers, given the mailbox mode is meant to reuse this skeleton?

The early return was a naive attempt to keep the indent levels down.
Will reorder the code to check for mgmt interrupt first.

>
> > +
> > +     for (i = 0; i < priv->num_ntfy_blks; i++) {
> > +             struct gve_notify_block *block = &priv->ntfy_blocks[i];
> > +
> > +             if (!block->irq_requested)
> > +                     continue;
> > +
> > +             irq_set_affinity_hint(priv->msix_vectors[i].vector,
> > +                                   NULL);
> > +             free_irq(priv->msix_vectors[i].vector, block);
> > +             block->irq = 0;
> > +             block->irq_requested = false;
> > +     }
> > +
> > +     if (priv->mgmt_irq_requested) {
> > +             free_irq(priv->msix_vectors[priv->mgmt_msix_idx].vector, priv);
> > +             priv->mgmt_irq_requested = false;
> > +     }
> > +}
>
> [ ... ]
>
> > @@ -761,6 +770,7 @@ static void gve_teardown_control_plane_resources(struct gve_priv *priv)
> >   */
> >  static void gve_reset_device(struct gve_priv *priv)
> >  {
> > +     gve_teardown_notify_blocks(priv);
> >       gve_teardown_control_plane_resources(priv);
> >       gve_adminq_free(priv);
> >  }
>
> [Severity: High]
>
> Can this end up calling disable_irq(0) on the reset path?
>
> gve_reset() has this ordering:
>
>         disable_work(&priv->service_task);
>         gve_reset_device(priv);
>         gve_queues_stop(priv);
>         gve_queues_mem_remove(priv);
>         gve_free_control_plane_resources(priv);
>
> With the new first statement in gve_reset_device(), the vectors are
> released and block->irq is cleared before gve_queues_stop() runs:
>
>                 free_irq(priv->msix_vectors[i].vector, block);
>                 block->irq = 0;
>                 block->irq_requested = false;
>
> gve_queues_stop() then walks gve_tx_stop_rings()/gve_rx_stop_rings(), and
> the per-ring stop only checks that the ring is still attached to a block,
> which gve_turndown() does not clear:
>
> drivers/net/ethernet/google/gve/gve_rx.c:gve_rx_stop_ring_gqi() {
>         if (!gve_rx_was_added_to_block(priv, idx))
>                 return;
>
>         gve_remove_napi(priv, ntfy_idx);
>         ...
> }
>
> and gve_remove_napi() uses block->irq without validating it:
>
> drivers/net/ethernet/google/gve/gve_utils.c:gve_remove_napi() {
>         struct gve_notify_block *block = &priv->ntfy_blocks[ntfy_idx];
>
>         disable_irq(block->irq);
>         netif_napi_del_locked(&block->napi);
> }
>
> gve_rx_stop_ring_dqo(), gve_tx_stop_ring_gqi() and gve_tx_stop_ring_dqo()
> reach gve_remove_napi() the same way.
>
> Before this patch the irqs were freed in gve_free_notify_blocks(), reached
> from gve_free_control_plane_resources(), which gve_reset() calls after
> gve_queues_stop(), so block->irq was still the real vector at that point.
>
> On x86 virq 0 has a descriptor created during early boot, so disable_irq(0)
> would mask that unrelated line and increment its disable depth once per
> still-attached queue with no matching enable_irq(); where no descriptor
> exists the call returns -EINVAL and the intended synchronization against
> the driver's own vector is simply skipped.
>
> The reset path is reachable from the service task on a device-requested
> reset and from gve_tx_timeout() -> gve_schedule_reset(), so it can be
> driven by ordinary queue stalls rather than only by administrative action.
>
> Would keeping gve_teardown_notify_blocks() after gve_queues_stop(), or
> skipping the disable_irq() when block->irq is 0, address this?
>
> The same ordering is still in place at the end of the series, so it does
> not look like a later patch fixes it.

A few things that need to be changed here:

1) gve_teardown_notify_blocks() does not need to happen as part of
gve_reset_device(). It should be in gve_teardown_device() instead,
after the device reset occurs.
2) gve_remove_napi() should guard against block->irq_reuqested
3) block->irq = 0 should be removed

Will make these changes in v8.

>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260916165533.2187434-1-hramamurthy%40google.com

-- Josh