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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 1 · Low: 2

- [High] For the default (untrusted) ENETC v4 VF the PF handlers reject
  exactly the messages the new callback sends, and…
- [Medium] enetc_vf_set_rx_mode() narrows the SI's promiscuous
  configuration before the replacement MAC hash filters are installed,…
- [Low] The commit message describes the change as a plain Rx-mode
  offload implementation for ENETC v4 VFs and never mentions that every…
- [Low] The single enetc_ndev_ops table used by all ENETC VFs now
  installs '.ndo_set_rx_mode_async = enetc_vf_set_rx_mode'…

--

net: enetc: implement ndo_set_rx_mode_async for ENETC v4 VF

This adds ndo_set_rx_mode_async to the ENETC VF driver so MAC filter
updates can be sent to the PF through the sleepable VSI mailbox path, and
sets IFF_UNICAST_FLT for ENETC v4 VFs. Two helpers are added, one for the
MAC promiscuous mode message and one for the 64-bit hash filter message.

A few questions about the interaction with the PF-side trust checks and
about the ordering of the two mailbox messages.

First, on the commit message itself:

> The ndo_set_rx_mode_async callback selects the appropriate filter
> configuration based on the current netdev flags:
>
>  - IFF_PROMISC: enable full promiscuous mode for both unicast and
>    multicast
>  - IFF_ALLMULTI: enable multicast promiscuous mode, disable unicast
>    promiscuous mode, and apply a unicast hash filter
>  - otherwise: disable all promiscuous modes and apply both
>               unicast and multicast hash filters
>
> Set IFF_UNICAST_FLT in priv_flags for ENETC v4 VFs so the network stack
> does not fall back to full promiscuous mode unnecessarily when unicast
> address filtering is supported by the hardware.
>
> This feature applies to ENETC v4 hardware only. ENETC v1 (LS1028A) does
> not support VF-PF MAC filter messaging and the callback returns early
> for such devices.

[Severity: Low]
Could the commit message mention that all of these messages are subject to
the PF-side ENETC_VF_FLAG_TRUSTED check in enetc_msg.c?

For an untrusted VF (the default), enetc_msg_set_vf_mac_promisc_mode()
denies any promisc-enable and enetc_msg_set_vf_mac_hash_filter() denies
unicast hash programming, so the described IFF_PROMISC and IFF_ALLMULTI
behaviour does not take effect, and the justification for IFF_UNICAST_FLT
("unicast address filtering is supported by the hardware") only holds for
trusted VFs.

