Re: [PATCH net 4/8] i40e: avoid deadlock when calling unregister_netdev()

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 (1) · Medium: 0 (2) · 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] Incomplete fix: the RTNL self-deadlock is only removed from the
  i40e_vsi_reinit_setup() arm of i40e_setup_pf_switch().

Pre-existing issues:
- [High] Dangling main-VSI references after a failed VSI reinit.
- [Medium] NULL pointer dereference when the reinit unwind re-enters
  ndo_stop after q_vectors/rings were released.
- [Medium] Memory leak in the asymmetric unwind of
  i40e_vsi_reinit_setup() as it exists at this commit: the two early…

--

i40e: avoid deadlock when calling unregister_netdev()

This adds a lock_acquired parameter to i40e_vsi_reinit_setup() so the
err_netdev unwind uses unregister_netdevice() when RTNL is already held by
the rebuild path, and unregister_netdev() otherwise. The caller in
i40e_setup_pf_switch() forwards its own lock_acquired flag.

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but now that this unwind can
actually run to completion instead of deadlocking, what happens to
pf->lan_vsi afterwards?

The tail of this path is:

	i40e_aq_delete_element(&pf->hw, vsi->seid, NULL);
	i40e_vsi_clear(vsi);
	return NULL;

and i40e_vsi_clear() does:

	pf->vsi[vsi->idx] = NULL;
	...
free_vsi:
	kfree(vsi);

pf->lan_vsi is never reset to I40E_NO_VSI, so i40e_pf_get_main_vsi()
returns NULL from then on.

After i40e_setup_pf_switch() returns -EAGAIN, i40e_rebuild() only clears
__I40E_RESET_FAILED, __I40E_RESET_RECOVERY_PENDING and
__I40E_TIMEOUT_RECOVERY_PENDING; neither __I40E_DOWN nor __I40E_SUSPENDED
is set, so the service task keeps running. Can the next watchdog tick or
link event then oops in i40e_link_event(), which dereferences the main VSI
with no NULL check?

	if (new_link == old_link &&
	    new_link_speed == old_link_speed &&
	    (test_bit(__I40E_VSI_DOWN, vsi->state) ||
	     new_link == netif_carrier_ok(vsi->netdev)))
		return;
	...
	i40e_print_link_message(vsi, new_link);

There is also the ethtool -L caller. i40e_reconfig_rss_queues() caches the
main VSI pointer, calls the void i40e_reset_and_rebuild(pf, true, true) and
then keeps using the pointer:

	i40e_reset_and_rebuild(pf, true, true);
	...
	if (queue_count < vsi->rss_size) {
	...
	qcount = vsi->num_queue_pairs / vsi->tc_config.numtc;

Does that dereference a VSI that this error path already kfree()d, since the
rebuild failure is not reported back to the caller?

[Severity: Medium]
This is a pre-existing issue, but the ordering between err_rings and
err_netdev looks worth a second look now that the unregister actually
proceeds under RTNL.

err_rings calls i40e_vsi_free_q_vectors(vsi), which NULLs every
vsi->q_vectors[i], and then falls through to this unregister. Both
unregister_netdevice() and unregister_netdev() reach
unregister_netdevice_many_notify() -> netif_close_many(), which calls
ndo_stop for any device that still has IFF_UP set:

net/core/dev.c:netif_close_many() {
	list_for_each_entry_safe(dev, tmp, head, close_list)
		if (!(dev->flags & IFF_UP))
			list_del_init(&dev->close_list);

	__dev_close_many(head);
}

IFF_UP is still set because the reset quiesce calls ndo_stop directly rather
than dev_close():

i40e_quiesce_vsi() {
	...
		vsi->netdev->netdev_ops->ndo_stop(vsi->netdev);
	...
}

So i40e_close() -> i40e_vsi_close() runs on the already dismantled VSI:

	if (!test_and_set_bit(__I40E_VSI_DOWN, vsi->state))
		i40e_down(vsi);

and i40e_down() -> i40e_napi_disable_all() dereferences the freed vector
array without a NULL check:

		struct i40e_q_vector *q_vector = vsi->q_vectors[q_idx];

		if (q_vector->rx.ring || q_vector->tx.ring)

__I40E_VSI_DOWN can still be clear here, since i40e_prep_for_reset()
returns early and skips i40e_pf_quiesce_all_vsi() when a reset is already
pending:

	if (test_and_set_bit(__I40E_RESET_RECOVERY_PENDING, pf->state))
		return;

Can this combination oops in i40e_napi_disable_all(), or is there something
in the ethtool/ndo entry points that prevents a reinit from starting while
another reset is pending?

[Severity: Medium]
This isn't a bug introduced by this patch either, and it looks already
addressed in this tree, but noting it for completeness: the two early
goto err_netdev paths (i40e_vsi_alloc_arrays() failure and i40e_get_lump()
failure) skip i40e_vsi_free_q_vectors(), which only the err_rings label
performs:

err_rings:
	i40e_vsi_free_q_vectors(vsi);
err_netdev:

With the older shape of this function, which kept the previously allocated
q_vectors alive across the reinit, i40e_vsi_clear() -> i40e_vsi_free_arrays()
would only free the pointer array:

	if (free_qvectors) {
		kfree(vsi->q_vectors);
		vsi->q_vectors = NULL;
	}

leaving each struct i40e_q_vector orphaned, since free_netdev() only unlinks
NAPI instances from dev->napi_list and does not free the containing
q_vector. The commit "i40e: keep q_vectors array in sync with channel count
changes" moves i40e_vsi_free_q_vectors() to the top of
i40e_vsi_reinit_setup() and re-creates the vectors via
i40e_vsi_setup_vectors(), so no live q_vector objects remain when the early
err_netdev paths run and the leak is gone.

[Severity: High]
Should the other arm of this if/else get the same treatment? Both branches
are entered with identical lock state:

i40e_rebuild() {
	...
	if (!lock_acquired)
		rtnl_lock();
	ret = i40e_setup_pf_switch(pf, reinit, true);
	...
}

but i40e_vsi_setup() still takes RTNL itself, both on success:

	ret = register_netdev(vsi->netdev);
	if (ret)
		goto err_dl_port;
	vsi->netdev_registered = true;

and on its unwind, which is the exact pattern this patch just replaced:

err_msix:
	if (vsi->netdev_registered) {
		vsi->netdev_registered = false;
		unregister_netdev(vsi->netdev);
		free_netdev(vsi->netdev);
		vsi->netdev = NULL;
	}

Can the !main_vsi branch be reached with RTNL held? The err_netdev path
patched above ends in i40e_vsi_clear(), which sets pf->vsi[vsi->idx] = NULL
and kfree()s the VSI without resetting pf->lan_vsi, so
i40e_pf_get_main_vsi() returns NULL on the next reset. The following
rebuild would then take:

		if (!main_vsi)
			main_vsi = i40e_vsi_setup(pf, I40E_VSI_MAIN,
						  uplink_seid, 0);

with rtnl_mutex already held, and register_netdev() -> rtnl_lock() would
self-deadlock on the non-recursive mutex, which is the failure mode this
commit describes fixing.

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