Re: [PATCH net-next v6 06/12] gve: simplify reset logic

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 2 (1) · Medium: 1 · Low: 0

- [High] Inconsistent work quiescing across the new gve_teardown_device()
  call sites leaves priv->stats_report_task able to run after…
- [High] The new reset/close flow releases data-plane DMA before the
  hardware reset that revokes the device's access to it.
- [Medium] gve_mgmnt_intr() now returns IRQ_NONE without queueing
  priv->service_task whenever GVE_PRIV_FLAGS_DEVICE_RESOURCES_OK is…

Pre-existing issues:
- [High] After gve_reset() fails to recover (gve_recover() ->
  teardown_device), the netdev is left administratively UP (nothing…

--

Here is the report.

gve: simplify reset logic

This reworks the gve reset path: recursive resets and resets during
teardown are removed, allocation is split from device setup, and a single
gve_teardown_device() / gve_recover() pair replaces the old
gve_reset_and_teardown() / gve_reset_recovery() helpers.

A few questions on the new teardown ordering and work-item quiescing
below.

> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> index 2fe280cf7e680..87388856ff6c5 100644
> --- a/drivers/net/ethernet/google/gve/gve_main.c
> +++ b/drivers/net/ethernet/google/gve/gve_main.c
> @@ -270,6 +270,10 @@ static irqreturn_t gve_mgmnt_intr(int irq, void *arg)
>  {
>  	struct gve_priv *priv = arg;
>  
> +	/* Device resources are not okay; consider the interrupt spurious. */
> +	if (!gve_get_device_resources_ok(priv))
> +		return IRQ_NONE;
> +
>  	queue_work(priv->gve_wq, &priv->service_task);
>  	return IRQ_HANDLED;
>  }

[Severity: Medium]
Can this drop a management notification instead of latching it?

The mgmt vector is live from gve_alloc_notify_blocks()/setup_mgmt_irq()
until it is released, but device_resources_ok is only set at the end of
gve_setup_control_plane_resources() and is cleared at the very start of
gve_teardown_control_plane_resources(). So for the whole teardown window
inside gve_teardown_device(), and for gve_recover()'s
adminq_init/verify/setup_device phase, this returns IRQ_NONE without
queueing the service task.

gve_service_task() is the only reader of reg_bar0->device_status (it feeds
gve_handle_status() for the device-requested-reset bit and the link
handling), and the only other re-sample point is the
queue_work(priv->gve_wq, &priv->service_task) at the end of
gve_queues_start(). When gve_recover() runs with setup_queues == false
(was_up false, or gve_reset(priv, true) from gve_close()'s reset path),
gve_open() is never called:

	if (setup_queues) {
		err = gve_open(priv->dev);

Is a device reset request or link transition asserted in that window
recovered anywhere, or is it lost until the next unrelated open?

Since gve_reset() already brackets the teardown with
disable_work()/enable_work(&priv->service_task), would keeping the
unconditional queue_work() here be equivalent for exclusion purposes
without discarding the notification? Also, for a dedicated (non-shared)
MSI-X vector, repeatedly returning IRQ_NONE feeds the core spurious
interrupt detector.

[ ... ]

> -static void gve_trigger_reset(struct gve_priv *priv);
> -
> -static void gve_teardown_device_resources(struct gve_priv *priv)
> +/**
> + * gve_teardown_control_plane_resources() - Request the device to release any
> + * shared allocated resources.

[ ... ]

> +static void gve_teardown_device(struct gve_priv *priv)
> +{
> +	gve_teardown_control_plane_resources(priv);
> +	gve_adminq_free(priv);
> +	/*
> +	 * Free any resources shared with the device only after we have a
> +	 * guarantee that the device will not try to access such resources.
> +	 * Device commands in gve_teardown_control_plane_resources can fail, in
> +	 * which case, device resources won't be relinquished until
> +	 * gve_adminq_free is called to trigger a device reset.
> +	 */
> +	gve_free_control_plane_resources(priv);
> +}
> +

[Severity: High]
Is priv->stats_report_task guaranteed to be quiesced by the time
gve_free_control_plane_resources() -> gve_free_stats_report() frees the
coherent buffer?

gve_free_stats_report() only stops the timer and frees:

	if (!priv->stats_report)
		return;

	timer_delete_sync(&priv->stats_report_timer);
	dma_free_coherent(&priv->pdev->dev, priv->stats_report_len,
			  priv->stats_report, priv->stats_report_bus);
	priv->stats_report = NULL;

The only cancel_work_sync(&priv->stats_report_task) added by this patch is
in gve_queues_stop(), which the teardown paths only reach when the
interface was up (gve_reset() calls it under if (was_up); gve_remove(),
gve_shutdown() and gve_suspend() reach it only via gve_close()).

gve_set_priv_flags() in gve_ethtool.c arms the timer with no
netif_running() check:

	if (flags & BIT(0)) {
		mod_timer(&priv->stats_report_timer,
			  round_jiffies(jiffies +
					msecs_to_jiffies(priv->stats_report_timer_period)));
	}

so with report-stats enabled on a down interface the chain
gve_stats_report_timer() -> gve_stats_report_schedule() ->
queue_work(priv->gve_wq, &priv->stats_report_task) can leave the work
pending across gve_teardown_device(). gve_handle_report_stats() then does:

	struct stats *stats = priv->stats_report->stats;
	...
	be64_add_cpu(&priv->stats_report->written_count, 1);

which is either a write into the just-freed DMA coherent buffer or a NULL
dereference once priv->stats_report has been set to NULL. In gve_remove()
the pending item is only drained by destroy_workqueue(), i.e. after
gve_teardown_device() has already freed it.

Would it be simpler to do the cancel_work_sync(&priv->stats_report_task)
inside gve_free_stats_report() (or in gve_teardown_device()) so every call
site is covered regardless of whether the interface was up?

[ ... ]

> @@ -1435,6 +1472,12 @@ static int gve_open(struct net_device *dev)
>  	struct gve_priv *priv = netdev_priv(dev);
>  	int err;
>  
> +	if (!gve_get_device_resources_ok(priv)) {
> +		dev_err(&priv->pdev->dev,
> +			"Attempting to open netdev without resources. Device must be reset.");
> +		return -ENODEV;
> +	}
> +

[Severity: High]
This isn't a bug introduced by this patch (the baseline
gve_reset_recovery() error path also left the interface up with
priv->xsk_pools == NULL after gve_teardown_priv_resources()), but since
this patch restructures exactly that path and adds the resources_ok gate
here, should the other user-reachable entry points get the same gate?

After gve_recover() takes its teardown_device label, the netdev is still
administratively up (nothing calls dev_close()), while
gve_teardown_device() has set priv->xsk_pools = NULL,
priv->ptype_lut_dqo = NULL, priv->tx/priv->rx = NULL, freed the notify
blocks, and dma_pool_free()d the adminq buffer while priv->adminq keeps
its stale pointer.

gve_xsk_pool_disable() only bounds-checks the queue id:

	if (qid >= priv->rx_cfg.num_queues)
		return -EINVAL;

	clear_bit(qid, priv->xsk_pools);

and closing an AF_XDP zero-copy socket reaches ndo_bpf(XDP_SETUP_XSK_POOL,
pool == NULL) without any netif_running() test (xp_clear_dev() /
xp_disable_drv_zc()), so this is a clear_bit() on a NULL bitmap.
gve_set_priv_flags() likewise memsets priv->stats_report->stats when
report-stats is turned off, and gve_adjust_queues()/gve_adjust_config()
issue adminq commands through the freed priv->adminq while netif_running()
is true.

The failed-recovery precondition is easy to reach now that
gve_add_flow_rule()/gve_del_flow_rule() call gve_reset(priv, false) on
-ETIME, and gve_recover() bails out to teardown_device when e.g.
gve_adminq_verify_driver_compatibility() times out.

> @@ -1451,41 +1494,16 @@ static int gve_open(struct net_device *dev)
>  	return 0;
>  }
>  
> -static int gve_queues_stop(struct gve_priv *priv)
> +static void gve_queues_stop(struct gve_priv *priv)
>  {
> -	int err;
> +	gve_unreg_xdp_info(priv);
> +	gve_drain_page_cache(priv);
>  
> -	netif_carrier_off(priv->dev);
> -	if (gve_get_device_rings_ok(priv)) {
> -		gve_turndown(priv);
> -		gve_drain_page_cache(priv);
> -		err = gve_destroy_rings(priv);
> -		if (err)
> -			goto err;
> -		err = gve_unregister_qpls(priv);
> -		if (err)
> -			goto err;
> -		gve_clear_device_rings_ok(priv);
> -	}
>  	timer_delete_sync(&priv->stats_report_timer);
> -
> -	gve_unreg_xdp_info(priv);
> +	cancel_work_sync(&priv->stats_report_task);
>  
>  	gve_tx_stop_rings(priv, gve_num_tx_queues(priv));
>  	gve_rx_stop_rings(priv, priv->rx_cfg.num_queues);
> -
> -	priv->interface_down_cnt++;
> -	return 0;
> -
> -err:
> -	/* This must have been called from a reset due to the rtnl lock
> -	 * so just return at this point.
> -	 */
> -	if (gve_get_reset_in_progress(priv))
> -		return err;
> -	/* Otherwise reset before returning */
> -	gve_reset_and_teardown(priv, true);
> -	return gve_reset_recovery(priv, false);
>  }
>  
>  static int gve_close(struct net_device *dev)
> @@ -1493,12 +1511,30 @@ static int gve_close(struct net_device *dev)
>  	struct gve_priv *priv = netdev_priv(dev);
>  	int err;
>  
> -	err = gve_queues_stop(priv);
> -	if (err)
> -		return err;
> +	gve_turndown(priv);
> +
> +	/* Surrender to reset if the queue destroying adminq cmds fail. Reset
> +	 * will not re-enable the interface.
> +	 */
> +	if (gve_get_device_rings_ok(priv)) {
> +		gve_clear_device_rings_ok(priv);
> +		err = gve_destroy_rings(priv);
> +		if (err)
> +			goto reset;
> +		err = gve_unregister_qpls(priv);
> +		if (err)
> +			goto reset;
> +	}
>  
> +	gve_queues_stop(priv);
>  	gve_queues_mem_remove(priv);
> +	priv->interface_down_cnt++;
>  	return 0;
> +
> +reset:
> +	gve_queues_stop(priv);
> +	err = gve_reset(priv, true);
> +	return err;
>  }

