RE: [PATCH v3 net-next 12/15] net: enetc: add VF support for i.MX94 and i.MX95

[Wei Fang]

> > @@ -1138,6 +1141,7 @@ static struct pci_driver enetc4_pf_driver = {
> >  	.id_table = enetc4_pf_id_table,
> >  	.probe = enetc4_pf_probe,
> >  	.remove = enetc4_pf_remove,
> > +	.sriov_configure = enetc_sriov_configure,
> >  };
> >  module_pci_driver(enetc4_pf_driver);
> 
> [Severity: Medium]
> Enabling .sriov_configure here, together with the new 0xef00 VF ID, makes
> the PSI/VSI mailbox path live on ENETC v4 for the first time.  Can the PF
> mailbox worker and a VF mailbox sender deadlock against each other on
> rtnl_lock until the poll times out?
> 
> The PF worker path takes rtnl_lock while serving a VF request:
> 
> enetc_msg_task() -> enetc_msg_handle_rxmsg() ->
> enetc_msg_handle_link_speed()
>   -> enetc_msg_get_link_speed() {
> 	rtnl_lock();
> 	phylink_ethtool_ksettings_get(priv->phylink, &link_info);
> 	rtnl_unlock();
>   }
> 
> The VF side runs from ndo_set_mac_address, so rtnl is already held, and it
> then waits up to 200 ms for the PF to answer:
> 
> enetc_vf_set_mac_addr() -> enetc_msg_vsi_set_primary_mac_addr()
>   -> enetc_msg_vsi_send() {
> 	err = read_poll_timeout(enetc_rd, vsimsgsr,
> 				!(vsimsgsr & ENETC_VSIMSGSR_MB),
> 				1000, 200000, false, &si->hw, ENETC_VSIMSGSR);
>   }
> 
> If the single worker is servicing a trusted VF's link-speed request while a
> kernel-bound VF issues its own request, the worker blocks in rtnl_lock()
> held by that VF's sender, so the VF's message cannot be consumed.  The VF
> operation then fails with "VSI mailbox timeout" after holding rtnl for
> 200 ms, and the still-unconsumed message makes the following VF sends fail
> with "VSI mailbox is busy" (-EIO) until the worker drains it.
> 
> The trigger needs a mixed configuration: one trusted VF issuing link-speed
> queries (for example a vfio/DPDK-owned VF, since untrusted requests return
> PERM_DENY before rtnl_lock) plus a second kernel-bound ENETC v4 VF on the
> same host.  The later patch adding ndo_set_rx_mode_async for the ENETC v4
> VF introduces a second rtnl-held 200 ms mailbox waiter on the same path.
> 

The rtnl_lock() in enetc_msg_get_link_speed() is intentional, as discussed
earlier: for a MAC sitting behind a PCS with rate matching, the speed cached
at mac_link_up() is the PCS/interface rate, not the external PHY link speed,
so querying phylink is what gives the VF the correct value. In practice a DPDK
VF does not poll link speed in a tight loop, so the chance of this rtnl acquisition
overlapping another VF's rtnl-held mailbox send is small.

If the concerned situation does occur, the worst case is that one of the two VFs
gets a mailbox timeout error and retries, which is acceptable. So no change is
needed here.