Thank you for your contribution! Sashiko AI review found 7 potential
issue(s) to consider:
Critical: 0 · High: 1 · Medium: 4 (2) · Low: 2
- [High] gve_recover()'s new failure path calls gve_teardown_device()
(e.g. when gve_open() fails on queue-memory allocation), freeing all…
- [Medium] The -ETIME (admin-queue timeout) reset trigger was removed
from the shared helper gve_adminq_configure_flow_rule() and re-added…
- [Medium] gve_mgmnt_intr() now silently discards the management
interrupt whenever DEVICE_RESOURCES_OK is clear.
- [Medium] gve_queues_start()'s `reset:` path returns gve_reset()'s
status instead of the original error, so a successful reset converts a…
- [Medium] Queue-creation failure during reset recovery frees device-
owned ring memory before the hardware reset.
- [Low] The patch removes a synchronous gve_reset() on admin-queue
timeout from the shared flow-rule helper, which fixes a genuine…
- [Low] The commit message uses a `deconfigure_device_resources` failure
as its example of a path that 'preempts its reset with another reset…
Pre-existing issues:
- [Medium] Failed queue setup during reset recovery omits
gve_unreg_xdp_info(), leaking the XDP memory-model reference on DQO…
- [Medium] A service task that already observed GVE_PRIV_FLAGS_DO_RESET
can block on RTNL while a concurrent direct gve_reset() (ethtool/flow-…
--
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 the
hardware reset is now performed before shared resources are freed.
> gve: simplify reset logic
>
> [ ... ]
>
> 2) Removing resets during teardown. This is partly covered by removing
> recursive resets, but the primary goal in this case is to ensure that
> the driver is capable of actually executing a hardware reset if
> something goes wrong with a control plane operation. As it stands, if
> `deconfigure_device_resources` fails, for example, GVE will preempt
> its reset with another reset without actually invoking a hardware
> reset, which could actually help with recovery.
[Severity: Low]
Is the deconfigure_device_resources example accurate? In the baseline that
branch calls gve_trigger_reset(), which is just a wrapper for
gve_adminq_release():
gve_teardown_device_resources()
err = gve_adminq_deconfigure_device_resources(priv);
if (err) {
dev_err(...);
gve_trigger_reset(priv);
}
and gve_adminq_release() does perform the hardware reset and waits for the
device to acknowledge it:
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 path that really recursed is the flow-rule -ETIME case, where
gve_adminq_configure_flow_rule() called gve_reset() from inside teardown.
Could the changelog name that case instead?
> diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/ethernet/google/gve/gve_adminq.c
> index a62cb7a921d07..901673d2e2648 100644
> --- a/drivers/net/ethernet/google/gve/gve_adminq.c
> +++ b/drivers/net/ethernet/google/gve/gve_adminq.c
[ ... ]
> @@ -1377,12 +1376,8 @@ gve_adminq_configure_flow_rule(struct gve_priv *priv,
> sizeof(struct gve_adminq_configure_flow_rule),
> flow_rule_cmd);
>
> - if (err == -ETIME) {
> - dev_err(&priv->pdev->dev, "Timeout to configure the flow rule, trigger reset");
> - gve_reset(priv, true);
> - } else if (!err) {
> + if (!err)
> priv->flow_rules_cache.rules_cache_synced = false;
> - }
>
> return err;
> }
[Severity: Medium]
This helper has three callers, but the -ETIME handling is re-added below only
for the add and delete cases. Does the third caller lose reset on timeout?
gve_set_features() /* ethtool -K <dev> ntuple off */
gve_flow_rules_reset()
ops->reset_flow_rules() == gve_adminq_reset_flow_rules()
gve_adminq_configure_flow_rule()
gve_adminq_reset_flow_rules() just forwards the return value:
return gve_adminq_configure_flow_rule(priv, &flow_rule_cmd);
and gve_set_features() does:
err = gve_flow_rules_reset(priv);
if (err)
goto revert_features;
so an admin-queue timeout there no longer starts recovery, while before this
patch it called gve_reset(priv, true). The v3 changelog says "only reset when
failing to program flow rules as ethtool op", and gve_set_features() is such
an ethtool op, so was this call site simply missed?
[Severity: Low]
Removing this gve_reset() also fixes a real recursion: teardown calls
gve_flow_rules_reset(), so an adminq timeout during gve_reset(),
gve_remove() or gve_suspend() re-entered gve_reset() from inside teardown.
Should the patch carry a Fixes: tag for 6f3bc487565d ("gve: Add flow steering
ethtool support")? The trailers currently only have Reviewed-by and
Signed-off-by lines.
> diff --git a/drivers/net/ethernet/google/gve/gve_flow_rule.c b/drivers/net/ethernet/google/gve/gve_flow_rule.c
> index 2c80cda28ef30..fae552f4ad6fb 100644
> --- a/drivers/net/ethernet/google/gve/gve_flow_rule.c
> +++ b/drivers/net/ethernet/google/gve/gve_flow_rule.c
> @@ -278,6 +278,11 @@ int gve_add_flow_rule(struct gve_priv *priv, struct ethtool_rxnfc *cmd)
> goto out;
>
> err = gve_adminq_add_flow_rule(priv, rule, fsp->location);
> + if (err == -ETIME) {
> + dev_err(&priv->pdev->dev,
> + "Timeout to add flow rule, trigger reset.");
> + gve_reset(priv, false);
> + }
>
> out:
> kvfree(rule);
> @@ -290,9 +295,17 @@ int gve_add_flow_rule(struct gve_priv *priv, struct ethtool_rxnfc *cmd)
> int gve_del_flow_rule(struct gve_priv *priv, struct ethtool_rxnfc *cmd)
> {
> struct ethtool_rx_flow_spec *fsp = (struct ethtool_rx_flow_spec *)&cmd->fs;
> + int err;
>
> if (!priv->max_flow_rules)
> return -EOPNOTSUPP;
>
> - return gve_adminq_del_flow_rule(priv, fsp->location);
> + err = gve_adminq_del_flow_rule(priv, fsp->location);
> + if (err == -ETIME) {
> + dev_err(&priv->pdev->dev,
> + "Timeout to delete flow rule, trigger reset.");
> + gve_reset(priv, false);
> + }
> +
> + return err;
> }
> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> index 2fe280cf7e680..82bf2c5fed3f2 100644
> --- a/drivers/net/ethernet/google/gve/gve_main.c
> +++ b/drivers/net/ethernet/google/gve/gve_main.c
[ ... ]
> @@ -270,6 +271,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_HANDLED;
> +
> queue_work(priv->gve_wq, &priv->service_task);
> return IRQ_HANDLED;
> }
[Severity: Medium]
Can this drop a device-requested reset?
The mgmt vector is requested in gve_alloc_notify_blocks() during the
allocation phase, while gve_set_device_resources_ok() only runs at the very
end of gve_setup_control_plane_resources(), after several blocking adminq
round trips. Any event arriving in that window, including
GVE_DEVICE_STATUS_RESET_MASK, is acked as handled and thrown away.
The only consumer of that bit is gve_service_task():
u32 status = ioread32be(&priv->reg_bar0->device_status);
gve_handle_status(priv, status);
and it only runs when the service task is queued. On the paths that call
gve_recover() with setup_queues == false (gve_reset(priv, true) from
gve_close(), and gve_resume() when up_before_suspend is false) there is no
queue_work() from gve_queues_start(), so nothing re-samples device_status
after the flag is set again.
Since disable_work()/enable_work() already stop the service task across
reset, and gve_reset() returns early on reset_in_progress, would simply
queueing the work (a no-op while disabled) or re-reading device_status after
gve_set_device_resources_ok() avoid the lost-event window?
[ ... ]
> @@ -1416,10 +1465,11 @@ static int gve_queues_start(struct gve_priv *priv,
> reset:
> if (gve_get_reset_in_progress(priv))
> goto stop_and_free_rings;
> - gve_reset_and_teardown(priv, true);
> - /* if this fails there is nothing we can do so just ignore the return */
> - gve_reset_recovery(priv, false);
> - /* return the original error */
> +
> + /* Attempt to reset. If reset is successful, gve_queues_start was
> + * successful.
> + */
> + err = gve_reset(priv, false);
> return err;
> stop_and_free_rings:
> gve_tx_stop_rings(priv, gve_num_tx_queues(priv));
[Severity: Medium]
Returning gve_reset()'s status here reports success for a queue start that
failed. Can that silently discard a requested configuration?
In gve_queues_start() these two assignments happen only after
gve_register_qpls() succeeds:
priv->header_split_enabled = rx_alloc_cfg->enable_header_split;
priv->rx_cfg.packet_buffer_size = rx_alloc_cfg->packet_buffer_size;
The earlier priv->rx_cfg = *rx_alloc_cfg->qcfg_rx does not carry them,
because gve_rx_get_curr_alloc_cfg() sets cfg->qcfg_rx = &priv->rx_cfg, so an
ethtool rx-buf-len or tcp-data-split change lives only in the temporary
alloc cfg.
If gve_init_rss_config() or gve_register_qpls() fails, we jump to reset, and
gve_reset() -> gve_recover() -> gve_open() -> gve_get_curr_alloc_cfgs()
rebuilds the rings from the stale priv values, while gve_open(),
gve_adjust_config() and gve_set_ringparam() all see 0. Would committing the
requested configuration before entering reset recovery, or propagating the
original error, be preferable?
[Severity: Medium]
The commit message says "shared resources between the device and driver are
not freed until after the hardware reset has completed". Does the
stop_and_free_rings path below meet that guarantee during reset recovery?
gve_create_rings() returns on partial success without cleaning up, for
example TX queues created and RX creation failing:
err = gve_adminq_create_rx_queues(priv, priv->rx_cfg.num_queues);
if (err) {
/* This failure will trigger a reset - no need to clean
* up
*/
return err;
}
With reset_in_progress set by the enclosing gve_reset(), we then take
stop_and_free_rings and call gve_queues_mem_remove(), which dma_free_coherent()s
the ring memory and releases the QPL pages while those queues are still
registered with the device. The hardware reset only happens afterwards in
gve_recover() -> gve_teardown_device() -> gve_reset_device(), whose teardown
adminq commands can block before the reset register write. Could the device
still write into that freed memory?
[Severity: Medium]
This isn't a bug introduced by this patch, but the same stop_and_free_rings
path never calls gve_unreg_xdp_info(), so the XDP memory-model reference on
DQO page pools looks leaked.
gve_reg_xdp_info() registers the pool:
xdp_rxq_info_reg_mem_model(&rx->xdp_rxq, MEM_TYPE_PAGE_POOL,
rx->dqo.page_pool);
and __xdp_reg_mem_model() takes an extra reference:
if (type == MEM_TYPE_PAGE_POOL)
page_pool_use_xdp_mem(allocator, mem_allocator_disconnect, mem);
while gve_rx_free_ring_dqo() only does one page_pool_destroy(). Since
xdp_rxq_info_unreg() is reached only from gve_unreg_xdp_info(), does the pool
and mem-model id stay around after a failed queue start with XDP enabled on
DQO?
[ ... ]
> @@ -2514,30 +2554,7 @@ static int gve_init_priv(struct gve_priv *priv)
> return 0;
> }
>
> -static void gve_teardown_priv_resources(struct gve_priv *priv)
> -{
> - gve_teardown_device_resources(priv);
> - gve_adminq_free(priv);
> - bitmap_free(priv->xsk_pools);
> - priv->xsk_pools = NULL;
> -}
> -
> -static void gve_trigger_reset(struct gve_priv *priv)
> -{
> - /* Reset the device by releasing the AQ */
> - gve_adminq_release(priv);
> -}
> -
> -static void gve_reset_and_teardown(struct gve_priv *priv, bool was_up)
> -{
> - gve_trigger_reset(priv);
> - /* With the reset having already happened, close cannot fail */
> - if (was_up)
> - gve_close(priv->dev);
> - gve_teardown_priv_resources(priv);
> -}
> -
> -static int gve_reset_recovery(struct gve_priv *priv, bool was_up)
> +static int gve_recover(struct gve_priv *priv, bool setup_queues)
> {
> int err;
>
> @@ -2545,62 +2562,67 @@ static int gve_reset_recovery(struct gve_priv *priv, bool was_up)
> if (err) {
> dev_err(&priv->pdev->dev,
> "Failed to alloc admin queue: err=%d\n", err);
> - goto err;
> + goto teardown_device;
> }
>
> err = gve_adminq_verify_driver_compatibility(priv);
> if (err) {
> dev_err(&priv->pdev->dev,
> "Could not verify driver compatibility: err=%d\n", err);
> - goto err_free_adminq;
> + goto teardown_device;
> }
>
> err = gve_setup_device(priv);
> if (err)
> - goto err_free_adminq;
> - if (was_up) {
> + goto teardown_device;
> +
> + if (setup_queues) {
> err = gve_open(priv->dev);
> if (err)
> - return err;
> + goto teardown_device;
> }
> +
> return 0;
>
> -err_free_adminq:
> - gve_adminq_free(priv);
> -err:
> - dev_err(&priv->pdev->dev, "Reset failed! !!! DISABLING ALL QUEUES !!!\n");
> - gve_turndown(priv);
> +teardown_device:
> + dev_err(&priv->pdev->dev, "Recover failed! !!! DISABLING ALL QUEUES !!!\n");
> + gve_teardown_device(priv);
> return err;
> }
[Severity: High]
Routing a gve_open() failure into gve_teardown_device() frees the whole
control plane while the netdev stays registered and administratively
running. Can that be dereferenced afterwards?
gve_teardown_device() -> gve_free_control_plane_resources() calls
gve_free_rss_config_cache():
kfree(rss_config->hash_key);
kfree(rss_config->hash_lut);
memset(rss_config, 0, sizeof(*rss_config));
and gve_free_notify_blocks(), which leaves priv->ntfy_blocks NULL, while
priv->cache_rss_config, priv->rss_key_size and priv->rss_lut_size (set in
gve_init_priv()) are untouched and nothing closes the interface.
Only gve_open() got the new resources_ok gate, so ETHTOOL_GRSSH still
reaches gve_get_rxfh() -> gve_get_rss_config_cache():
memcpy(rxfh->key, rss_config->hash_key, priv->rss_key_size);
with hash_key == NULL, and ETHTOOL_GRSSH is one of the commands that skips
the ns_capable(net->user_ns, CAP_NET_ADMIN) check in net/ethtool/ioctl.c.
An ethtool channel change is a second route: gve_adjust_queues() still sees
netif_running(priv->dev) true, so gve_adjust_config() -> gve_queues_start()
-> gve_tx_start_rings() -> gve_tx_add_to_block() runs:
struct gve_notify_block *block = &priv->ntfy_blocks[ntfy_idx];
struct gve_tx_ring *tx = &priv->tx[queue_idx];
block->tx = tx;
on a NULL array. Should these entry points also be rejected (or the netdev
detached) while control-plane resources are gone?
>
> -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);
[Severity: Medium]
This is a pre-existing issue, but the new early return only covers
overlapping resets, not a stale pending request. gve_handle_reset() samples
the bit before taking the locks and never rechecks it:
if (gve_get_do_reset(priv)) {
rtnl_lock();
netdev_lock(priv->dev);
gve_reset(priv, false);
If a direct gve_reset() from the ethtool or flow-rule path completes while
the worker is blocked on RTNL, it clears both do_reset and
reset_in_progress (and disable_work() does not wait for an already running
worker), so the worker performs a second full reset once it gets the locks.
Would rechecking do_reset after acquiring the locks be reasonable?
>
> - /* 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);
> }
>
> - /* Set it all back up */
> - err = gve_reset_recovery(priv, was_up);
> + disable_work(&priv->service_task);
> + gve_reset_device(priv);
> + gve_queues_stop(priv);
> + gve_queues_mem_remove(priv);
> + gve_free_control_plane_resources(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);
> +
> gve_clear_reset_in_progress(priv);
> priv->reset_cnt++;
> priv->interface_up_cnt = 0;
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260916165533.2187434-1-hramamurthy%40google.com
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.