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

quoted

+static int enetc_vf_enable_iff_promisc(struct enetc_si *si) {
+	int err;
+
+	err = enetc_vf_set_mac_promisc(si, ENETC_MAC_FILTER_TYPE_ALL,
+				       true, true);
+	if (err)
+		dev_err(&si->pdev->dev,
+			"Failed to enable promiscuous mode, err: %pe\n",
+			ERR_PTR(err));
+
+	return err;
+}
+
+static int enetc_vf_disable_iff_promisc(struct enetc_si *si,
+					struct netdev_hw_addr_list *uc,
+					struct netdev_hw_addr_list *mc)
+{
+	int err;
+
+	err = enetc_vf_set_mac_hash_filter(si, uc, mc);
+	if (err) {
+		dev_err_once(&si->pdev->dev,
+			     "Failed to set MAC hash filters, err: %pe\n",
+			     ERR_PTR(err));
+		return err;
+	}
+
+	err = enetc_vf_set_mac_promisc(si, ENETC_MAC_FILTER_TYPE_ALL,
+				       false, false);

[Severity: Medium]
Is the early return above correct when the PF only denies the unicast part of the
request?

enetc_vf_disable_iff_promisc() sends the combined hash filter first with type =
UC | MC.  On the PF side, enetc_msg_set_vf_mac_hash_filter() handles
ENETC_MAC_FILTER_TYPE_ALL like this:

drivers/net/ethernet/freescale/enetc/enetc_msg.c:
	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;
	}

so the multicast hash was applied and only unicast was refused, yet
enetc_msg_vsi_send() maps UCF_DENY to -EACCES.  For the default untrusted
VF that means the 'return err' path is always taken and the following

	enetc_vf_set_mac_promisc(si, ENETC_MAC_FILTER_TYPE_ALL, false, false);

message, the one that actually leaves promiscuous mode, is never sent.
Would it be better to treat UCF_DENY as a partial success and continue, or to
send the promisc-off message before the hash filter?

This is a false positive. When a VF changes from trusted to untrusted, its
promiscuous mode is already disabled on the PF side. No need to send
the promisc-off message, its pointless.

The same ordering concern applies to enetc_vf_enable_iff_allmulti() below:
the hardware-changing step (enable MC promisc with flush_macs = true) runs
first and the restricting step (disable UC promisc) runs last, with no rollback if a
later mailbox transaction returns -EIO ("VSI mailbox is busy"), -ETIME ("VSI
mailbox timeout") or -EBUSY from the PSI.  Can that leave a trusted VF with
UC/MC promiscuity armed in the SI while the netdev has already left
promiscuous mode?

This is also a false positive, the ndo_set_rx_mode_async function is called
within the workqueue. When these errors are returned, the work is
rescheduled, so the process will continue to attempt. If PF works without
faults, the setup will eventually complete successfully; if there are faults,
moving the order will not solve the problem.

Since the SI promiscuity bits live in PF register space indexed by si_id and are
re-applied after FLR by enetc4_pf_vf_flr_handler(), a skipped promisc-off
message appears to persist until the VF issues another one or the PF revokes
trust.

quoted

+	if (err)
+		dev_err_once(&si->pdev->dev,
+			     "Failed to disable promiscuous mode, err: %pe\n",
+			     ERR_PTR(err));
+
+	return err;
+}
+
+static int enetc_vf_enable_iff_allmulti(struct enetc_si *si,
+					struct netdev_hw_addr_list *uc)
+{
+	int err;
+
+	err = enetc_vf_set_mac_promisc(si, ENETC_MAC_FILTER_TYPE_MC,
+				       true, true);
+	if (err) {
+		dev_err(&si->pdev->dev,
+			"Failed to enable multicast promiscuous mode, err: %pe\n",
+			ERR_PTR(err));
+		return err;
+	}

[Severity: High]
For an untrusted VF, does this leave the IFF_ALLMULTI and IFF_PROMISC cases
with no MAC filter programmed at all?

Yes, for untrusted VFs, attempting to enable IFF_ALLMULTI or IFF_PROMISC will
return directly without configuring any MAC filter. Configuring a restricted
multicast filter is only permitted in the third case (IFF_ALLMULTI and IFF_PROMISC
flags are not set).

Both enetc_vf_enable_iff_allmulti() and enetc_vf_enable_iff_promisc() start by
requesting promiscuous mode, and the PF rejects that unconditionally before
touching any register:

drivers/net/ethernet/freescale/enetc/enetc_msg.c:
enetc_msg_set_vf_mac_promisc_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;
	}

That becomes -EACCES and both helpers return immediately, so the unicast hash
filter, the unicast-promisc-off message and any multicast hash filter are never
sent.

Note also that the multicast snapshot is not passed into either of these two
branches, and enetc_vf_set_mac_hash_filter(si, NULL, mc) has no caller in the
patch.  The PF does permit a bounded multicast-only hash filter for an
untrusted VF:

