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

[Wei Fang <wei.fang@nxp.com>]

> > +static int enetc_pf_send_msg(struct enetc_pf *pf, u32 msg_code, u16
> ms_mask)
> > +{
> > +	struct enetc_hw *hw = &pf->si->hw;
> > +	u16 old_ms_mask = ms_mask;
> > +	u16 ms_status;
> > +	u32 val;
> > +
> > +	/* The MS bit is set, indicating that the corresponding VF has not
> > +	 * read the last message, PF cannot send new message to the VF. To
> > +	 * avoid sending messages to such a VF, the bit corresponding to VF
> > +	 * is cleared from ms_mask. Because the MS bit can only be written
> > +	 * as 1, writing a 0 has no effect. Writing a 1 when the bit is
> > +	 * already set is undefined.
> > +	 */
> > +	ms_status = enetc_rd(hw, ENETC_PSIMSGSR) & 0xffff;
> > +	ms_mask &= ~ms_status;
> > +	if (!ms_mask)
> > +		return -EIO;
> 
> [Severity: High]
> Can a link state change be lost permanently here?
> 
> The return happens before the enetc_wr(hw, ENETC_PSIMSGSR, ...) below, so
> when the target VF still has an unread message the new link state is never
> published, and enetc_msg_notify_vf_link_status() only prints
> "PF notifies link status failed". Nothing records that the VF is out of
> date, and notifications are generated only on the phylink transitions in
> enetc4_pl_mac_link_up()/enetc4_pl_mac_link_down().
> 
> Single-VF sequence:
> 
>   link up   -> PSIMSGSR written with "link up", the poll below times out
>                after 200 ms because the VF has not read VSIMSGRR yet
>   link down -> ms_status still has MS(0) set, ms_mask becomes 0,
>                -EIO is returned and "link down" is never written
> 
> The VF then consumes the stale "link up" message and keeps carrier on plus
> a stale TX PAUSE derived congestion-mode setting until the next PF link
> transition. The same drop can hit the initial notification issued from
> enetc_msg_register_link_status_notifier(), which has already replied
> ENETC_MSG_CLASS_ID_CMD_SUCCESS to the VF.
> 
> Would it make sense to record a pending state and re-send it once the MS
> bit is observed clear, rather than dropping the transition?

The scenario is theoretically possible, but it does not require a fix.

To lose a transition, a VF would have to leave its previous PF message unread
for longer than the 200 ms send window, and a reverse link change would have
to occur within that same window. That does not happen in practice:

The PHY state machine polls link status once per second, so consecutive link
changes are at least ~1 s apart - more than enough time for the VF to consume
the message and clear its MS bit. In real usage, link transitions are infrequent
to begin with.

The PF also waits up to 200 ms for the VF to read the message. A VF that fails
to read a single message within that window is already malfunctioning (e.g. a
stalled), and I don't think the PF should compensate for a broken VF.

So no fix is needed. Recording a pending state to re-send later is possible, but
since every notification already carries the current state and the next transition
corrects any stale value, the extra complexity is not justified.

> 
> > +
> > +	if (ms_mask != old_ms_mask)
> > +		dev_warn_ratelimited(&pf->si->pdev->dev,
> > +				     "PF cannot send message to VF(s) 0x%x\n",
> > +				     ms_mask ^ old_ms_mask);
> 
> [Severity: Low]
> The printed value is a PSIMSGSR MS-bit mask, not a VF index mask, since
> 
>   #define  PSIMSGSR_MS(n)		BIT((n) + 1)
> 
> so a stuck VF0 is reported as 0x2 and VF1 as 0x4. Should the value be
> shifted back to VF numbering before printing, or should the message say
> that it is an MS mask?

No fix is needed; it stays consistent with the registers, making debugging
easier.

> 
> > +
> > +	enetc_wr(hw, ENETC_PSIMSGSR,
> > +		 FIELD_PREP(PSIMSGSR_MC, msg_code) | ms_mask);
> > +
> > +	return read_poll_timeout(enetc_rd, val, !(val & ms_mask), 1000,
> > +				 200000, false, hw, ENETC_PSIMSGSR);
> > +}
> 
> [Severity: High]
> This waits up to 200 ms for a bit that only the VF can clear, and every
> caller holds pf->msg_lock across it. Can an untrusted guest use that to
> stall the host?
> 
> The VF-initiated path has no ENETC_VF_FLAG_TRUSTED check (unlike the MAC
> filter handlers) and no rate limiting:
> 
>   enetc_msg_task()
>     enetc_msg_handle_rxmsg()
>       enetc_msg_handle_link_status()   /* cmd_id from the VF */
>         enetc_msg_register_link_status_notifier()
>           mutex_lock(&pf->msg_lock);
>           enetc_msg_notify_vf_link_status()
>             enetc_pf_send_msg()        /* sleeps up to 200 ms */
> 
> enetc_msg_task() handles all VFs sequentially in one work item with the MR
> interrupts disabled, so a VF that registers, drains its mailbox and
> re-registers in a loop keeps pf->msg_lock and the mailbox work item busy
> and denies mailbox service to the sibling VFs.
> 
> The same pf->msg_lock is taken by enetc_pf_notify_vf_link_status() from
> enetc4_pl_mac_link_up()/enetc4_pl_mac_link_down(), and phylink runs those
> from its resolve work, which is flushed synchronously from ndo_open/
> ndo_stop with rtnl_lock held:
> 
> drivers/net/phy/phylink.c:phylink_run_resolve_and_disable() {
> 	...
> 	flush_work(&pl->resolve);
> 	...
> }
> 
> so the guest-induced stall propagates into the host PF link handling and
> rtnl-held paths. In the other direction, one unresponsive registered VF
> adds up to 200 ms to every PF link transition, because the poll condition
> !(val & ms_mask) waits for all bits of the aggregate mask.
> 
> Is the synchronous acknowledgement wait needed at all here? Link state is
> idempotent, so a fire-and-forget write with a deferred retry, or at least
> dropping pf->msg_lock before waiting, would keep the PF link state machine
> independent of VF behaviour. The commit message does not mention that a PF
> link transition can now block on VF acknowledgement.

