Re: [PATCH v3 net-next 12/15] net: enetc: add VF support for i.MX94 and i.MX95

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 2 · Low: 0

- [High] An untrusted VF can repeatedly force the PF mailbox worker to
  wait up to 200 ms while holding pf->msg_lock.
- [Medium] Commit message claims driver-side recognition of per-instance
  VF capabilities that the patch does not implement, and is internally…
- [Medium] Blocking mailbox wait under rtnl_lock vs. a PF mailbox worker
  that itself takes rtnl_lock.

--

net: enetc: add VF support for i.MX94 and i.MX95

This adds the ENETC v4 VF PCI device ID 0xef00 to the VF driver, adds VF
driver data and enetc4 SI ops for revisions 4.1 and 4.3, and hooks
.sriov_configure into the enetc4 PF driver so VFs can be created on i.MX94
and i.MX95.  It also restricts the rxnfc ethtool callbacks to revision 1.

There are no Link: tags in the commit header.

A few questions below.

[Severity: Medium]
The commit message says:

> On i.MX95 (v4.1), each ENETC instance supports 2 VFs.
>
> The i.MX94 (v4.3) has two types of ENETC with different VF capabilities:
>  - standalone ENETC (same as i.MX95): does not support VFs

Are these two statements consistent?  The first says each i.MX95 instance
supports 2 VFs, the second says the i.MX94 standalone ENETC is the same as
i.MX95 and supports no VFs.

The message then says:

> The driver is updated to recognize these SoC-specific VF capabilities
> and handle each ENETC instance accordingly.

Where does that recognition happen?  The only PF-side change here is the
unconditional .sriov_configure in enetc4_pf_driver, which applies to every
entry of enetc4_pf_id_table (both the standalone PF 0xe101 and the PPM
0xe110), plus the VF teardown in enetc4_pf_remove().  There is no new field
in struct enetc_drvdata and no revision or device-ID test bounding the VF
count; enetc_init_sriov_resources() in enetc_pf_common.c still derives it
purely from the PCI capability:

	pf->total_vfs = pci_sriov_get_totalvfs(pf->si->pdev);

and that function is not touched by this patch.  So if the i.MX94
standalone instance advertises a non-zero TotalVFs, a write to
sysfs sriov_numvfs would still be accepted on the instance the message
declares unsupported.  Is the gating code missing from the patch, or should
the message be reworded?

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc4_pf.c b/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
> index c421c0e7355b..a945a120c553 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc4_pf.c

[ ... ]

> @@ -1138,6 +1141,7 @@ static struct pci_driver enetc4_pf_driver = {
>  	.id_table = enetc4_pf_id_table,
>  	.probe = enetc4_pf_probe,
>  	.remove = enetc4_pf_remove,
> +	.sriov_configure = enetc_sriov_configure,
>  };
>  module_pci_driver(enetc4_pf_driver);

[Severity: Medium]
Enabling .sriov_configure here, together with the new 0xef00 VF ID, makes
the PSI/VSI mailbox path live on ENETC v4 for the first time.  Can the PF
mailbox worker and a VF mailbox sender deadlock against each other on
rtnl_lock until the poll times out?

The PF worker path takes rtnl_lock while serving a VF request:

enetc_msg_task() -> enetc_msg_handle_rxmsg() -> enetc_msg_handle_link_speed()
  -> enetc_msg_get_link_speed() {
	rtnl_lock();
	phylink_ethtool_ksettings_get(priv->phylink, &link_info);
	rtnl_unlock();
  }

The VF side runs from ndo_set_mac_address, so rtnl is already held, and it
then waits up to 200 ms for the PF to answer:

enetc_vf_set_mac_addr() -> enetc_msg_vsi_set_primary_mac_addr()
  -> enetc_msg_vsi_send() {
	err = read_poll_timeout(enetc_rd, vsimsgsr,
				!(vsimsgsr & ENETC_VSIMSGSR_MB),
				1000, 200000, false, &si->hw, ENETC_VSIMSGSR);
  }

If the single worker is servicing a trusted VF's link-speed request while a
kernel-bound VF issues its own request, the worker blocks in rtnl_lock()
held by that VF's sender, so the VF's message cannot be consumed.  The VF
operation then fails with "VSI mailbox timeout" after holding rtnl for
200 ms, and the still-unconsumed message makes the following VF sends fail
with "VSI mailbox is busy" (-EIO) until the worker drains it.

The trigger needs a mixed configuration: one trusted VF issuing link-speed
queries (for example a vfio/DPDK-owned VF, since untrusted requests return
PERM_DENY before rtnl_lock) plus a second kernel-bound ENETC v4 VF on the
same host.  The later patch adding ndo_set_rx_mode_async for the ENETC v4
VF introduces a second rtnl-held 200 ms mailbox waiter on the same path.

[Severity: High]
With SR-IOV now available on ENETC v4, can an untrusted VF repeatedly stall
the PF mailbox worker for 200 ms at a time while pf->msg_lock is held?

enetc_msg_handle_link_status() dispatches
ENETC_MSG_REGISTER_LINK_CHANGE_NOTIFIER to
enetc_msg_register_link_status_notifier() with no ENETC_VF_FLAG_TRUSTED
check, unlike enetc_msg_get_link_speed() which does test the flag.  That
function holds pf->msg_lock across the send:

enetc_msg_register_link_status_notifier() {
	mutex_lock(&pf->msg_lock);
	...
	enetc_msg_notify_vf_link_status(pf, PSIMSGSR_MS(vf_id));
msg_unlock:
	mutex_unlock(&pf->msg_lock);
}

and the send waits for the VF to consume the notification:

enetc_pf_send_msg() {
	...
	return read_poll_timeout(enetc_rd, val, !(val & ms_mask), 1000,
				 200000, false, hw, ENETC_PSIMSGSR);
}

A guest can loop: acknowledge the previous notification so its MS bit is
clear (otherwise the next enetc_pf_send_msg() short-circuits with -EIO),
re-register, and then never read the new notification.  Each iteration
occupies the single VF mailbox work item enetc_msg_task() for 200 ms with
PSIIER interrupts disabled and pf->msg_lock held.

Does that starve the other VFs?  Their enetc_msg_vsi_send() has the same
200 ms timeout, so their ndo_set_mac_address and rx-mode requests would
fail, and the PF link path enetc4_pl_mac_link_up()/enetc4_pl_mac_link_down()
-> enetc_pf_notify_vf_link_up()/_down() -> mutex_lock(&pf->msg_lock) would
stall for up to 200 ms per transition.  Would a trust check on the
registration path, or a non-blocking notification, be preferable here?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831025441.635045-1-wei.fang%40oss.nxp.com