[Severity: High]
On this new reset: label, is it safe to release the ring buffers before
the hardware reset?

gve_destroy_rings() or gve_unregister_qpls() failing (typically -ETIME
from gve_adminq_kick_and_wait() -> gve_adminq_wait_for_cmd()) means the
NIC never acknowledged that it stopped using those queues. gve_queues_stop()
then does:

	gve_tx_stop_rings(priv, gve_num_tx_queues(priv));
	gve_rx_stop_rings(priv, priv->rx_cfg.num_queues);

which for DQO returns RX buffer pages via gve_free_to_page_pool()/
gve_free_qpl_page_dqo(), dma_unmaps pending TX buffers
(gve_tx_clean_pending_packets() -> gve_unmap_packet()) and memsets the
descriptor/completion rings and q_resources.

The point at which the device is guaranteed to have stopped touching that
memory only comes later, in gve_reset() -> gve_teardown_device() ->
gve_adminq_free() -> gve_adminq_release():

	iowrite32be(GVE_DRIVER_STATUS_RESET_MASK, &priv->reg_bar0->driver_status);
	while (!(ioread32be(&priv->reg_bar0->device_status)
			& GVE_DEVICE_STATUS_DEVICE_IS_RESET)) { ... }

The pre-patch code returned from gve_queues_stop() before stopping the
rings on a destroy failure, and gve_reset_and_teardown() called
gve_trigger_reset() (which blocks until DEVICE_IS_RESET) first. Should the
hardware reset happen before gve_queues_stop() here?

