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

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

Pre-existing issues:
- [High] AF_XDP socket pool bindings are silently detached and forgotten upon driver reset.
- [High] Background tasks (`stats_report_task`, `service_task`) are not canceled before the resources they depend on are freed, leading to a Use-After-Free and kernel panic.
- [High] Null Pointer Dereference in `gve_xsk_pool_enable` and `gve_xsk_pool_disable` if device setup fails.
--

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

gve: refactor gve_init_priv for reset path

The driver does not need to renegotiate all properties with
the device on a reset since those should stay constant through
a reset. Hence change gve_init_priv() into a method that only
sets these properties into the priv structure and hence needs
to be only called once during gve_probe().

> --- a/drivers/net/ethernet/google/gve/gve_main.c
> +++ b/drivers/net/ethernet/google/gve/gve_main.c
[ ... ]
> +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 allocation silently
detach and forget existing AF_XDP socket pool bindings upon a driver reset?

If a driver reset occurs, for example due to a Tx timeout, the recovery path
calls gve_setup_device() which unconditionally allocates a new, zeroed
xsk_pools bitmap.

When queues restart, gve_reg_xdp_info() will find no registered pools and
configure the hardware for standard traffic instead of zero-copy. Could this
cause existing AF_XDP applications to stop receiving and sending packets
without notification?

> +	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;

[Severity: High]
This is a pre-existing issue, but can this lead to a NULL pointer
dereference if the device setup fails?

If gve_setup_device() fails during a reset recovery and frees the bitmap,
xsk_pools is set to NULL. If a privileged user then attempts to attach or
detach an AF_XDP socket before the device is fully recovered:

gve_xsk_pool_enable()
    set_bit(qid, priv->xsk_pools);

Would this unconditionally dereference the NULL priv->xsk_pools pointer?

> +err:
> +	return err;
> +}
[ ... ]
> -err_free_xsk_bitmap:
> -	bitmap_free(priv->xsk_pools);
> -	priv->xsk_pools = NULL;
> -err:
> -	gve_adminq_free(priv);
> -	return err;
>  }
>  
>  static void gve_teardown_priv_resources(struct gve_priv *priv)

[Severity: High]
This is a pre-existing issue not touched by this patch, but does the driver
have a use-after-free during module removal?

In gve_remove(), gve_teardown_priv_resources() is called before
destroy_workqueue(). This frees priv->stats_report.

Because the driver does not cancel the background tasks before freeing these
resources, if stats_report_task is queued, destroy_workqueue() will flush
and execute it. Could this cause gve_handle_report_stats() to dereference
the freed stats_report pointer and panic?

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