RE: [PATCH v4 net-next 11/15] net: enetc: restore VF MAC promiscuous mode after FLR for ENETC v4

[Wei Fang]

> > @@ -542,6 +559,29 @@ static void enetc_msg_handle_rxmsg(struct
> enetc_pf *pf, int vf_id,
> >  	kfree(msg);
> >  }
> >
> > +static void enetc_vf_flr_handler(struct enetc_pf *pf) {
> > +	u32 flr_mask = ENETC_VFFLR_MASK(pf->num_vfs);
> > +	struct enetc_hw *hw = &pf->si->hw;
> > +	u32 flr_status;
> > +
> > +	if (!pf->ops->vf_flr_handler)
> > +		return;
> > +
> > +	flr_status = enetc_rd(hw, ENETC_PSIIDR) & flr_mask;
> > +	if (!flr_status)
> > +		return;
> > +
> > +	for (int i = 0; i < pf->num_vfs; i++) {
> > +		if (!(ENETC_VFFLR_BIT(i) & flr_status))
> > +			continue;
> > +
> > +		/* Clear FLR interrupt status, W1C */
> > +		enetc_wr(hw, ENETC_PSIIDR, ENETC_VFFLR_BIT(i));
> > +		pf->ops->vf_flr_handler(pf, i);
> > +	}
> > +}
> > +
> >  static void enetc_msg_task(struct work_struct *work)  {
> >  	struct enetc_si *si = container_of(work, struct enetc_si, msg_task);
> > @@ -550,6 +590,8 @@ static void enetc_msg_task(struct work_struct *work)
> >  	u32 mr_status, mr_mask;
> >  	int i;
> >
> > +	enetc_vf_flr_handler(pf);
> > +
> 
> [Severity: Medium]
> How long can the SI stay in the hardware promiscuous default before this
> restore runs?
> 
> The interrupt handler only masks PSIIER and queues the work:
> 
> drivers/net/ethernet/freescale/enetc/enetc_msg.c:enetc_msg_psi_msix() {
> 	enetc_disable_psiier_interrupts(pf);
> 	schedule_work(&si->msg_task);
> }
> 
> so between the FLR taking effect (PSIPMMR[SIn_MAC_UP]/[SIn_MAC_MP] back
> at their promiscuous reset values) and enetc_msg_task() ->
> enetc_vf_flr_handler() -> enetc4_pf_vf_flr_handler() running, the SI keeps
> receiving all port unicast and multicast traffic. Since the hardware default
> ignores trust state, does this window also apply to an untrusted VF that has
> never been granted promiscuous mode?
> 
> Is the window bounded here? PSIIER is only re-armed at the very end of
> 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);
> 
> so an FLR latched while the message loop is in progress is not serviced until that
> loop finishes. For a trusted VF the loop can reach enetc_msg_get_link_speed(),
> which blocks on the global lock:
> 
> 	rtnl_lock();
> 	phylink_ethtool_ksettings_get(priv->phylink, &link_info);
> 	rtnl_unlock();
> 
> Can a guest keep msg_task busy this way while another VF (or the same one) sits
> in the FLR-induced promiscuous state?
> 
> Would it make sense to force UC/MC promiscuous off for the affected SI as soon
> as the FLR event is observed, before the tracked policy is re-applied, and to note
> the residual window in the commit message?
> 

I don't think this window is worth fixing - the risk is very low and the proposed
change adds complexity for little gain.

First, the exposure is theoretical rather than practically useful. A VF FLR resets
the entire VF function, not just PSIPMMR - the Rx BD rings, SI enable state and
DMA configuration are all reset too. So immediately after the FLR the VF has no
armed Rx ring and cannot receive any frame, promiscuous or not. Before it can
capture anything it must first re-initialize and re-arm an Rx ring, which is not
instantaneous. The promiscuous state is only the reset default and is corrected
by the PF asynchronously; there is no guarantee the VF can bring up a ring and
line it up with that short window to actually sniff another SI's traffic. It cannot
reliably exploit the timing.

Second, under normal conditions the detect-to-restore latency is very short
(interrupt latency plus a workqueue wakeup, between tens and hundreds of
microseconds).

Third, moving the promiscuous-mode clear into enetc_msg_psi_msix() would
not give a real guarantee anyway: the guest owns the FLR trigger, so it can simply
issue another FLR before the workqueue runs and return the SI to the reset-default
promiscuous state. So the hardirq clear does not close the window in any
meaningful sense.

So I'd keep the current design: clear/re-apply the policy from the work item.
No change needed.