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

[Wei Fang]

> > +static void enetc_msg_register_link_status_notifier(struct enetc_pf *pf,
> > +						    int vf_id)
> > +{
> > +	struct enetc_si *si = pf->si;
> > +
> > +	spin_lock(&si->gen_lock);
> > +	enetc_pf_reply_msg(&si->hw, vf_id, ENETC_PF_MSG_SUCCESS);
> > +
> > +	/* 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) {
> > +		spin_unlock(&si->gen_lock);
> > +		return;
> > +	}
> > +
> > +	pf->link_status_ms_mask |= PSIMSGSR_MS(vf_id);
> > +	spin_unlock(&si->gen_lock);
> > +
> > +	/* Notify VF the current link status */
> > +	queue_work(si->workqueue, &pf->link_status_task);
> > +}
> 
> [Severity: Medium]
> This queues the shared, VF-agnostic link_status_task in order to deliver one
> VF's initial status. Does that mean a single VF registering re-sends the
> notification to every VF currently set in link_status_ms_mask?

For a VF that has already obtained the link status, obtaining the same message
again will not have any impact, and the VF should ignore consecutive identical
link status messages.

> 
> The retry decision in enetc_pf_send_link_status_msg() is made on the
> aggregate mask rather than per VF:
> 
> 	ms_status = enetc_rd(&si->hw, ENETC_PSIMSGSR) & 0xfffe;
> 	if ((ms_mask & ms_status) && retry_num++ < 200) {
> 		spin_unlock(&si->gen_lock);
> 		usleep_range(1000, 1020);
> 		goto retry;
> 	}
> 
> Can one VF that never reads its mailbox therefore add up to roughly 200 ms
> of sleeping to every registration and every link transition seen by all the
> other registered VFs?

This situation will not have any impact; it will still send message notifications
to the normally functioning VF after the timeout.

> 
> The ms_status_check loop also advances vf_state->msg_fail_cnt for every VF
> in ms_mask. Can a VF that happened to be busy during a notification
> triggered by some other VF's registration accumulate failures it did not
> cause, and eventually be evicted from link_status_ms_mask?

We set the failure count to 10, and the device will only be removed after ten
consecutive failures. If this happens, it means that the VF is abnormal, and
removal is the correct procedure.

> 
> > +
> > +static void enetc_msg_unregister_link_status_notifier(struct enetc_pf *pf,
> > +						      int vf_id)
> > +{
> > +	spin_lock(&pf->si->gen_lock);
> > +	pf->link_status_ms_mask &= ~PSIMSGSR_MS(vf_id);
> > +	enetc_pf_reply_msg(&pf->si->hw, vf_id, ENETC_PF_MSG_SUCCESS);
> > +	spin_unlock(&pf->si->gen_lock);
> > +}
> 
> [Severity: Low]
> Neither this function nor enetc_msg_register_link_status_notifier() resets
> vf_state->msg_fail_cnt.
> 
> If a VF accumulated up to 10 consecutive delivery failures under a previous
> registration, does the first busy notification after it re-registers
> immediately clear its bit from link_status_ms_mask?

Yes, this is intentional. A normal VF can definitely process mailbox messages
within 200ms; otherwise, it's an abnormal VF deliberately creating a timeout.

> 
> Later in the series the Linux VF driver unregisters on ndo_close and
> registers again on ndo_open, so the counter survives an ifdown/ifup cycle.
> Would clearing vf_state->msg_fail_cnt in the register path be preferable?

No, see above.

