Re: [PATCH net-next v5 06/12] gve: simplify reset logic

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (1) · Medium: 0 · Low: 2

- [Low] The patch silently fixes a real pre-existing adminq DMA/dma_pool
  leak without a Fixes: tag and without disclosing it in the commit…
- [Low] Commit message goal 2 misdescribes the pre-patch defect and
  overstates the change.

Pre-existing issues:
- [High] The stats-report DMA buffer can be freed while
  priv->stats_report_task is queued or running, giving a NULL-pointer…

--

gve: simplify reset logic

This reworks the gve reset path: recursive resets and resets issued from
the teardown path are removed, control plane resource allocation is split
from device setup, and resources shared with the device are only freed
after gve_adminq_free() has reset the hardware.  Flow rule timeout
handling moves from the adminq layer into the ethtool add/delete entry
points.

A few questions on the changelog and on the stats report worker below.

[Severity: Low]
The changelog says:

    2) Removing resets during teardown. ... As it stands, if
       `deconfigure_device_resources` fails, for example, GVE will preempt
       its reset with another reset without actually invoking a hardware
       reset, which could actually help with recovery.

Is the "without actually invoking a hardware reset" part accurate?

gve_trigger_reset() was just a call to gve_adminq_release(), and that
function does poke the reset registers and wait for the device:

	iowrite32be(GVE_DRIVER_STATUS_RESET_MASK, &priv->reg_bar0->driver_status);
	while (!(ioread32be(&priv->reg_bar0->device_status)
			& GVE_DEVICE_STATUS_DEVICE_IS_RESET)) {

(older revisions zero adminq_pfn and poll it instead).  So a hardware
reset did happen on that path; what was missing afterwards was the
adminq/dma_pool teardown, see the next comment.

Also, is "Removing resets during teardown" fully true after this change?
The new gve_close() still has a reset label calling gve_reset(priv, true)
when gve_destroy_rings() or gve_unregister_qpls() fails, and both
gve_shutdown() and gve_suspend() call gve_close(), so queue teardown can
still perform a full hardware reset plus recovery before the caller tears
the device down again.  Could the changelog mention that remaining path?

> diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/ethernet/google/gve/gve_adminq.c
> index a62cb7a921d07..901673d2e2648 100644
> --- a/drivers/net/ethernet/google/gve/gve_adminq.c
> +++ b/drivers/net/ethernet/google/gve/gve_adminq.c

[ ... ]

> @@ -394,7 +394,6 @@ void gve_adminq_release(struct gve_priv *priv)
>  	}
>  	gve_clear_device_rings_ok(priv);
>  	gve_clear_device_resources_ok(priv);
> -	gve_clear_admin_queue_ok(priv);
>  }
>  
>  void gve_adminq_free(struct gve_priv *priv)

[Severity: Low]
Does this one-line removal fix a pre-existing leak of the adminq buffer
and its dma_pool?