The message also documents only the ENETC v1 early return, not the new
policy at the out: label that maps -EOPNOTSUPP, -EACCES and -EPERM to 0.

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_vf.c b/drivers/net/ethernet/freescale/enetc/enetc_vf.c
> index 322705202d49..4e717afba7f7 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_vf.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_vf.c
> @@ -213,6 +213,142 @@ static int enetc_vf_setup_tc(struct net_device *ndev, enum tc_setup_type type,
>  	}
>  }
>  
> +static int enetc_vf_set_mac_promisc(struct enetc_si *si, int type, bool en)
> +{
> +	struct enetc_msg_mac_promisc_mode *msg;
> +	struct device *dev = &si->pdev->dev;
> +	struct enetc_msg_swbd msg_swbd;
> +
> +	if (!(type & ENETC_MAC_FILTER_TYPE_ALL))
> +		return -EINVAL;
> +
> +	msg_swbd.size = ALIGN(sizeof(*msg), ENETC_MSG_ALIGN);
> +	msg_swbd.vaddr = dma_alloc_coherent(dev, msg_swbd.size,
> +					    &msg_swbd.dma, GFP_KERNEL);
> +	if (!msg_swbd.vaddr)
> +		return -ENOMEM;
> +
> +	msg = (struct enetc_msg_mac_promisc_mode *)msg_swbd.vaddr;
> +	msg->config = FIELD_PREP(ENETC_MSG_MAC_TYPE,
> +				 type & ENETC_MAC_FILTER_TYPE_ALL);
> +	msg->config |= FIELD_PREP(ENETC_MSG_MAC_PROMISC_MODE, en);
> +	msg->config |= FIELD_PREP(ENETC_MSG_MAC_FLUSH_MACS, en);
> +	enetc_msg_fill_common_hdr(&msg_swbd, ENETC_MSG_CLASS_ID_MAC_FILTER,
> +				  ENETC_MSG_SET_MAC_PROMISC_MODE, 0, 0);
> +
> +	return enetc_msg_vsi_send(si, &msg_swbd);
> +}
> +
> +static int enetc_vf_set_mac_hash_filter(struct enetc_si *si,
> +					struct netdev_hw_addr_list *uc,
> +					struct netdev_hw_addr_list *mc)
> +{
> +	struct enetc_msg_mac_hash_filter *msg;
> +	struct enetc_mac_filter *mac_filter;
> +	struct device *dev = &si->pdev->dev;
> +	struct net_device *ndev = si->ndev;
> +	struct enetc_msg_swbd msg_swbd;
> +	struct netdev_hw_addr *ha;
> +	u32 msg_size, tbl_cnt;
> +	int mac_filter_type;
> +	int i = 0;
> +
> +	if (ndev->flags & IFF_PROMISC)
> +		return 0;
> +
> +	if (ndev->flags & IFF_ALLMULTI) {
> +		tbl_cnt = 2;
> +		mac_filter_type = ENETC_MAC_FILTER_TYPE_UC;
> +	} else {
> +		tbl_cnt = 4;
> +		mac_filter_type = ENETC_MAC_FILTER_TYPE_ALL;
> +	}
> +

[ ... ]

> +	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_promisc_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.

> +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_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;
	}
	...
}

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

>  /* Probing/ Init */
>  static const struct net_device_ops enetc_ndev_ops = {
>  	.ndo_open		= enetc_open,
> @@ -225,6 +361,7 @@ static const struct net_device_ops enetc_ndev_ops = {
>  	.ndo_setup_tc		= enetc_vf_setup_tc,
>  	.ndo_hwtstamp_get	= enetc_hwtstamp_get,
>  	.ndo_hwtstamp_set	= enetc_hwtstamp_set,
> +	.ndo_set_rx_mode_async	= enetc_vf_set_rx_mode,
>  };

[Severity: Low]
This ops table is shared by ENETC v1 and v4 VFs, but the callback returns 0
immediately for v1 without programming anything. Core code uses callback
presence as the rx-filtering capability test:

net/core/dev_ioctl.c:dev_ifsioc() {
	...
	case SIOCADDMULTI:
		if ((!ops->ndo_set_rx_mode && !ops->ndo_set_rx_mode_async) ||
		    ifr->ifr_hwaddr.sa_family != AF_UNSPEC)
			return -EINVAL;
	...
}

Does SIOCADDMULTI/SIOCDELMULTI on an ENETC v1 VF now return success instead
of -EINVAL while no hardware filter is touched, and __dev_set_rx_mode()
queue rx-mode work that can never do anything? Could the revision test be
applied where the ops are selected, as the probe path already does for
si->ops, so the capability is only advertised where it is implemented?

>  
>  static void enetc_vf_get_revision(struct enetc_si *si)
> @@ -280,6 +417,9 @@ static void enetc_vf_netdev_setup(struct enetc_si *si, struct net_device *ndev,
>  	ndev->vlan_features = NETIF_F_SG | NETIF_F_HW_CSUM |
>  			      NETIF_F_TSO | NETIF_F_TSO6;
>  
> +	if (!is_enetc_rev1(si))
> +		ndev->priv_flags |= IFF_UNICAST_FLT;
> +

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831025441.635045-1-wei.fang%40oss.nxp.com