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

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.

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

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.

[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);
}

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?

[Severity: High]
This isn't a bug introduced by this patch — the pre-patch err_vsi path also
ended in i40e_vsi_clear() — but while the error path is being reworked, can
the callers still use the freed struct i40e_vsi after a failed reinit?

i40e_vsi_clear() does kfree(vsi) and sets pf->vsi[vsi->idx] = NULL, and
i40e_vsi_reinit_setup() returns NULL. i40e_setup_pf_switch() converts that
into -EAGAIN:

	if (!main_vsi) {
		dev_info(&pf->pdev->dev, "setup of MAIN VSI failed\n");
		i40e_cloud_filter_exit(pf);
		i40e_fdir_teardown(pf);
		return -EAGAIN;
	}

but i40e_rebuild() and i40e_reset_and_rebuild() both return void, so the
failure never reaches the operation that started the rebuild.

i40e_reconfig_rss_queues() keeps using its cached pointer after the reset,
including a write:

	i40e_reset_and_rebuild(pf, true, true);
	...
	if (queue_count < vsi->rss_size) {
	...
	vsi->rss_size = min_t(int, pf->alloc_rss_size, qcount);

i40e_xdp_setup() does the same, and writes through the ring array:

	i40e_reset_and_rebuild(pf, true, true);
	...
	for (i = 0; i < vsi->num_queue_pairs; i++)
		WRITE_ONCE(vsi->rx_rings[i]->xdp_prog, vsi->xdp_prog);

Is this a use-after-free read and write on the freed VSI? It still looks
present at the end of the series, so would a complete fix either propagate
the failure out of i40e_rebuild()/i40e_reset_and_rebuild() or keep the VSI
alive until these callers are done with it?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918212458.550425-1-anthony.l.nguyen%40intel.com