> 
> > +
> > +static u16 enetc_msg_handle_link_status(struct enetc_pf *pf, int vf_id,
> > +					void *vf_msg)
> > +{
> > +	struct enetc_msg_header *msg_hdr = vf_msg;
> > +
> > +	switch (msg_hdr->cmd_id) {
> > +	case ENETC_MSG_GET_CURRENT_LINK_STATUS:
> > +		/* Currently, this message is intended only for
> > +		 * DPDK-owned VFs.
> > +		 */
> > +		enetc_msg_get_link_status(pf, vf_id);
> > +		break;
> > +	case ENETC_MSG_REGISTER_LINK_CHANGE_NOTIFIER:
> > +		enetc_msg_register_link_status_notifier(pf, vf_id);
> > +		break;
> 
> [Severity: Medium]
> This dispatch has no ENETC_VF_FLAG_TRUSTED check and no rate limiting,
> unlike enetc_msg_set_vf_primary_mac_addr() and the other MAC filter
> handlers in this file. The link speed handler added later in the series
> documents exactly this abuse class in its own comment ("A malicious or
> malfunctioning VM could potentially spam these messages in a tight
> loop...").
> 
> The registration handler replies ENETC_PF_MSG_SUCCESS first, which clears
> the VF's MR bit and lets the VF post the next message right away, then
> calls queue_work() unconditionally.
> 
> Can a VF assigned to an untrusted guest register, never consume its
> mailbox, and keep re-sending
> ENETC_MSG_REGISTER_LINK_CHANGE_NOTIFIER so
> that the PF's single ordered workqueue spends 200 x ~1 ms sleeping on each
> invocation, delaying carrier notifications for the co-tenant VFs?
> 
> The 10-failure eviction does not appear to bound this, since the VF can
> simply register again and restore its bit in link_status_ms_mask.

Link status is a basic, read-only, non-privileged capability that every VF
needs to report carrier state to its guest stack/containers/VMs. Gating
it behind trusted would break the common untrusted-VF use case without
closing any real attack surface, since mailbox spam is already bounded by
the idempotent work, busy-VF exclusion, the 10-failure eviction, and
rate-limited logging.

Furthermore, on the PF side, the PHY driver itself checks the link status
once per second, so the interval between two link status updates is at
least greater than 1 second, thus it will not affect the link status notifications
of other VFs.

> 
> > +	case ENETC_MSG_UNREGISTER_LINK_CHANGE_NOTIFIER:
> > +		enetc_msg_unregister_link_status_notifier(pf, vf_id);
> > +		break;
> > +	default:
> > +		return ENETC_PF_MSG_NOTSUPP;
> > +	}
> > +
> > +	return 0;
> > +}
> 
> [ ... ]
> 
> > @@ -340,6 +442,22 @@ static int enetc_msg_psi_init(struct enetc_pf *pf)
> >  	return err;
> >  }
> >
> > +static void enetc_msg_clear_vf_config(struct enetc_pf *pf, int vf_id)
> > +{
> > +	struct enetc_vf_state *vf_state = &pf->vf_state[vf_id];
> > +	struct enetc_si *si = pf->si;
> > +
> > +	/* For ENETC v1, we only support setting the VF's MAC address via
> > +	 * VSI-to-PSI messages, so there is no configuration to clear.
> > +	 */
> > +	if (is_enetc_rev1(si))
> > +		return;
> > +
> > +	spin_lock(&si->gen_lock);
> > +	vf_state->msg_fail_cnt = 0;
> > +	spin_unlock(&si->gen_lock);
> > +}
> 
> [Severity: Medium]
> This resets software state only; the hardware PSIMSGSR[MS] bit for that VSI
> is left set. The comment in enetc_pf_send_link_status_msg() states that the
> PF cannot write the MS bit as 0, so only the VSI can clear it by reading
> VSIMSGRR.
> 
> Is there any path that disarms an MS bit left set by a VF that stopped
> consuming messages, for example a crashed or paused guest, a VF FLR, a VFIO
> release, or a killed DPDK application?

Usually, the VF driver will perform FLR at probe, the FLR will clear the state.

> 
> The commit message says:
> 
>   "The link_status_ms_mask tracks the VFs registered for notification and
>    is cleared when SR-IOV is disabled."
> 
> but a VF FLR clears neither link_status_ms_mask nor msg_fail_cnt: the
> enetc4_pf_vf_flr_handler() added later in the series restores only the
> promiscuous state. Does a re-created VF at the same index then look
> registered to the PF while the notification channel to it is blocked?

As clarified above, the VF FLR will clear hardware state, so the notification
will not be blocked.

> 
> Two follow-on effects in enetc_pf_send_link_status_msg(): every later PF
> link transition burns up to 200 x ~1 ms of polling because the retry gate
> is keyed on the aggregate mask, and after 11 such events the VF's bit is
> dropped from link_status_ms_mask with only a dev_warn_ratelimited(), while
> the VF still considers itself registered because it was acked with
> ENETC_PF_MSG_SUCCESS. Should there be an error reply, a re-arm, or a
> periodic retry so the VF's carrier state and its RX BD congestion mode
> decision (driven by the TX PAUSE bit) do not stay stale until an
> administrative ifdown/ifup?

The link status message is sent proactively by the PF to the VF, not by the
VF actively querying the link status. Therefore, it's impossible for the PF
to send an error response to the VF. Furthermore, since the VF is no longer
able to respond to link status messages at this point, it also won't receive
error notifications proactively sent by the PF.

Recovery is possible if the VF actively performs an ifup/down operation.