drivers/net/ethernet/freescale/enetc/enetc_msg.c:
enetc_msg_set_vf_mac_hash_filter() {
	case ENETC_MAC_FILTER_TYPE_MC:
		mc_hash = (u64)msg->hash_tbl[1] << 32 | msg->hash_tbl[0];
		if (!trusted &&
		    hweight64(mc_hash) > ENETC_VF_MC_HASH_BITS_MAX) {
			pf_msg = ENETC_PF_MSG_PERM_DENY;
			goto vf_state_unlock;
		}

		enetc_set_si_mc_hash_filter(si, si_id, mc_hash);

so multicast groups joined while the interface is in allmulti or promisc mode
appear to be silently dropped, with the SI keeping whatever hash filter the last
non-allmulti update left behind.  IFF_ALLMULTI and IFF_PROMISC are entered
by ordinary operations (bridge/bond/macvlan enslavement, 'ip link set ...
allmulticast on', tcpdump), including from a guest owning the VF.

This also seems to contradict the commit message:

	For an untrusted VF (the default) the PF denies promiscuous mode and
	unicast hash filtering, so only the multicast hash filter is
	applied, limited to ENETC_VF_MC_HASH_BITS_MAX buckets.

That only holds for the third (non-promisc, non-allmulti) branch, not for these
two.

The original intention of the commit message is to indicate that multicast
filtering with configuration restrictions is only allowed in the third case.

One more thing: unlike enetc_vf_disable_iff_promisc(), which uses
dev_err_once(), these two helpers use plain dev_err().  Since the denial is
guaranteed for an untrusted VF, wouldn't that print an error line on every
rx_mode update, i.e. on every multicast join/leave and every bridge or bond
membership change?

This is intentional. Compared to other interfaces, ndo_set_rx_mode_async()
is called relatively frequently. If the VF is not trusted, printing an error log
every time it is called would annoy users, so printing it only once is sufficient.

quoted

+
+	err = enetc_vf_set_mac_hash_filter(si, uc, NULL);
+	if (err) {
+		dev_err(&si->pdev->dev,
+			"Failed to set unicast filter, err: %pe\n",
+		ERR_PTR(err));
+		return err;
+	}
+
+	err = enetc_vf_set_mac_promisc(si, ENETC_MAC_FILTER_TYPE_UC,
+				       false, false);
+	if (err)
+		dev_err(&si->pdev->dev,
+			"Failed to disable unicast promiscuous mode, err: %pe\n",
+		ERR_PTR(err));
+
+	return err;
+}
+
+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;
+
+	if (ndev->flags & IFF_PROMISC)
+		err = enetc_vf_enable_iff_promisc(si);
+	else if (ndev->flags & IFF_ALLMULTI)
+		err = enetc_vf_enable_iff_allmulti(si, uc);
+	else
+		err = enetc_vf_disable_iff_promisc(si, uc, mc);
+
+	/* 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: Medium]
Is the premise behind this mapping accurate?  The comment, and the commit
message wording

	Since a denied request always fails, map -EOPNOTSUPP, -EACCES and
	-EPERM to 0 so the core does not retry an operation that can never
	succeed.

assume the denial is permanent, but PF trust is runtime-mutable:

"The denial is permanent" is not true, enetc_vf_set_rx_mode() will
not return -EACCES or -EPERM when VF is changed to trusted.

drivers/net/ethernet/freescale/enetc/enetc_pf_common.c:
enetc_pf_set_vf_trust() {
	if (setting) {
		vf_state->flags |= ENETC_VF_FLAG_TRUSTED;
	} else {

That path only flips the flag; it neither notifies the VF nor replays the previously
rejected configuration.  So does

	ip link set <pf> vf N trust on

as suggested in the commit message actually take effect before some unrelated
flag or address-list change happens to re-trigger the callback?

Also, ndo_set_rx_mode_async is documented in include/linux/netdevice.h as
returning 0 on success and a negative errno to request a core retry, and
netif_rx_mode_run() in net/core/dev_addr_lists.c treats 0 as installed:

	if (err)
		netif_rx_mode_schedule_retry(dev);
	else
		dev->rx_mode_retry_count = 0;

Reporting 0 for a rejected or partially applied configuration means the core and
userspace believe the filter is in hardware with no diagnostic emitted.
The -EOPNOTSUPP case looks similar: a PF or firmware that does not implement
the MAC filter message class returns it, and the VF would then report success
with nothing programmed.

Mapping -EOPNOTSUPP, -EACCES and -EPERM to 0 is an intentional design
trade-off, and I'd keep it as is.

The core's retry is exponential backoff meant for transient failures that may heal
on retry. A trust-based or capability denial is not transient in that sense - until an
admin changes trust, every retry is guaranteed to fail, so it just makes the core
spin for ~15s and log "retry limit reached" on every address-list change. For a
default-untrusted VF this is recurring dmesg noise with no benefit, since the core
can only retry and retrying cannot succeed.