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

quoted

+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").

In this particular case, it is expected that VF will obtain an error code, and I
don't think there is anything wrong with that. Furthermore, aside from the
IP revision being obtained during VF probes, which is not under RTNL lock
protection, all other mailbox messages are within the RTNL lock context, so
the situation you mentioned doesn't exist.

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?

The VF side should handle these errors.

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?

The commit message has explained why rntl_lock is used:
The speed is read via phylink_ethtool_ksettings_get() rather than the
speed passed to the mac_link_up() callback. When the MAC has a PCS
layer, mac_link_up() reports the PCS link speed, which may differ from
the external PHY link speed; phylink_ethtool_ksettings_get() returns the
actual external link speed.

[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'?

The link speed query is a infrequent message. Normally, query speed is only
activated when the link status changes. A trusted VF means it will not perform
the abnormal behaviors; otherwise, the "trust" would be meaningless.