> 
> The later VF patch does drain VSIMSGRR in enetc_vf_wq_task_destroy(), but
> that covers only the clean Linux VF unbind case, not FLR or guest death.
> 
> >  static void enetc_msg_psi_free(struct enetc_pf *pf)
> >  {
> >  	struct enetc_si *si = pf->si;
> 
> [ ... ]
> 
> > @@ -367,6 +487,11 @@ int enetc_sriov_configure(struct pci_dev *pdev, int
> num_vfs)
> >  	int err;
> >
> >  	if (!num_vfs) {
> > +		spin_lock(&si->gen_lock);
> > +		pf->sriov_enabled = false;
> > +		pf->link_status_ms_mask = 0;
> > +		spin_unlock(&si->gen_lock);
> > +
> >  		pci_disable_sriov(pdev);
> >  		enetc_msg_psi_free(pf);
> >  		pf->num_vfs = 0;
> 
> [ ... ]
> 
> > @@ -396,3 +535,114 @@ int enetc_sriov_configure(struct pci_dev *pdev, int
> num_vfs)
> >  	return err;
> >  }
> >  EXPORT_SYMBOL_GPL(enetc_sriov_configure);
> > +
> > +void enetc_pf_send_link_status_msg(struct enetc_pf *pf)
> > +{
> > +	struct enetc_ndev_priv *priv = netdev_priv(pf->si->ndev);
> > +	u16 pf_msg, ms_mask, new_ms_msk, ms_status;
> > +	struct enetc_si *si = pf->si;
> > +	int retry_num = 0;
> > +
> > +retry:
> > +	spin_lock(&si->gen_lock);
> > +	ms_mask = pf->link_status_ms_mask;
> > +	/* VFs have unregistered link status notification, return directly  */
> > +	if (!ms_mask)
> > +		goto unlock;
> > +
> > +	/* 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(&si->hw, ENETC_PSIMSGSR) & 0xfffe;
> > +	if ((ms_mask & ms_status) && retry_num++ < 200) {
> > +		spin_unlock(&si->gen_lock);
> > +		/* Wait VFs to handle the last message */
> > +		usleep_range(1000, 1020);
> > +		goto retry;
> > +	}
> > +
> > +	/* None of the relevant VFs have processed the previous message, and
> > +	 * the PF has tried 200 times. This situation indicates that VF has
> > +	 * malfunctioned.
> > +	 */
> > +	new_ms_msk = ms_mask & (~ms_status);
> > +	if (!new_ms_msk) {
> > +		dev_err_ratelimited(&si->pdev->dev,
> > +				    "All registered VFs (MS: 0x%x) are busy\n",
> > +				    ms_mask);
> > +		goto ms_status_check;
> > +	}
> > +
> > +	if (new_ms_msk != ms_mask)
> > +		dev_warn_ratelimited(&si->pdev->dev,
> > +				     "Failed to notify link status to VFs (MS: 0x%x)\n",
> > +				     ms_mask ^ new_ms_msk);
> > +
> > +	pf_msg = enetc_build_link_status_msg(priv, pf->link_up);
> > +	enetc_wr(&si->hw, ENETC_PSIMSGSR,
> > +		 FIELD_PREP(PSIMSGSR_MC, pf_msg) | new_ms_msk);
> 
> [Severity: Medium]
> Delivery here looks one-shot. Both the "All registered VFs ... are busy"
> path and the new_ms_msk != ms_mask path abandon the pending notification:
> link_status_task is not re-queued and nothing records that the excluded VFs
> still owe a message, only a rate-limited log line is emitted.
> 
> Is the ms_status snapshot also racy? A VSI clears its own MS bit
> asynchronously and outside si->gen_lock, so if a VF clears its bit between:
> 
> 	ms_status = enetc_rd(&si->hw, ENETC_PSIMSGSR) & 0xfffe;
> 
> and:
> 
> 	enetc_wr(&si->hw, ENETC_PSIMSGSR,
> 		 FIELD_PREP(PSIMSGSR_MC, pf_msg) | new_ms_msk);
> 
> does that VF consume the previous message content and never receive the
> current link state? gen_lock cannot close this window because the MS bit is
> mutated by a peer PCI function.

This is a limitation; the software cannot guarantee that the VF will not clear
the MS bit during this window. However, this is only a theoretical issue. Before
the operation, the PF has already waited for 200ms, which is more than enough
time for a normally functioning VF to process the previous message. Therefore,
I don't think it's necessary to eliminate this theoretical issue.

> 
> With the VF side added later in the series, a phy-less v4 VF relies
> entirely on this notification for its carrier: enetc_phylink_connect()
> returns without calling netif_carrier_on() when
> vf_reg_link_status_notifier() succeeds. Does a dropped initial notification
> then leave the VF netdev with no carrier until some later PF link change or
> an administrative ifdown/ifup?

When PF sends a message, VF will generate an interrupt for processing. First,
I don't think the interrupt will be lost. Second, normal interrupt processing is
usually in the microsecond range. We have set the timeout to 200ms, so a
normal VF won't lose the notification. For a VF that malfunctions, we do not
expect it to process notifications correctly. If it subsequently recovers, the link
message notification can be resumed using ifdown/ifup.