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

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

Pre-existing issues:
- [High] AF_XDP zero-copy socket pool bindings are broken and their DMA mappings are leaked across device resets.
- [High] Concurrent use-after-free and NULL pointer dereference of `priv->stats_report` during `gve_stats_report_task`.
--

--- Patch [8]: [PATCH net-next v3 08/15] gve: refactor gve_init_priv for reset path ---
commit 02971463a0e35f7deed56d888e3c49b020365965
Author: Harshitha Ramamurthy <[email protected]>
gve: refactor gve_init_priv for reset path

This patch refactors device initialization into distinct steps by introducing
gve_setup_device() and gve_adminq_init(). It reorganizes device properties
configuration to ensure gve_init_priv() is only called once, preparing the
driver for upcoming mailbox mode additions.

> 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
[ ... ]
> @@ -2414,6 +2414,35 @@ 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->num_registered_pages = 0;
> +
> +	priv->xsk_pools = bitmap_zalloc(priv->rx_cfg.max_queues, GFP_KERNEL);

[Severity: High]
This isn't a bug introduced by this patch, but does this unconditionally
overwrite the existing AF_XDP zero-copy socket pool bindings across a reset?

When a reset occurs (e.g., via MTU change or watchdog timeout),
gve_setup_device() re-allocates priv->xsk_pools as a new, zeroed bitmap. The
old bitmap is freed during teardown, but the previously bound XSK pools do
not appear to be restored.

Consequently, when queues are restarted, wouldn't gve_get_xsk_pool() return
NULL, causing XDP zero-copy registration to be skipped and fall back to
standard memory models (PAGE_SHARED/PAGE_POOL)?

> +	if (!priv->xsk_pools) {
> +		err = -ENOMEM;
> +		goto err;
> +	}
> +
> +	gve_set_netdev_xdp_features(priv);
> +	if (!gve_is_gqi(priv))
> +		priv->dev->xdp_metadata_ops = &gve_xdp_metadata_ops;
> +
> +	err = gve_setup_device_resources(priv);
> +	if (err)
> +		goto err_free_xsk_bitmap;
> +
> +	return 0;
> +
> +err_free_xsk_bitmap:
> +	bitmap_free(priv->xsk_pools);
> +	priv->xsk_pools = NULL;
> +err:
> +	return err;
> +}

[Severity: High]
This is a pre-existing issue, but since this patch refactors the device setup
and reset recovery paths, could there be a race condition when tearing down
the stats report?

In gve_free_stats_report(), the stats timer is deleted and the stats_report
DMA memory is freed. However, it does not call
cancel_work_sync(&priv->stats_report_task).

If the gve_stats_report_task executes asynchronously on priv->gve_wq during
or immediately after the memory is freed, could it lead to a use-after-free
or NULL pointer dereference when accessing priv->stats_report->stats in
gve_handle_report_stats()?

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