Re: [PATCH net-next v4 09/10] enic: wire V2 SR-IOV enable with admin channel and MBOX

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
enic: wire V2 SR-IOV enable with admin channel and MBOX

This commit extends enic_sriov_configure() to handle V2 SR-IOV VFs,
allocating per-VF MBOX state and opening the admin channel before calling
pci_enable_sriov(). It also reverses the order in the disable path.

Note: The format of this report is altered due to recitation restrictions.
The original patch code is not quoted, and a free-form summary of the
observations is provided instead.

In enic_sriov_v2_enable(), enic->num_vfs is set and ENIC_SRIOV_ENABLED is
asserted, but is the enic->pp array reallocated to accommodate the VFs?

If enic->pp remains a 1-element array, could subsequent network device
operations bypass the vf < enic->num_vfs check and calculate an
out-of-bounds pointer for vf > 0 using the ENIC_PP_BY_INDEX macro,
corrupting the heap?

Is enic_sriov_configure() actually registered as the .sriov_configure
callback in the enic_driver structure?

Because the function is marked __maybe_unused, does this leave the sysfs
interface unable to invoke it, making the V2 VF dynamic enablement
unreachable?

When enic_sriov_configure() is called via sysfs, it mutates enic->pp and
enic->num_vfs. Does this happen without holding the rtnl_lock?

If concurrent network device operations running under rtnl_lock validate
the VF index and resolve enic->pp right before the sysfs path calls
kfree(enic->pp), can this result in a use-after-free when the stale pointer
is dereferenced?

In the SR-IOV disable path inside enic_sriov_configure(), the code frees
the existing port profiles array and reallocates a single element with
kzalloc_obj().

If this allocation fails and returns -ENOMEM, enic->pp is left as NULL.
Could a subsequent NDO call targeting the PF pass the enic_is_valid_pp_vf()
check and crash when dereferencing the NULL enic->pp?

When V2 SR-IOV is enabled, enic_sriov_v2_enable() calls enic_mbox_init(),
which unconditionally executes mutex_init(&enic->mbox_lock) and
init_completion(&enic->mbox_comp).

If SR-IOV is disabled and later re-enabled, will re-initializing these
active synchronization primitives over their existing state violate locking
rules?

The commit message states:
"The existing V1/USNIC SR-IOV paths are unchanged."

However, in enic_remove(), the calls to enic_dev_deinit(enic) and
vnic_dev_close(enic->vdev) were moved to execute after pci_disable_sriov().
Additionally, the enic->priv_flags &= ~ENIC_SRIOV_ENABLED cleanup was
dropped.

Could the commit message be updated to reflect this behavioral change to the
hardware teardown sequence, or was this change unintended?