Re: [PATCH net-next v2 08/15] gve: refactor gve_init_priv for reset path

[sashiko-bot@kernel.org]

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

New issues:
- [High] gve_reset_recovery error path leaves device in a state where subsequent ndo_open causes Use-After-Free and NULL pointer dereferences.

Pre-existing issues:
- [High] Device reset leaks XSK pool DMA mappings and silently breaks XDP sockets.
--

commit b96244d58e5385fb7dc71f1bd8136017d84f17d7
Author: Harshitha Ramamurthy <hramamurthy@google.com>

gve: refactor gve_init_priv for reset path

This commit refactors gve_init_priv() into targeted setup functions to bypass
full initialization during the reset path. It moves admin queue allocation and
device properties negotiation into gve_probe(). This prepares the driver for
adding mailbox mode control plane initialization in upcoming patches.

> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> index d693caed7e3d1..746ff69a28ddc 100644
> --- a/drivers/net/ethernet/google/gve/gve_main.c
> +++ b/drivers/net/ethernet/google/gve/gve_main.c
> @@ -2414,6 +2416,33 @@ static void gve_set_buf_sizes(struct gve_priv *priv)
>  		priv->header_buf_size = device_info->header_buf_size;
>  }
>  
> +static int gve_setup_device(struct gve_priv *priv)
> +{
> +	int err;
> +
> +	priv->xsk_pools = bitmap_zalloc(priv->rx_cfg.max_queues, GFP_KERNEL);

[Severity: High]
This is a pre-existing issue, but does this allocation cause an XDP socket
DMA mapping leak and silently break XDP sockets during a device reset?

When a device reset occurs, such as from a transmit timeout, the teardown
process frees the priv->xsk_pools bitmap without unmapping the active XDP
socket DMA mappings.

Then gve_reset_recovery() calls gve_setup_device() here, which allocates a
fresh, zeroed xsk_pools bitmap. When the interface is brought back up, the
driver reads the zeroed bitmap, forgetting which queues had active XSK pools.
This causes XDP sockets to silently stop receiving traffic and permanently
leaks the DMA mappings established by gve_xsk_pool_enable().

> +	if (!priv->xsk_pools) {
> +		err = -ENOMEM;
> +		goto err;
> +	}

[ ... ]

> @@ -2554,15 +2540,29 @@ static int gve_reset_recovery(struct gve_priv *priv, bool was_up)
>  {
>  	int err;
>  
> -	err = gve_init_priv(priv, true);
> -	if (err)
> +	err = gve_adminq_init(priv);
> +	if (err) {
> +		dev_err(&priv->pdev->dev,
> +			"Failed to alloc admin queue: err=%d\n", err);
>  		goto err;
> +	}
> +
> +	err = gve_setup_device(priv);
> +	if (err)
> +		goto err_free_adminq;
>  	if (was_up) {
>  		err = gve_open(priv->dev);
>  		if (err)
> -			goto err;
> +			goto err_free_device;
>  	}
>  	return 0;
> +
> +err_free_device:
> +	gve_teardown_device_resources(priv);
> +	bitmap_free(priv->xsk_pools);
> +	priv->xsk_pools = NULL;
> +err_free_adminq:
> +	gve_adminq_free(priv);

[Severity: High]
If gve_open() fails here, the error path tears down device resources (freeing
priv->ntfy_blocks) and the admin queue DMA pool via gve_adminq_free().

However, the net_device remains registered. If a user subsequently attempts to
bring the interface up, gve_open() is invoked again. Because gve_open()
currently does not check if resources are initialized, it proceeds to call
gve_queues_start().

Could this trigger a use-after-free in gve_adminq_issue_cmd() (dereferencing
priv->adminq from the destroyed pool) and a null pointer dereference in
gve_turnup() or gve_reg_xdp_info() (dereferencing priv->ntfy_blocks)?

I notice this issue is fixed later in the series by commit d54cafae24e9
("gve: simplify reset logic") which adds a gve_get_device_resources_ok()
check at the start of gve_open().

>  err:
>  	dev_err(&priv->pdev->dev, "Reset failed! !!! DISABLING ALL QUEUES !!!\n");

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