Re: [PATCH net 1/8] i40e: unregister netdev before clearing VSI on reinit failure

[Maciej Fijalkowski <maciej.fijalkowski@intel.com>]

On Tue, Sep 22, 2026 at 01:31:05AM +0000, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 (1) · Medium: 1 · Low: 0
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [High] The patch makes the two early allocation failures in
>   i40e_vsi_reinit_setup() jump to the new err_netdev label, which calls…
> - [Medium] At this commit the reinit teardown at the top of
>   i40e_vsi_reinit_setup() deliberately keeps the existing per-vector…
> 
> Pre-existing issues:
> - [High] When i40e_vsi_reinit_setup() fails it ends in i40e_vsi_clear(),
>   which does kfree(vsi) and sets pf->vsi[vsi->idx] = NULL, and returns…
> 
> --
> 
> i40e: unregister netdev before clearing VSI on reinit failure
> 
> The patch splits the error path of i40e_vsi_reinit_setup() so that the two
> early allocation failures (i40e_vsi_alloc_arrays() and i40e_get_lump())
> reach a new err_netdev label which unregisters and frees the netdev before
> calling i40e_vsi_clear().
> 
> A few questions about the new error path below.
> 
> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> > index 0cd0e5597c90..de4c0737f72e 100644
> > --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> > @@ -14268,7 +14268,7 @@ static struct i40e_vsi *i40e_vsi_reinit_setup(struct i40e_vsi *vsi)
> >  	i40e_set_num_rings_in_vsi(vsi);
> >  	ret = i40e_vsi_alloc_arrays(vsi, false);
> >  	if (ret)
> > -		goto err_vsi;
> > +		goto err_netdev;
> 
> [Severity: Medium]
> Can these two new gotos leak the struct i40e_q_vector allocations?
> 
> The teardown at the top of i40e_vsi_reinit_setup() intentionally keeps the
> per-vector objects alive:
> 
> 	i40e_put_lump(pf->qp_pile, vsi->base_queue, vsi->idx);
> 	i40e_vsi_clear_rings(vsi);
> 
> 	i40e_vsi_free_arrays(vsi, false);
> 
> With free_qvectors == false, both vsi->q_vectors and every individual
> struct i40e_q_vector are still allocated at this point.
> 
> The new err_netdev label is placed below i40e_vsi_free_q_vectors(), so the
> i40e_vsi_alloc_arrays() and i40e_get_lump() failures skip the per-vector
> destructor i40e_free_q_vector(), which is what does netif_napi_del() and
> kfree_rcu(q_vector, rcu).

This is fixed by patch 7, which frees the q_vectors at the start of
i40e_vsi_reinit_setup() before the allocation paths can fail. The
i40e_get_lump() failure is also routed through err_rings so that the
newly allocated q_vectors are released.

> 
> The final i40e_vsi_clear() then calls i40e_vsi_free_arrays(vsi, true),
> which frees only the q_vectors pointer array, and kfree(vsi) — so
> num_q_vectors vector objects, each with an embedded NAPI, are never freed.
> 
> Would "goto err_rings" be the intended target here, since it falls through
> into the same netdev teardown block that this patch wants to reach?
> 
> For the record, the leak is gone at the end of the series: "i40e: keep
> q_vectors array in sync with channel count changes" adds
> i40e_vsi_free_q_vectors() to the top-of-function teardown and reroutes the
> i40e_get_lump() failure to err_rings, so nothing per-vector remains
> allocated when err_netdev is taken. It only exists in this intermediate
> commit.
> 
> >  
> >  	alloc_queue_pairs = vsi->alloc_queue_pairs *
> >  			    (i40e_enabled_xdp_vsi(vsi) ? 2 : 1);
> > @@ -14278,7 +14278,7 @@ static struct i40e_vsi *i40e_vsi_reinit_setup(struct i40e_vsi *vsi)
> >  		dev_info(&pf->pdev->dev,
> >  			 "failed to get tracking for %d queues for VSI %d err %d\n",
> >  			 alloc_queue_pairs, vsi->seid, ret);
> > -		goto err_vsi;
> > +		goto err_netdev;
> >  	}
> >  	vsi->base_queue = ret;
> >  
> > @@ -14303,6 +14303,7 @@ static struct i40e_vsi *i40e_vsi_reinit_setup(struct i40e_vsi *vsi)
> >  
> >  err_rings:
> >  	i40e_vsi_free_q_vectors(vsi);
> > +err_netdev:
> >  	if (vsi->netdev_registered) {
> >  		vsi->netdev_registered = false;
> >  		unregister_netdev(vsi->netdev);
> 
> [Severity: High]
> Is RTNL already held when the two newly redirected failures land here?
> 
> i40e_vsi_reinit_setup() is only reached from i40e_setup_pf_switch() with
> reinit == true:
> 
> 	else if (reinit)
> 		main_vsi = i40e_vsi_reinit_setup(main_vsi);
> 
> and i40e_rebuild() wraps that call in RTNL:
> 
> 	if (!lock_acquired)
> 		rtnl_lock();
> 	ret = i40e_setup_pf_switch(pf, reinit, true);
> 
> unregister_netdev() takes the lock itself:
> 
> net/core/dev.c:unregister_netdev() {
> 	rtnl_net_dev_lock(dev);
> 	unregister_netdevice(dev);
> 	rtnl_net_dev_unlock(dev);
> }
> 

Patch 4 passes the lock state into i40e_vsi_reinit_setup() and uses
unregister_netdevice() when RTNL is already held.


> So a kzalloc failure in i40e_vsi_alloc_arrays() or an exhausted
> pf->qp_pile in i40e_get_lump(), reached via ethtool -L or an XDP attach
> going through i40e_reset_and_rebuild(pf, true, true) -> i40e_rebuild() ->
> i40e_setup_pf_switch(), would recursively acquire RTNL. Does that hang the
> task in D state while holding RTNL, blocking further network configuration
> system-wide?
> 
> Before this patch those two failures went to err_vsi and never touched
> RTNL, so the exposure is new here.
> 
> This is addressed later in the same series by "i40e: avoid deadlock when
> calling unregister_netdev()", which adds a lock_acquired parameter and
> selects unregister_netdevice() on the RTNL-held path, so the final state of
> the series is fine. Could the label split and the lock_acquired plumbing be
> folded into one patch so this commit is not broken on its own?
> 
> > @@ -14312,7 +14313,6 @@ static struct i40e_vsi *i40e_vsi_reinit_setup(struct i40e_vsi *vsi)
> >  	if (vsi->type == I40E_VSI_MAIN)
> >  		i40e_devlink_destroy_port(pf);
> >  	i40e_aq_delete_element(&pf->hw, vsi->seid, NULL);
> > -err_vsi:
> >  	i40e_vsi_clear(vsi);
> >  	return NULL;
> >  }
> 
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918212458.550425-1-anthony.l.nguyen%40intel.com