The commit message applies exactly this reasoning to the control plane:

    With the new reset functionality, shared resources between the device
    and driver are not freed until after the hardware reset has completed

Does the same invariant hold for the data-plane rings and buffers on this
path?

[ ... ]

> -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) {
> +
> +	if (was_up) {
>  		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);
> +		if (gve_get_device_rings_ok(priv)) {
> +			gve_clear_device_rings_ok(priv);
> +			gve_destroy_rings(priv);
> +			gve_unregister_qpls(priv);
>  		}
> -		/* Clean up any remaining resources */
> -		gve_teardown_priv_resources(priv);
> +		gve_queues_stop(priv);
>  	}

[Severity: High]
Same ordering question here, with the added detail that the return values
of gve_destroy_rings() and gve_unregister_qpls() are not inspected at all,
so gve_queues_stop() unmaps/recycles the ring memory unconditionally,
including for a device whose adminq is not responding.

There is a mirror of this on the recovery side: with RESET_IN_PROGRESS
set, a create-rings failure makes gve_queues_start() take
stop_and_free_rings:

	stop_and_free_rings:
		gve_tx_stop_rings(priv, gve_num_tx_queues(priv));
		gve_rx_stop_rings(priv, priv->rx_cfg.num_queues);
		gve_queues_mem_remove(priv);