This is a potential issue, I will improve this.

> 
> > +
> > +static void enetc_msg_notify_vf_link_status(struct enetc_pf *pf, u16
> ms_mask)
> > +{
> > +	struct enetc_ndev_priv *priv = netdev_priv(pf->si->ndev);
> > +	u16 pf_msg;
> > +
> > +	pf_msg = enetc_build_link_status_msg(priv, pf->link_up);
> > +	if (enetc_pf_send_msg(pf, pf_msg, ms_mask))
> > +		dev_err_ratelimited(&pf->si->pdev->dev,
> > +				    "PF notifies link status failed\n");
> > +}
> > +
> > +static void enetc_msg_register_link_status_notifier(struct enetc_pf *pf,
> > +						    int vf_id)
> > +{
> > +	u16 pf_msg = FIELD_PREP(ENETC_PF_MSG_CLASS_ID,
> > +				ENETC_MSG_CLASS_ID_CMD_SUCCESS);
> > +
> > +	mutex_lock(&pf->msg_lock);
> > +
> > +	enetc_pf_reply_msg(&pf->si->hw, vf_id, pf_msg);
> > +
> > +	/* SR-IOV is being disabled if pf->sriov_enabled is false, so no
> > +	 * need to set link_status_ms_mask and notify the link status.
> > +	 */
> > +	if (!pf->sriov_enabled)
> > +		goto msg_unlock;
> > +
> > +	pf->link_status_ms_mask |= PSIMSGSR_MS(vf_id);
> 
> [Severity: Medium]
> What clears this bit if the VF goes away without unregistering?
> 
> The bit is set here and cleared only by a cooperative
> ENETC_MSG_UNREGISTER_LINK_CHANGE_NOTIFIER in
> enetc_msg_unregister_link_status_notifier(), or globally in
> enetc_sriov_configure() when SR-IOV is disabled. A VM destroy, a guest
> crash/reboot, or a killed DPDK application leaves the bit set.
> 
> After that, every PF link transition reaches enetc_pf_send_msg() with the
> stale mask and either writes the MS bit for a VF that will never read it
> and then blocks the full 200 ms with pf->msg_lock held from the phylink
> callbacks, or, once the MS bit is stuck set, filters that VF out and (when
> it is the only registered VF) returns -EIO, logging "PF notifies link
> status failed" indefinitely. Since the poll waits for all bits of the mask,
> one dead VF also delays notifications to the live ones.
> 
> The FLR handler added later in the series, enetc4_pf_vf_flr_handler(),
> only restores the promiscuous mode bits and does not touch
> link_status_ms_mask, and enetc_sriov_configure() resets only the software
> mask without bringing PSIMSGSR back to a known state. Should the per-VF
> bit be cleared on FLR / VF teardown as well?

link_status_ms_mask is PF-side software state that records which VFs
have subscribed to link-status notifications. Its lifetime is tied to the VF
driver's register/unregister requests, not to the VF hardware.

FLR resets the VF hardware; it does not mean the VF has stopped wanting
link notifications. A VF can go through an FLR while still being a valid,
registered subscriber, without re-registering. Clearing its bit in the FLR
handler would silently stop delivering link updates to a VF that is still
alive and still subscribed - that would introduce a bug, not fix one.

So the FLR handler should only restore hardware-related state (e.g.
promiscuous filters), and must not touch link_status_ms_mask. Genuine
teardown (VM destroy / VF unbind / SR-IOV disable) already clears the
whole mask in enetc_sriov_configure(), and a graceful VF close sends
an explicit unregister. Clearing the bit on FLR is therefore neither correct
nor necessary.

As for the "VF goes away without unregistering" case, a follow-up
improvement will remove the 200 ms synchronous wait in
enetc_pf_send_msg() (moving to a fire-and-forget notification). Once that
is in place, a dead VF that never reads its message can no longer block the
PF link state machine: the pre-send MS check simply skips that VF, and
notifications to the live VFs are unaffected. So this concern will be mitigated
by that change.

> 

--
pw-bot: cr