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

[Joshua Washington]

On Fri, Sep 18, 2026 at 12:57 AM <netdev-bot+sashiko@xxxxxxxxxx> wrote:
>
> 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?

Yes, this was a bad example on my part. A better example would be:

gve_close() (from gve_shutdown, for example)
  gve_queues_stop() -> device_rings_ok() == true
    gve_destroy_rings() -> fails, reset_in_progress == false
      gve_reset_and_teardown() -> was_up == true
      gve_close() ...

However, even in this case, it is not accurate to say that the HW reset does not
happen. I will update the commit description to correct this.

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

gve_set_features() was missed. Will attempt a reset there as well.

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

This was originally done in an attempt to prevent an interrupt from
coming in while the device is suspended. However, this is solved by
the symmetric paired disable_work/enable_work in gve_suspend and
gve_resume, and in gve_reset. So this can be removed.

>
> [ ... ]
>
> > @@ -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?

In this case, it makes sense to propagate the original error. There is
no guarantee that the configuration itself is not the cause of the
error warranting a reset. Will revert this to propagating the original
error.

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

Yes, this will have to be moved. Since gve_queues_mem_remove is
idempotent, the simplest solution might be to call
gve_queues_mem_remove as part of gve_teardown_device(), freeing the
queues at the same time as control plane resources are freed.

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

If gve_open fails as part of gve_recover, that is meant to be an
unrecoverable state. Even if the gve_open was changed to only turndown
the device as was the case before this patch, there is still the
problem that a failure can happen before gve_open() is called. Given
that this issue predates this patch, I think it makes sense to come up
with a solution for this issue in a future patch.

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

This is a good point. Will defend against TOCTOU in gve_handle_reset().

>
> >
> > -     /* 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

-- Josh