Re: [PATCH net-next v2 13/15] gve: setup and teardown management interrupts

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The workqueue `priv->gve_wq` is destroyed before the management IRQ is freed in `gve_remove()`, leading to a use-after-free if the interrupt fires during device teardown.
- [High] `stats_report_task` is not canceled when freeing the stats report, leading to a NULL pointer dereference if the interface is reset while down.
--

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

gve: setup and teardown management interrupts
    
Introduce control ops to setup/teardown control plane IRQs. Admin
queue-specific functionality is moved to admin queue files.

> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> index 7b034b439cebb..8005373e91bde 100644
> --- a/drivers/net/ethernet/google/gve/gve_main.c
> +++ b/drivers/net/ethernet/google/gve/gve_main.c
> @@ -267,14 +267,6 @@ static void gve_free_stats_report(struct gve_priv *priv)
>  	priv->stats_report = NULL;
>  }

[Severity: High]
This is a pre-existing issue, but does this function fail to cancel
the stats_report_task?

gve_free_stats_report() calls timer_delete_sync(&priv->stats_report_timer),
but it does not call cancel_work_sync(&priv->stats_report_task).

If the user enables stats reporting while the interface is down, the timer
starts. If a reset is triggered while the interface is down, gve_reset() skips
gve_queues_stop() (where the task is normally canceled) and proceeds to
teardown.

gve_free_control_plane_resources() then calls gve_free_stats_report(), which
frees priv->stats_report and sets it to NULL.

If the stats_report_task was queued just before the timer was deleted, it would
execute asynchronously and call gve_handle_report_stats(), which unconditionally
dereferences priv->stats_report->stats.

Could this lead to a NULL pointer dereference?

[ ... ]

> @@ -499,6 +491,7 @@ static int gve_alloc_notify_blocks(struct gve_priv *priv)
>  
>  static void gve_teardown_notify_blocks(struct gve_priv *priv)
>  {
> +	const struct gve_ctrl_ops *ops = priv->ctrl_ops;
>  	int i;
>  
>  	if (!priv->ntfy_blocks)
> @@ -518,29 +511,24 @@ static void gve_teardown_notify_blocks(struct gve_priv *priv)
>  		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;
> -	}
> +	ops->teardown_mgmt_irq(priv);

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for the
management IRQ to fire after the workqueue is destroyed?

In gve_remove() in drivers/net/ethernet/google/gve/gve_main.c,
destroy_workqueue(priv->gve_wq) is called before gve_teardown_device(priv).

gve_teardown_device() eventually reaches this code to tear down the management
IRQ. Because the IRQ is still active and unmasked after destroy_workqueue(),
an interrupt could fire during this window.

If that happens, gve_mgmnt_intr() calls:
    queue_work(priv->gve_wq, &priv->service_task);

Could this result in a use-after-free regression on the destroyed workqueue
pointer?

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