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

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.

[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.

[ ... ]

[ ... ]

[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?

[ ... ]

[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.

[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?

[ ... ]

[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?

[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?

[ ... ]

[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?

[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