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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing cancel_work_sync() for stats_report_task leads to NULL Pointer Dereference during device teardown.
--

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

    gve: split up notify block allocation and setup paths
    
    Before this patch, notify block allocation and setup occurred in the same
    method. This all occurred before gve_adminq_configure_device_resources,
    which populates the irq_db_indicies array, a DMA region with BAR offsets
    for MSI-X vectors.

> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> index cc27141240fd1..d2f974a0cc877 100644
> --- a/drivers/net/ethernet/google/gve/gve_main.c
> +++ b/drivers/net/ethernet/google/gve/gve_main.c

[ ... ]

> @@ -749,6 +758,7 @@ static void gve_teardown_control_plane_resources(struct gve_priv *priv)
>  
>  static void gve_teardown_device(struct gve_priv *priv)
>  {
> +	gve_teardown_notify_blocks(priv);
>  	gve_teardown_control_plane_resources(priv);

[Severity: High]
This is a pre-existing issue, but does the device teardown path properly
cancel the stats_report_task?

If a user enables stats reporting while the interface is down,
stats_report_timer is armed and enqueues stats_report_task into the
workqueue. If the device is subsequently unbound, gve_teardown_device()
calls gve_teardown_control_plane_resources(), which eventually calls
gve_free_stats_report().

In drivers/net/ethernet/google/gve/gve_main.c:gve_free_stats_report():

	timer_delete_sync(&priv->stats_report_timer);
	dma_free_coherent(&priv->pdev->dev, priv->stats_report_len,
			  priv->stats_report, priv->stats_report_bus);
	priv->stats_report = NULL;

This deletes the timer and frees the memory, but leaves the pending task in
the workqueue.

Then gve_remove() continues and flushes the workqueue:

	gve_teardown_device(priv);
	destroy_workqueue(priv->gve_wq);

When the workqueue is flushed, stats_report_task will execute
gve_handle_report_stats(), which dereferences priv->stats_report:

void gve_handle_report_stats(struct gve_priv *priv)
{
	struct stats *stats = priv->stats_report->stats;

Could this lead to a NULL pointer dereference? Should cancel_work_sync() be
called on stats_report_task before freeing priv->stats_report?

>  	gve_adminq_free(priv);
>  	/*

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