Re: [PATCH net-next v2 12/15] gve: introduce new methods to handle IRQ doorbells

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] DMA memory for irq_db_indices and counter_array is freed before the hardware is guaranteed to be stopped, allowing the hardware to corrupt kernel memory if admin queue commands time out or fail.
--

commit e39d080d71706978b42ac097e883fbae5157b9ec
Author: Joshua Washington <joshwash@google.com>

gve: introduce new methods to handle IRQ doorbells

This commit introduces request_db_info and free_db_resources to
struct gve_ctrl_ops to encapsulate the configuration of device resources
(counter arrays and IRQ doorbell indices). It also manages GQ ring counters
in request_db_info.

> diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/ethernet/google/gve/gve_adminq.c
> index d814108deeefe..259dcd617216c 100644
> --- a/drivers/net/ethernet/google/gve/gve_adminq.c
> +++ b/drivers/net/ethernet/google/gve/gve_adminq.c
[ ... ]
> @@ -1719,3 +1719,74 @@ void gve_adminq_set_num_queues(struct gve_priv *priv)
[ ... ]
> +int gve_adminq_request_db_info(struct gve_priv *priv)
> +{
[ ... ]
> +	err = gve_adminq_configure_device_resources(priv,
> +						    priv->counter_array_bus,
> +						    priv->num_event_counters,
> +						    priv->irq_db_indices_bus,
> +						    priv->num_ntfy_blks);
> +	if (unlikely(err)) {
> +		dev_err(&priv->pdev->dev,
> +			"could not setup device_resources: err=%d\n", err);
> +		err = -ENXIO;
> +		goto abort_with_irq_db_indices;
> +	}
> +
> +	for (i = 0; i < priv->num_ntfy_blks; i++)
> +		priv->ntfy_blocks[i].irq_db_index =
> +			&priv->irq_db_indices[i].index;
> +	return 0;
> +
> +abort_with_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;
> +abort_with_counter_array:
> +	gve_free_counter_array(priv);
> +	return err;
> +}

[Severity: High]
Does this error path introduce a use-after-free window? 

If the configure command in gve_adminq_configure_device_resources() times
out (e.g. returns -ETIME), the hardware might still be running and attempting
to access the memory.

Since we immediately free the DMA memory via dma_free_coherent() and
gve_free_counter_array() before the hardware is guaranteed to be stopped, can
the hardware overwrite reallocated kernel memory?

> +
> +void gve_adminq_free_db_resources(struct gve_priv *priv)
> +{
> +	int err;
> +
> +	/* Log error in deconfigure device, but don't fail. This is only ever
> +	 * called as a reset is about to be triggered, so it would be redundant
> +	 * to trigger a reset.
> +	 */
> +	err = gve_adminq_deconfigure_device_resources(priv);
> +	if (err)
> +		dev_err(&priv->pdev->dev,
> +			"Could not deconfigure device resources: err=%d\n",
> +			err);
> +
> +	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;
> +	gve_free_counter_array(priv);
> +}

[Severity: High]
Could freeing the DMA memory here cause memory corruption? 

In an earlier patch in this series ('gve: simplify reset logic'), the teardown
logic was explicitly fixed to delay freeing shared resources until after
gve_adminq_free() triggers a device reset.

However, gve_teardown_device() executes gve_teardown_control_plane_resources()
(which now calls ops->free_db_resources()) before gve_adminq_free():

drivers/net/ethernet/google/gve/gve_main.c:gve_teardown_device() {
	...
	gve_teardown_notify_blocks(priv);
	gve_teardown_control_plane_resources(priv);
	gve_adminq_free(priv);
	...
}

If the deconfigure command in gve_adminq_deconfigure_device_resources() times
out or fails, the active hardware might continue writing to the freed DMA
memory since the device hasn't been reset yet.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260602235900.3050782-1-hramamurthy@google.com?part=12