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

[Wei Fang]

> >  static void enetc_msg_task(struct work_struct *work)
> >  {
> >  	struct enetc_si *si = container_of(work, struct enetc_si, msg_task);
> > @@ -602,6 +642,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 state before this
> restore runs?
> 
> The hardware puts the SI into UC/MC promiscuous mode at the instant of the
> FLR, but the policy is only re-applied from the work item:
> 
> enetc_msg_psi_msix() {
> 	enetc_disable_psiier_interrupts(pf);
> 	schedule_work(&si->msg_task);
> 
> 	return IRQ_HANDLED;
> }
> 
> enetc_msg_task() -> enetc_vf_flr_handler() -> pf->ops->vf_flr_handler()
> 	-> enetc4_pf_vf_flr_handler()
> 
> Since the guest owning the VF triggers the FLR itself (vfio-pci reset, or a
> driver bind path reaching pcie_flr()), it knows exactly when that window
> opens and only needs to re-arm an Rx BD ring to receive frames destined for
> other SIs until enetc4_pf_vf_flr_handler() clears the bits in PSIPMMR.
> 
> The scan also happens once, at the top of enetc_msg_task():
> 
> 	enetc_vf_flr_handler(pf);
> 
> 	mr_mask = ENETC_PSIMR_MASK(pf->num_vfs);
> 
> and enetc_vf_flr_handler() takes a single PSIIDR snapshot with an early
> return:
> 
> 	flr_status = enetc_rd(hw, ENETC_PSIIDR) & flr_mask;
> 	if (!flr_status)
> 		return;
> 
> If an FLR lands while msg_task is already part-way through the VF message
> loop, is the restore then delayed until the whole in-flight batch finishes
> and the re-queued work runs?  The FLR bits are not re-checked before
> enetc_enable_psiier_interrupts(pf) at the end of the work item.
> 
> Would it be feasible to clear PSIPMMR[SIn_MAC_UP]/[SIn_MAC_MP] directly in
> enetc_msg_psi_msix() for the VFs whose FLR bits are set, and leave the full
> policy re-apply in the work item?  As written the handler sleeps on
> vf_state->lock and pf->msg_lock, so it cannot run from the hardirq.
> 

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

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.

Finally, it would add real complexity. enetc4_pf_vf_flr_handler() accesses
PSIPMMR under vf_state->lock and pf->msg_lock, which are mutexes and
cannot be taken in hardirq context. Doing the clear in the ISR would require
converting the PSIPMMR synchronization to an irq-safe spinlock, which touches
every path that writes PSIPMMR (set_rx_mode, the promisc message handler,
trust-off, clear_vf_config). That is a non-trivial change to the locking model for
a window that is not practically exploitable.

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