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

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?

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

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