Re: [PATCH v4 net-next 04/15] net: enetc: add link speed message support to PF driver

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

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

- [Medium] enetc_msg_get_link_speed()
  (drivers/net/ethernet/freescale/enetc/enetc_msg.c:274) introduces an…

--

Verified the relevant code paths. Report below.

net: enetc: add link speed message support to PF driver

Adds a new mailbox message class 0x81 so a VF can query the current link
speed from the PF.  The PF reads the speed with
phylink_ethtool_ksettings_get() under rtnl_lock() and encodes it into the
8-bit class code of the reply, restricting the query to trusted VFs.

[ ... ]

+static u16 enetc_msg_get_link_speed(struct enetc_pf *pf, int vf_id)
+{
+	struct enetc_ndev_priv *priv = netdev_priv(pf->si->ndev);
+	struct enetc_vf_state *vf_state = &pf->vf_state[vf_id];
+	struct ethtool_link_ksettings link_info = {};
+
+	/* A malicious or malfunctioning VM could potentially spam these
+	 * messages in a tight loop causing global rtnl_lock contention,
+	 * which may severely starve other processes on the host that
+	 * require rtnl_lock for routine network configuration, resulting
+	 * in a system-wide control-plane denial of service. Therefore,
+	 * we expect the VF query for link speed to be trusted. There's no
+	 * need to consider the transition from trusted to untrusted here,
+	 * as this won't cause rtnl_lock() to be called frequently.
+	 */
+	mutex_lock(&vf_state->lock);
+	if (!(vf_state->flags & ENETC_VF_FLAG_TRUSTED)) {
+		mutex_unlock(&vf_state->lock);
+
+		return ENETC_PF_MSG_PERM_DENY;
+	}
+	mutex_unlock(&vf_state->lock);
+
+	rtnl_lock();
+	phylink_ethtool_ksettings_get(priv->phylink, &link_info);
+	rtnl_unlock();

[Severity: Medium]
Is it safe to block on rtnl_lock() inside the PSI mailbox service work
item?  This looks like the first sleeping, host-global lock acquisition on
that path: enetc_msg_handle_link_status() only uses si->gen_lock and never
sleeps.

The completion code is only written back to the VF after the class handler
returns, in enetc_msg_task():

	for (i = 0; i < pf->num_vfs; i++) {
		...
		enetc_msg_handle_rxmsg(pf, i, &msg_code);
		...
		enetc_pf_reply_msg(hw, i, msg_code);
	}

out:
	enetc_msg_enable_mr_int(pf);

while the VF side waits with a fixed 200 ms bound in enetc_msg_vsi_send():

	err = read_poll_timeout(enetc_rd, vsimsgsr,
				!(vsimsgsr & ENETC_VSIMSGSR_MB),
				1000, 200000, false, &si->hw, ENETC_VSIMSGSR);
	if (err) {
		dev_err(dev, "VSI mailbox timeout\n");

		return err;
	}

If any unrelated host rtnl holder (for example 'ip link del' going through
synchronize_net(), an ndo_open of another device, bonding/team
reconfiguration or a module load) keeps rtnl for longer than ~200 ms, does
the reply not arrive after the VF has already given up?  In that case the
VF logs "VSI mailbox timeout" and returns -ETIMEDOUT, and because
VSIMSGSR[MB] stays set until the PF finally replies, subsequent VF sends
hit the earlier check and fail with -EIO ("VSI mailbox is busy").

There is a second effect from the same wait.  enetc_msg_task() services all
VFs serially in one loop and only re-arms the PSIIER MR interrupt sources at
enetc_msg_enable_mr_int(pf) on exit, so while one VF's speed query sleeps on
rtnl, do the pending messages of the other VFs (set primary MAC, MAC hash
filter, promisc mode) not get delayed and potentially time out too?

Would a non-blocking source for the speed work here, or alternatively the
deferred-reply mechanism (ENETC_MSG_CLASS_ID_CMD_DEFERRED) so the mailbox
handshake is not held open across rtnl?

[Severity: Medium]
On the trust gate itself: the comment above the mutex_lock(&vf_state->lock)
check describes the tight-loop spam scenario, but the check only narrows
which VFs can reach rtnl_lock(), it does not bound the rate.  Nothing
throttles inbound class 0x81 messages - the MR interrupt sources are simply
re-armed at the end of enetc_msg_task(), and the only per-VF counter
(msg_fail_cnt) tracks PF-to-VF send failures rather than inbound queries.

Can a trusted VF still drive repeated rtnl acquire/release from the PF work
item at mailbox round-trip rate, i.e. is the primitive the comment warns
about still reachable from guest-controlled code once an admin sets 'trust
on'?

For reference, a couple of related concerns were checked and do not appear
to be problems: vf_state->lock is dropped before rtnl_lock() so there is no
ordering issue against ndo_set_vf_trust, and enetc_msg_psi_free() (and its
cancel_work_sync()) is only reached from enetc_sriov_configure(), which does
not hold rtnl.

+
+	return enetc_build_link_speed_msg(link_info.base.speed,
+					  link_info.base.duplex);
+}

[ ... ]

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