Re: [PATCH net-next v4 09/15] gve: simplify reset logic

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Dangling pointers for `priv->tx` and `priv->rx` are left behind if `gve_create_rings()` fails during interface open, leading to a Use-After-Free.
- [High] The `gve_recover` function unconditionally forces the `service_task` work item to become enabled, breaking synchronization guarantees in power management and teardown paths and leading to a `WARN_ON_ONCE` or hardware fault.
--

--- Patch [9]: [PATCH net-next v4 09/15] gve: simplify reset logic ---
commit 49cb37f017cb334dfcf8209cde2a7c5d05f9e88e
Author: Joshua Washington <[email protected]>
Subject: gve: simplify reset logic

This commit simplifies the reset logic in the GVE driver by removing
recursive resets and restructuring the teardown and deallocation steps.
It ensures that the driver can execute a hardware reset even if control
plane operations fail, and consolidates the recovery path.

> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> --- a/drivers/net/ethernet/google/gve/gve_main.c
> +++ b/drivers/net/ethernet/google/gve/gve_main.c
> @@ -2515,93 +2543,81 @@ static int gve_init_priv(struct gve_priv *priv)
> [ ... ]
> +static int gve_recover(struct gve_priv *priv, bool setup_queues)
> +{
> +	bool service_task_enabled = false;
> +	int err;
> +
> +	err = gve_adminq_init(priv);
> +	if (err) {
> +		dev_err(&priv->pdev->dev,
> +			"Failed to alloc admin queue: err=%d\n", err);
> +		goto teardown_device;
> +	}
> +
> +	err = gve_adminq_verify_driver_compatibility(priv);
> +	if (err) {
> +		dev_err(&priv->pdev->dev,
> +			"Could not verify driver compatibility: err=%d\n", err);
> +		goto teardown_device;
> +	}
> +
> +	err = gve_setup_device(priv);
> +	if (err)
> +		goto teardown_device;
> +
> +	/* In the case of cascading failures, the disable count could be greater
> +	 * than 1. The service task must be enabled before starting queues.
> +	 */
> +	while (!service_task_enabled)
> +		service_task_enabled = enable_work(&priv->service_task);

[Severity: High]
Does this loop risk breaking the synchronization guarantees of power
management callbacks?

Functions like gve_suspend(), gve_shutdown(), and gve_remove() explicitly
synchronize and disable the background work by calling disable_work_sync()
to ensure the task does not execute while hardware resources are unavailable:

gve_suspend() {
    disable_work_sync(&priv->service_task);
    ...
    if (was_up)
        gve_close(priv->dev);
    ...
}

If gve_close() fails during one of these callbacks (for example, due to an
adminq timeout), it triggers a fallback to gve_reset() which in turn calls
gve_recover(). Won't this loop unconditionally undo the intentional
disable_work_sync(), leaving the device in suspend or shutdown with the
service task fully active?

Additionally, on the subsequent gve_resume(), gve_recover() is called again.
Since the disable depth would already be 0, wouldn't calling enable_work() 
here trigger a WARN_ON_ONCE(!offqd->disable) in the core workqueue subsystem?

> +
> +	if (setup_queues) {
> +		err = gve_open(priv->dev);

[Severity: High]
If gve_create_rings() fails during this nested gve_open() call, could it
leave dangling pointers for priv->tx and priv->rx?

When gve_create_rings() fails during an initial gve_open(), it triggers
gve_reset(). Because the interface is marked as starting, gve_reset()
performs cleanup via gve_queues_mem_remove() (which properly nullifies
priv->tx and priv->rx) and then calls gve_recover() to restart the queues.

Inside gve_recover(), this nested call to gve_open() allocates new ring
memory and updates the priv->tx and priv->rx pointers. If queue creation
fails again, gve_queues_start() detects that reset_in_progress is true (set
by the outer gve_reset) and jumps to the stop_and_free_rings label:

gve_queues_start() {
...
stop_and_free_rings:
    gve_tx_stop_rings(priv, gve_num_tx_queues(priv));
    gve_rx_stop_rings(priv, priv->rx_cfg.num_queues);
free_rings:
    gve_queues_mem_free(priv, tx_alloc_cfg, rx_alloc_cfg);
    return err;
}

The stop_and_free_rings path frees the memory using gve_queues_mem_free(),
but doesn't appear to nullify priv->tx and priv->rx like
gve_queues_mem_remove() does. Could this lead to a use-after-free if
another context like ethtool -S checks if (priv->tx) and dereferences
these pointers?

> +		if (err)
> +			goto disable_work;
> +	}
> +
> +	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=9