Before this patch gve_adminq_release() cleared admin_queue_ok, and
gve_adminq_free() starts with an early return keyed off that same bit:

	void gve_adminq_free(struct gve_priv *priv)
	{
		if (!gve_get_admin_queue_ok(priv))
			return;
		gve_adminq_release(priv);
		dma_pool_free(priv->adminq_pool, priv->adminq, priv->adminq_bus_addr);
		dma_pool_destroy(priv->adminq_pool);

so every caller of gve_trigger_reset() skipped the dma_pool_free() and
dma_pool_destroy() that followed:

gve_reset_and_teardown()
  gve_trigger_reset()             /* gve_adminq_release(), clears admin_queue_ok */
  gve_teardown_priv_resources()
    gve_adminq_free()             /* early return, pool and buffer leaked */

The same happened for the three error branches in
gve_teardown_device_resources().  Would it make sense to split this into
its own patch with a Fixes: tag, or at least mention the leak in the
changelog?  As written the fix is buried in a ~350 line refactor and
stable tooling has nothing to pick up.

[ ... ]

> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> index 16ce55c6d74f6..acd09e9f416c1 100644
> --- a/drivers/net/ethernet/google/gve/gve_main.c
> +++ b/drivers/net/ethernet/google/gve/gve_main.c

[ ... ]

> @@ -1451,41 +1494,16 @@ static int gve_open(struct net_device *dev)
>  	return 0;
>  }
>  
> -static int gve_queues_stop(struct gve_priv *priv)
> +static void gve_queues_stop(struct gve_priv *priv)
>  {
> -	int err;
> +	gve_unreg_xdp_info(priv);
> +	gve_drain_page_cache(priv);
>  
> -	netif_carrier_off(priv->dev);
> -	if (gve_get_device_rings_ok(priv)) {
> -		gve_turndown(priv);
> -		gve_drain_page_cache(priv);
> -		err = gve_destroy_rings(priv);
> -		if (err)
> -			goto err;
> -		err = gve_unregister_qpls(priv);
> -		if (err)
> -			goto err;
> -		gve_clear_device_rings_ok(priv);
> -	}
>  	timer_delete_sync(&priv->stats_report_timer);
> -
> -	gve_unreg_xdp_info(priv);
> +	cancel_work_sync(&priv->stats_report_task);

[Severity: High]
This isn't a bug introduced by this patch, since before it nothing
cancelled stats_report_task at all, but the new cancel_work_sync() only
runs on the interface-up path.  Should it live in gve_free_stats_report()
or gve_teardown_device() instead so it covers the down-interface cases?

gve_free_stats_report() only synchronizes the timer before freeing the
coherent buffer:

	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;

An already queued stats_report_task survives that.  The timer can be
armed from ethtool with the interface down, there is no netif_running()
or resource-state check in gve_set_priv_flags():

	if (flags & BIT(0)) {
		mod_timer(&priv->stats_report_timer,
			  round_jiffies(jiffies +
				msecs_to_jiffies(priv->stats_report_timer_period)));
	}

and gve_stats_report_timer() -> gve_stats_report_schedule() queues
priv->stats_report_task, whose gve_handle_report_stats() writes into the
buffer:

	be64_add_cpu(&priv->stats_report->written_count, 1);

Can that write land on a NULL priv->stats_report, or into memory
dma_free_coherent() has already released?  The paths that skip
gve_queues_stop() look like:

gve_reset() with was_up == false   /* ethtool --reset all on a down iface */
  disable_work(&priv->service_task)  /* stats_report_task not disabled */
  gve_teardown_device()
    gve_free_control_plane_resources()
      gve_free_stats_report()

> @@ -2514,30 +2542,7 @@ static int gve_init_priv(struct gve_priv *priv)
>  	return 0;
>  }
>  
> -static void gve_teardown_priv_resources(struct gve_priv *priv)
> -{
> -	gve_teardown_device_resources(priv);
> -	gve_adminq_free(priv);
> -	bitmap_free(priv->xsk_pools);
> -	priv->xsk_pools = NULL;
> -}
> -
> -static void gve_trigger_reset(struct gve_priv *priv)
> -{
> -	/* Reset the device by releasing the AQ */
> -	gve_adminq_release(priv);
> -}
> -
> -static void gve_reset_and_teardown(struct gve_priv *priv, bool was_up)
> -{
> -	gve_trigger_reset(priv);
> -	/* With the reset having already happened, close cannot fail */
> -	if (was_up)
> -		gve_close(priv->dev);
> -	gve_teardown_priv_resources(priv);
> -}
> -
> -static int gve_reset_recovery(struct gve_priv *priv, bool was_up)
> +static int gve_recover(struct gve_priv *priv, bool setup_queues)
>  {
>  	int err;
>  
> @@ -2545,62 +2550,66 @@ static int gve_reset_recovery(struct gve_priv *priv, bool was_up)

[ ... ]

> -int gve_reset(struct gve_priv *priv, bool attempt_teardown)
> +int gve_reset(struct gve_priv *priv, bool skip_queue_setup)
>  {
>  	bool was_up = netif_running(priv->dev);
>  	int err;
>  
> +	if (gve_get_reset_in_progress(priv))
> +		return 0;
> +
>  	dev_info(&priv->pdev->dev, "Performing reset\n");
>  	gve_clear_do_reset(priv);
>  	gve_set_reset_in_progress(priv);
> -	/* If we aren't attempting to teardown normally, just go turndown and
> -	 * reset right away.
> -	 */
> -	if (!attempt_teardown) {
> +
> +	if (was_up) {
>  		gve_turndown(priv);
> -		gve_reset_and_teardown(priv, was_up);
> -	} else {
> -		/* Otherwise attempt to close normally */
> -		if (was_up) {
> -			err = gve_close(priv->dev);
> -			/* If that fails reset as we did above */
> -			if (err)
> -				gve_reset_and_teardown(priv, was_up);
> +		if (gve_get_device_rings_ok(priv)) {
> +			gve_clear_device_rings_ok(priv);
> +			gve_destroy_rings(priv);
> +			gve_unregister_qpls(priv);
>  		}
> -		/* Clean up any remaining resources */
> -		gve_teardown_priv_resources(priv);
> +		gve_queues_stop(priv);
>  	}
>  
> -	/* Set it all back up */
> -	err = gve_reset_recovery(priv, was_up);
> +	disable_work(&priv->service_task);
> +	gve_teardown_device(priv);
> +	gve_queues_mem_remove(priv);

Following the comment above: with was_up == false, gve_queues_stop() is
not called here, so the new cancel_work_sync(&priv->stats_report_task) is
skipped while gve_teardown_device() frees the stats report buffer.  Only
service_task is covered by disable_work() here.

[ ... ]

> @@ -2981,7 +2991,8 @@ static void gve_remove(struct pci_dev *pdev)
>  	void __iomem *reg_bar = priv->reg_bar0;
>  
>  	unregister_netdev(netdev);
> -	gve_teardown_priv_resources(priv);
> +	disable_work_sync(&priv->service_task);
> +	gve_teardown_device(priv);
>  	destroy_workqueue(priv->gve_wq);

Same question for removal of a down interface: stats_report_task is not
synchronized before gve_teardown_device() frees priv->stats_report.

[ ... ]

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