RE: [PATCH v3 net-next 13/15] net: enetc: implement ndo_set_rx_mode_async for ENETC v4 VF

[Wei Fang]

> > +	if (mac_filter_type & ENETC_MAC_FILTER_TYPE_UC) {
> > +		mac_filter = &si->mac_filter[UC];
> > +		enetc_reset_mac_addr_filter(mac_filter);
> > +		netdev_hw_addr_list_for_each(ha, uc)
> > +			enetc_add_mac_addr_ht_filter(mac_filter, ha->addr);
> > +
> > +		bitmap_to_arr32(&msg->hash_tbl[i], mac_filter->mac_hash_table,
> > +				ENETC_MADDR_HASH_TBL_SZ);
> > +		i += 2;
> > +	}
> > +
> > +	if (mac_filter_type & ENETC_MAC_FILTER_TYPE_MC) {
> > +		mac_filter = &si->mac_filter[MC];
> > +		enetc_reset_mac_addr_filter(mac_filter);
> > +		netdev_hw_addr_list_for_each(ha, mc)
> > +			enetc_add_mac_addr_ht_filter(mac_filter, ha->addr);
> > +
> > +		bitmap_to_arr32(&msg->hash_tbl[i], mac_filter->mac_hash_table,
> > +				ENETC_MADDR_HASH_TBL_SZ);
> > +	}
> > +
> > +	enetc_msg_fill_common_hdr(&msg_swbd,
> ENETC_MSG_CLASS_ID_MAC_FILTER,
> > +				  ENETC_MSG_SET_MAC_HASH_TABLE, 0, 0);
> > +
> > +	return enetc_msg_vsi_send(si, &msg_swbd);
> > +}
> > +
> > +static int enetc_vf_set_rx_mode(struct net_device *ndev,
> > +				struct netdev_hw_addr_list *uc,
> > +				struct netdev_hw_addr_list *mc)
> > +{
> > +	struct enetc_ndev_priv *priv = netdev_priv(ndev);
> > +	struct enetc_si *si = priv->si;
> > +	int err;
> > +
> > +	/* For ENETC v1, we cannot return -EOPNOTSUPP or any other error,
> > +	 * otherwise ndev->rx_mode_retry_timer will try to set rx_mode
> > +	 * multiple times, which is pointless.
> > +	 */
> > +	if (is_enetc_rev1(si))
> > +		return 0;
> > +
> > +	if (ndev->flags & IFF_PROMISC) {
> > +		err = enetc_vf_set_mac_promisc(si, ENETC_MAC_FILTER_TYPE_ALL,
> > +					       true);
> > +	} else if (ndev->flags & IFF_ALLMULTI) {
> > +		err = enetc_vf_set_mac_promisc(si, ENETC_MAC_FILTER_TYPE_UC,
> > +					       false);
> > +		if (err)
> > +			goto out;
> > +
> > +		err = enetc_vf_set_mac_promisc(si, ENETC_MAC_FILTER_TYPE_MC,
> > +					       true);
> > +	} else {
> > +		err = enetc_vf_set_mac_promisc(si, ENETC_MAC_FILTER_TYPE_ALL,
> > +					       false);
> > +	}
> > +
> > +	if (err)
> > +		goto out;
> > +
> > +	err = enetc_vf_set_mac_hash_filter(si, uc, mc);
> > +
> 
> [Severity: Medium]
> Is the order of these two mailbox transactions the right way round?
> 
> enetc_vf_set_mac_promisc() sends FLUSH_MACS together with en, and the PF
> handler zeroes both hash filter registers when FLUSH_MACS is set:
> 
> drivers/net/ethernet/freescale/enetc/enetc_msg.c:enetc_msg_set_vf_mac_pr
> omisc_mode() {
> 	...
> 	if ((type & ENETC_MAC_FILTER_TYPE_UC) && flush_macs)
> 		enetc_set_si_uc_hash_filter(si, si_id, 0);
> 
> 	if ((type & ENETC_MAC_FILTER_TYPE_MC) && flush_macs)
> 		enetc_set_si_mc_hash_filter(si, si_id, 0);
> 	...
> }
> 
> So while promiscuous mode is active the SI hash filters are known to be
> empty (enetc_vf_set_mac_hash_filter() also returns early for IFF_PROMISC).
> On the way out of promiscuous mode this function completes the
> promisc-disable round trip first and only then installs the replacement
> hash filters.
> 
> Each enetc_msg_vsi_send() is a blocking round trip polled with
> read_poll_timeout(..., 1000, 200000, ...). Between the two sends the SI has
> promiscuity cleared and empty hash filters - can that drop all multicast
> and secondary unicast frames for the duration, two round trips in the
> IFF_ALLMULTI branch?
> 
> And if the hash filter send then fails (-EIO "VSI mailbox is busy", -ETIME
> on mailbox timeout, -EBUSY for a busy PSI), the promisc-off write is
> already committed and is not undone; netif_rx_mode_schedule_retry() gives
> up after NETIF_RX_MODE_RETRY_MAX attempts:
> 
> net/core/dev_addr_lists.c:netif_rx_mode_schedule_retry() {
> 	if (dev->rx_mode_retry_count >= NETIF_RX_MODE_RETRY_MAX) {
> 		netdev_err(dev, "rx_mode retry limit reached, giving up\n");
> 		return;
> 	}
> 	...
> }
> 
> Would programming the new (superset) filters before narrowing the
> promiscuous bits avoid both the window and the stuck state? The PF side
> already does something similar in enetc4_pf_set_uc_exact_filter() with its
> temporary unicast hash filter.
> 

I don't think this needs to be changed.

First, the window is acceptable. It lasts one mailbox round trip. rx_mode
changes are infrequent management events, so a brief loss during a
reconfiguration is tolerable.

Second, the retry mechanism exists precisely to cover a failed configuration.
The only errors the hash filter send can return here are -EIO (mailbox busy),
-ETIME (mailbox timeout) and -EBUSY (PSI busy) - all of which mean the PF
is not servicing mailbox messages. The core retries with backoff over roughly
15 seconds before giving up. If the PF cannot process a single message in that
time, the PF side is already broken, and reordering the two sends would not
rescue the configuration either - it would just fail at a different point.

> > +out:
> > +	/* If the error code is -EOPNOTSUPP or -EACCES or -EPERM, return 0
> > +	 * directly to avoid meaningless retries.
> > +	 */
> > +	if (err == -EOPNOTSUPP || err == -EACCES || err == -EPERM)
> > +		return 0;
> > +
> > +	return err;
> > +}
> > +
> 
> [Severity: High]
> For an untrusted VF - the default, since pf->vf_state is allocated with
> devm_kcalloc() and ENETC_VF_FLAG_TRUSTED is only set by
> enetc_pf_set_vf_trust() - the PF handlers reject exactly the messages this
> callback sends.
> 
> drivers/net/ethernet/freescale/enetc/enetc_msg.c:enetc_msg_set_vf_mac_pr
> omisc_mode() {
> 	...
> 	promisc = !!(msg->config & ENETC_MSG_MAC_PROMISC_MODE);
> 	if (promisc && !(vf_state->flags & ENETC_VF_FLAG_TRUSTED)) {
> 		pf_msg = ENETC_PF_MSG_PERM_DENY;
> 		goto vf_state_unlock;
> 	}
> 	...
> }
> 
> drivers/net/ethernet/freescale/enetc/enetc_msg.c:enetc_msg_set_vf_mac_ha
> sh_filter() {
> 	...
> 	case ENETC_MAC_FILTER_TYPE_UC:
> 		if (!trusted) {
> 			pf_msg = ENETC_PF_MSG_PERM_DENY;
> 			goto vf_state_unlock;
> 		}
> 	...
> 	case ENETC_MAC_FILTER_TYPE_ALL:
> 		...
> 		if (!trusted &&
> 		    (hweight64(mc_hash) <= ENETC_VF_MC_HASH_BITS_MAX)) {
> 			enetc_set_si_mc_hash_filter(si, si_id, mc_hash);
> 			pf_msg =
> ENETC_PF_MSG_MF(ENETC_MF_CLASS_CODE_UCF_DENY);
> 			goto vf_state_unlock;
> 		}
> 
> 		if (!trusted) {
> 			pf_msg = ENETC_PF_MSG_PERM_DENY;
> 			goto vf_state_unlock;
> 		}
> 	...
> }
> 
> enetc_msg_vsi_send() maps PERMISSION_DENY and UCF_DENY to -EACCES and
> returns before the dev_err(), so there is not even a log line.
> 
> In the IFF_ALLMULTI branch, does the MC promisc-enable message therefore
> always fail on an untrusted VF, making "if (err) goto out;" skip
> enetc_vf_set_mac_hash_filter() completely?
> 
> Even when it is reached, enetc_vf_set_mac_hash_filter() selects
> mac_filter_type = ENETC_MAC_FILTER_TYPE_UC under IFF_ALLMULTI, so the
> mc
> snapshot handed in by the core is never converted into an MC hash table.
> With MC promiscuous mode denied as well, does the multicast hash filter
> stay frozen at its previous contents - so groups joined after the VF is
> enslaved to a bridge or gets a macvlan on top (both call
> dev_set_allmulti()), such as IPv6 solicited-node addresses or mDNS, are
> silently dropped by hardware?
> 
> In the default branch the TYPE_ALL message is refused outright once more
> than ENETC_VF_MC_HASH_BITS_MAX (8) multicast hash bits are set, so
> PSIMMHFR0/1 keeps its stale value. Would it be worth falling back to the
> MC-only message, which the PF does accept from an untrusted VF?
> 
> The out: block then turns -EACCES, -EPERM and -EOPNOTSUPP into 0, and the
> core treats that as "installed":
> 
> net/core/dev_addr_lists.c:netif_rx_mode_run() {
> 	...
> 		if (err)
> 			netif_rx_mode_schedule_retry(dev);
> 		else
> 			dev->rx_mode_retry_count = 0;
> 	...
> }
> 
> Can this leave the SI hash filters permanently out of sync with the address
> list the stack believes is programmed, with no retry and no message? Since
> this patch also sets IFF_UNICAST_FLT, the core's unicast promiscuous
> fallback is suppressed too.
> 
> Two related points: the promisc-disable message carries FLUSH_MACS = en =
> false, so a denied hash filter update can also leave hardware more
> permissive than requested (removed multicast groups keep being received).
> And when trust is granted later, enetc_pf_set_vf_trust() only flips the
> PF-side flag - nothing replays or re-requests the receive mode
> configuration that the VF already reported as successfully installed, so
> does a configuration denied while untrusted stay unapplied indefinitely?

I think the current handling is fine for an untrusted VF.

First, an untrusted VF not getting allmulti is the security policy working as
intended. IFF_ALLMULTI means "receive all multicast", which is exactly
what we do not want to grant to an untrusted VF. If a VF genuinely needs
this (for example to be enslaved to a bridge or to carry a macvlan), the
administrator should mark it trusted with ip link set <pf> vf N trust on.

Second, returning 0 for a denied request is a limitation of the
ndo_set_rx_mode_async() interface. The interface only defines two
outcomes: return 0 for success, or a negative errno to request a retry
via the core backoff. There is no way to express "this was permanently
denied by policy, do not retry". A permission denial is a stable failure,
so returning an error would just make the core retry four times with
backoff and then log "rx_mode retry limit reached, giving up", which
is misleading and pointless. Returning 0 is the better of the two options
the interface offers.