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

[Wei Fang]

> > @@ -181,6 +212,11 @@ struct enetc_msg_mac_exact_filter {
> >   * cmd_id 0x0: get the current link status
> >   * cmd_id 0x1: register link status change notification
> >   * cmd_id 0x2: unregister link status change notification
> > + *
> > + * Link speed message, class_id 0x81.
> > + * cmd_id 0x0: get the current link speed.
> > + * cmd_id 0x1: register link speed change notification, not supported yet
> > + * cmd_id 0x2: unregister link speed change notification, not supported yet
> >   */
> 
> [Severity: Medium]
> 
> This documents cmd_id 0x0 as returning the current link speed, but can the
> reply describe a speed the port is not running at?
> 
> enetc_msg_get_link_speed() takes the value straight from
> phylink_ethtool_ksettings_get() with no link-state gating.  For a fixed
> link, phylink copies the configured speed/duplex unconditionally:
> 
> drivers/net/phy/phylink.c:phylink_ethtool_ksettings_get() {
> 	...
> 	case MLO_AN_FIXED:
> 		phylink_get_fixed_state(pl, &link_state);
> 		phylink_get_ksettings(&link_state, kset);
> 		break;
> 	...
> }
> 
> So with the carrier down, or before the PF netdev has ever been opened,
> enetc_build_link_speed_msg() still encodes a valid speed class code rather
> than ENETC_MSG_SPEED_UNKNOWN.  The MLO_AN_INBAND case reads the
> PCS live,
> which also need not match what the MAC was last programmed with.
> 
> Would it be more consistent to derive the answer from the state the PF
> already tracks, the way the sibling link-status message does?

No fix is needed here. Message class 0x81 (link speed) and class 0x80 (link
status) are intentionally separate, and each returns the correct thing.
> 
> drivers/net/ethernet/freescale/enetc/enetc_msg.c:enetc_msg_get_link_status()
> {
> 	...
> 	pf_msg = enetc_build_link_status_msg(priv, pf->link_up);
> 	...
> }
> 
> priv->speed is written next to PCR_PSPEED in enetc4_set_port_speed()
> (including the 10Mbps fallback on SPEED_UNKNOWN), so the programmed rate
> is
> already recorded.  As written, a VF can be told 1G/2.5G while the same PF
> reports the link as down over class 0x80.

GET_CURRENT_LINK_SPEED is meant to expose the same value userspace
would see via ethtool - the port's configured/negotiated speed/duplex. That
is a property that remains meaningful even when the carrier is down (for a
fixed link, ethtool likewise reports the configured speed regardless of link state).
This message never claims the link is currently up; whether the link is up is
answered by class 0x80, which the DPDK VF is expected to query separately

> 
> > diff --git a/drivers/net/ethernet/freescale/enetc/enetc_msg.c
> b/drivers/net/ethernet/freescale/enetc/enetc_msg.c
> > index e21414acdc0d..c3ae4c024f34 100644
> > --- a/drivers/net/ethernet/freescale/enetc/enetc_msg.c
> > +++ b/drivers/net/ethernet/freescale/enetc/enetc_msg.c
> > @@ -280,6 +280,93 @@ static u16 enetc_msg_handle_link_status(struct
> enetc_pf *pf, int vf_id,
> >  	return 0;
> >  }
> >
> > +static u16 enetc_build_link_speed_msg(int speed, int duplex)
> > +{
> > +	u32 speed_code = ENETC_MSG_SPEED_UNKNOWN;
> > +
> > +	switch (speed) {
> 
> [ ... ]
> 
> > +	return FIELD_PREP(ENETC_PF_MSG_CLASS_ID,
> > +			  ENETC_MSG_CLASS_ID_LINK_SPEED) |
> > +	       FIELD_PREP(ENETC_PF_MSG_CLASS_CODE_U8, speed_code);
> > +}
> > +
> > +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);
> 
> [Severity: Medium]
> 
> Should the trusted-VF requirement be described in the changelog and in the
> mailbox ABI comment in enetc_mailbox.h?
> 
> ENETC_VF_FLAG_TRUSTED is only ever set from enetc_pf_set_vf_trust()
> (ndo_set_vf_trust, i.e. "ip link set <pf> vf N trust on"), and pf->vf_state
> is allocated with devm_kcalloc() in enetc_init_sriov_resources(), so flags
> is zero by default.  A default setup therefore gets
> ENETC_PF_MSG_PERM_DENY / -EACCES back from enetc_msg_vsi_send().
> 
> The commit message says without qualification:
> 
>   "The PF will reply the current link speed when it receives the get link
>    speed message from VF."
> 
>   "VFs controlled by DPDK can obtain real-time link speed information from
>    the PF"
> 
> The ABI comment added to struct enetc_msg_generic also describes class 0x81
> cmd 0x0 with the same wording as the 0x80 status query, and
> enetc_msg_get_link_status() answers any VF with no trust check.  Two paired
> link-info queries with different permission models is surprising for a
> DPDK VF implementer reading only the header.

The asymmetry is intentional and technically justified, but you're right that it
should be documented. I'll update the changelog to state that the get-link-speed
query requires the VF to be trusted, and annotate the class 0x81 cmd 0x0 entry
in enetc_mailbox.h to note the trusted-VF requirement, distinguishing it from the
ungated 0x80 status query.

> 
> > +
> > +	rtnl_lock();
> > +	phylink_ethtool_ksettings_get(priv->phylink, &link_info);
> > +	rtnl_unlock();
> 
> [Severity: High]
> 
> Can blocking on rtnl here stall mailbox service for the other VFs?
> 
> This runs from the single work item that services every VF with the PSIIER
> MR interrupts masked:
> 
> drivers/net/ethernet/freescale/enetc/enetc_msg.c:enetc_msg_task() {
> 	...
> 	for (i = 0; i < pf->num_vfs; i++) {
> 		...
> 		enetc_msg_handle_rxmsg(pf, i, &msg_code);
> 		...
> 	}
> out:
> 	enetc_enable_psiier_interrupts(pf);
> }
> 
> rtnl hold times elsewhere (ndo_open with PCS/PHY negotiation, netdev
> unregistration, another driver probing, netlink dumps) are unbounded, while
> the VF side has a hard 200ms budget:
> 
> drivers/net/ethernet/freescale/enetc/enetc_vf.c: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");
> 	...
> }
> 
> So one iteration waiting on rtnl can push unrelated requests from other VFs
> past that timeout, with a hardware mailbox transaction left outstanding.
> For a host-assigned VF, enetc_msg_vsi_set_primary_mac_addr() is issued from
> ndo_set_mac_address with rtnl already held, i.e. the VF waits for a PF reply
> that needs the very lock the VF holds.
> 
> There is a second effect on teardown: enetc_msg_psi_free() does
> cancel_work_sync(&si->msg_task), and it is called from
> enetc_sriov_configure() (pci_driver::sriov_configure, invoked from
> sriov_numvfs_store() under the PCI device_lock) and from
> enetc_pf_remove()/enetc4_pf_remove().  Does SR-IOV disable/removal now
> wait
> for rtnl while holding device_lock, for a time a busy VF can extend?
> 
> Every other handler in this file avoids rtnl.  enetc_msg_get_link_status()
> answers from the cached pf->link_up under pf->msg_lock, and speed/duplex are
> already handed to enetc4_pl_mac_link_up() where they could be cached the
> same way.  Would caching the speed there and replying from the cache work
> instead?

this does not need a fix. Using phylink_ethtool_ksettings_get() here is deliberate
rather than an oversight, and the practical impact is negligible.

The speed passed into enetc4_pl_mac_link_up() is not always the actual link speed.
When there is a PCS after the MAC and rate matching is in effect, the speed given
to the callback is the PCS/interface rate, not the external PHY link speed. Only
phylink_ethtool_ksettings_get() reflects the real media/PHY link speed. So caching
the callback speed and replying from it could hand the VF the PCS rate instead of
the actual PHY link speed - exactly what DPDK needs to get right. Querying phylink
guarantees the VF sees the same link speed that ethtool reports on the PF.

In real usage a DPDK VF does not poll link speed in a tight loop - it queries it
occasionally (typically around link-change events), not on a hot path. Combined
with the existing trusted-VF gate, the chance of this rtnl_lock() acquisition
meaningfully contending with host control-plane operations is negligible.
There is no measurable stall in practice.

Given that the cached value can be semantically wrong for PCS/rate-matching
setups, and the rtnl acquisition is rare and already restricted to trusted VFs,
keeping the phylink query is the correct trade-off. No change is needed.