Re: [PATCH net-next v5 09/12] 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:

Pre-existing issues:
- [High] Missing cancellation of stats_report_task leads to NULL pointer dereference on stats_report
--

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

gve: introduce new methods to handle IRQ doorbells

This commit introduces request_db_info and release_db_resources to
struct gve_ctrl_ops. It encapsulates the configuration of device resources
such as counter arrays and IRQ doorbell indices.

> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> index 619ef46a7b30a..a160d36f718b9 100644
> --- a/drivers/net/ethernet/google/gve/gve_main.c
> +++ b/drivers/net/ethernet/google/gve/gve_main.c

[ ... ]

> @@ -602,7 +606,6 @@ static int gve_setup_notify_blocks(struct gve_priv *priv)
>  	return err;
>  }
>  
> -
>  static void gve_free_control_plane_resources(struct gve_priv *priv)
>  {
>  	bitmap_free(priv->xsk_pools);
> @@ -611,9 +614,10 @@ static void gve_free_control_plane_resources(struct gve_priv *priv)
>  	kvfree(priv->ptype_lut_dqo);
>  	priv->ptype_lut_dqo = NULL;
>  
> -	gve_free_stats_report(priv);
> -	gve_free_notify_blocks(priv);
> +	gve_free_irq_db_indices(priv);
>  	gve_free_counter_array(priv);
> +	gve_free_notify_blocks(priv);
> +	gve_free_stats_report(priv);

[Severity: High]
This isn't a bug introduced by this patch, but could this lead to a NULL
pointer dereference when the uncancelled work item executes?

Looking at gve_free_stats_report(), it stops the timer and frees the
priv->stats_report memory, but it does not cancel the stats_report_task
work item.

If the network interface is down but the report-stats feature is enabled via
ethtool, gve_queues_stop() is never called during device removal or
suspend. This means the work item remains pending because the only
cancel_work_sync() for this task happens in gve_queues_stop().

When the device is later removed or suspended, the workqueue is flushed
(or executes concurrently), which allows gve_stats_report_task() to run
after the memory is freed. This eventually calls into
gve_handle_report_stats(), which dereferences the NULL pointer:

drivers/net/ethernet/google/gve/gve_main.c:gve_handle_report_stats() {
	struct stats *stats = priv->stats_report->stats;
	...
}

Should a call to cancel_work_sync(&priv->stats_report_task) be added to
gve_free_stats_report() to ensure the work item cannot execute after the
underlying memory is destroyed?

>  	gve_free_rss_config_cache(priv);
>  	gve_free_flow_rule_caches(priv);
>  }

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