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

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

New issues:
- [High] The `while (!service_task_enabled)` loop in `gve_recover` forcefully enables `service_task`, overriding concurrent `disable_work_sync` calls from PM suspend, allowing the task to run while the device is suspended.

Pre-existing issues:
- [High] `gve_xsk_pool_enable` and `gve_xsk_pool_disable` unconditionally dereference `priv->xsk_pools`, which is NULL if the device failed to recover from a reset.
--

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

This commit simplifies the GVE reset logic by removing recursive resets,
avoiding resets during teardown, and separating control plane resource
allocation from device setup. Shared resources are now retained until the
hardware reset completes to prevent premature release.

> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> index 6f04eefd04166..578520e107930 100644
> --- a/drivers/net/ethernet/google/gve/gve_main.c
> +++ b/drivers/net/ethernet/google/gve/gve_main.c
> @@ -591,7 +591,22 @@ static void gve_free_notify_blocks(struct gve_priv *priv)
>  	priv->msix_vectors = NULL;
>  }
>  
> -static int gve_setup_device_resources(struct gve_priv *priv)
> +static void gve_free_control_plane_resources(struct gve_priv *priv)
> +{
> +	bitmap_free(priv->xsk_pools);
> +	priv->xsk_pools = NULL;

[Severity: High]
This is a pre-existing issue, but does the driver safely handle a NULL
xsk_pools bitmap if recovery fails?

If gve_recover() fails during a reset (for example, if gve_setup_device()
fails), priv->xsk_pools is left as NULL because it is freed here during
teardown.

Since the netdev remains registered, a subsequent AF_XDP socket bind from
userspace could call gve_xsk_pool_enable() or gve_xsk_pool_disable().
These functions unconditionally dereference the bitmap:

drivers/net/ethernet/google/gve/gve_main.c:gve_xsk_pool_enable() {
    ...
    set_bit(qid, priv->xsk_pools);
    ...
}

Could this lead to an immediate NULL pointer dereference if a user with
sufficient privileges configures AF_XDP on the failed device?

[ ... ]

> -static int gve_reset_recovery(struct gve_priv *priv, bool was_up)
> +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 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 err_free_adminq;
> +		goto teardown_device;
>  	}
>  
>  	err = gve_setup_device(priv);
>  	if (err)
> -		goto err_free_adminq;
> -	if (was_up) {
> +		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]
Could this loop override a concurrent suspend operation?

If a device reset (gve_reset) runs concurrently with power management
suspend (gve_suspend), gve_suspend increments the service task's disable
count via disable_work_sync() and waits for the rtnl_lock.

If gve_reset holds the rtnl_lock and executes gve_recover(), it drops the
disable count to 0 in this loop. Once gve_reset finishes, gve_suspend
acquires the lock, completes teardown, and suspends the device to D3.

Since the service_task is now enabled, couldn't it be scheduled to run while
the device is suspended? This might result in an MMIO read on a D3 state
device, causing a kernel panic or PCIe AER fatal error.

[ ... ]

> @@ -3017,16 +3025,14 @@ static int gve_suspend(struct device *dev)
>  	struct gve_priv *priv = netdev_priv(netdev);
>  	bool was_up = netif_running(priv->dev);
>  
> +	disable_work_sync(&priv->service_task);
> +
>  	priv->suspend_cnt++;

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