which frees the coherent rings and QPL pages even though some TX queues
may already have been created in hardware, with the reset handshake only
happening later when gve_recover() calls gve_teardown_device(). Can the
NIC still be writing into those pages at that point?

>  
> -	/* Set it all back up */
> -	err = gve_reset_recovery(priv, was_up);
> +	disable_work(&priv->service_task);
> +	gve_teardown_device(priv);
> +	gve_queues_mem_remove(priv);
> +
> +	enable_work(&priv->service_task);
> +	err = gve_recover(priv, was_up && !skip_queue_setup);
> +	if (err)
> +		dev_info(&priv->pdev->dev,
> +			 "Failed to recover in reset: %d\n", err);
> +

[Severity: High]
Only priv->service_task is disabled around gve_teardown_device() here.
When was_up is false, gve_queues_stop() (the sole
cancel_work_sync(&priv->stats_report_task) call site) is skipped, so a
stats work item queued by the timer can run concurrently with
gve_free_stats_report(). Should priv->stats_report_task be quiesced here
too?

[ ... ]

> @@ -2943,8 +2952,9 @@ static int gve_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	queue_work(priv->gve_wq, &priv->service_task);
>  	return 0;
>  
> -abort_with_gve_init:
> -	gve_teardown_priv_resources(priv);
> +abort_teardown_device:
> +	disable_work(&priv->stats_report_task);
> +	gve_teardown_device(priv);

[Severity: High]
Is this disabling the intended work item? During probe, the stats work has
no producer yet (its only producer is the timer armed by
gve_queues_start() or by ethtool on a registered netdev), while
priv->service_task is queueable at this point because gve_setup_device()
already set device_resources_ok and the mgmt IRQ is requested. Every other
teardown site in this patch uses disable_work_sync(&priv->service_task).

Should this be disable_work_sync(&priv->service_task) instead, or in
addition?

> @@ -2976,7 +2986,8 @@ static void gve_remove(struct pci_dev *pdev)
>  	void __iomem *reg_bar = priv->reg_bar0;
>  
>  	unregister_netdev(netdev);
> -	gve_teardown_priv_resources(priv);
> +	disable_work_sync(&priv->service_task);
> +	gve_teardown_device(priv);
>  	destroy_workqueue(priv->gve_wq);

[Severity: High]
For an interface that was already down, unregister_netdev() does not call
gve_close(), so nothing cancels priv->stats_report_task before
gve_teardown_device() frees priv->stats_report. destroy_workqueue() then
drains the pending stats work after the free. Does the ordering here need
a disable_work_sync(&priv->stats_report_task) as well?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909191136.3811892-1-hramamurthy%40google.com