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

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] In `gve_remove()`, the workqueue is destroyed before interrupts are unregistered, leading to a Use-After-Free if an interrupt fires and queues work.
- [High] Missing state check in `gve_reset()` leads to Use-After-Free of `priv->adminq` when called concurrently or after system suspend/shutdown.

Pre-existing issues:
- [High] XDP rxq info is leaked when gve_queues_start fails during an in-progress reset, leaving dangling pointers in the network stack.
--

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

gve: simplify reset logic

This commit simplifies the reset logic in the GVE driver by removing
recursive resets, avoiding resets during teardown, and decomposing allocation
and setup steps.

> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> index 746ff69a28ddc..f94f3536be4c6 100644
> --- a/drivers/net/ethernet/google/gve/gve_main.c
> +++ b/drivers/net/ethernet/google/gve/gve_main.c
> @@ -1419,10 +1448,11 @@ static int gve_queues_start(struct gve_priv *priv,
>  reset:
>  	if (gve_get_reset_in_progress(priv))
>  		goto stop_and_free_rings;
> -	gve_reset_and_teardown(priv, true);
> -	/* if this fails there is nothing we can do so just ignore the return */
> -	gve_reset_recovery(priv, false);
> -	/* return the original error */
> +
> +	/* Attempt to reset. If reset is successful, gve_queues_start was
> +	 * successful.
> +	 */
> +	err = gve_reset(priv, false);
>  	return err;
>  stop_and_free_rings:
>  	gve_tx_stop_rings(priv, gve_num_tx_queues(priv));
>  	gve_rx_stop_rings(priv, priv->rx_cfg.num_queues);

[Severity: High]
This is a pre-existing issue, but in the stop_and_free_rings error path
inside gve_queues_start(), the rx arrays are freed via
gve_queues_mem_remove(priv) without unregistering the XDP rxq structures. 

Could this leak xdp_rxq info and leave dangling pointers in the network
stack?

[ ... ]

> @@ -2549,54 +2547,52 @@ 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) {
> -		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);
> -		}
> -		/* Clean up any remaining resources */
> -		gve_teardown_priv_resources(priv);
> +
> +	if (was_up) {
> +		gve_queues_stop(priv);
> +		gve_destroy_rings(priv);
> +		gve_unregister_qpls(priv);
>  	}

[Severity: High]
If a background task executes gve_reset() after gve_suspend() or
gve_shutdown() has already freed priv->adminq, wouldn't this unconditional
call to gve_destroy_rings(priv) write to the freed admin queue ring buffer?

The previous code appeared to verify that device rings were still okay
before attempting to tear them down.

[ ... ]

> @@ -2977,8 +2973,8 @@ static void gve_remove(struct pci_dev *pdev)
>  	void __iomem *reg_bar = priv->reg_bar0;
>  
>  	unregister_netdev(netdev);
> -	gve_teardown_priv_resources(priv);
>  	destroy_workqueue(priv->gve_wq);
> +	gve_teardown_device(priv);
>  	priv->ctrl_ops->unmap_db_bar(priv);
>  	free_netdev(netdev);
>  	pci_iounmap(pdev, reg_bar);

[Severity: Critical]
Since gve_teardown_device() is responsible for freeing the notify blocks and
interrupts, does tearing down the workqueue first expose us to a
use-after-free if gve_mgmnt_intr() fires and calls
queue_work(priv->gve_wq, &priv->service_task)?

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