Re: [PATCH v3 net-next 09/15] net: enetc: add MAC address filtering support for VFs of ENETC v4

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] Commit message mischaracterizes the privilege model of
  ENETC_MSG_SET_MAC_HASH_TABLE.
- [Medium] The patch lets a VF arm PF-owned hardware filtering state
  (PSIPMMR unicast/multicast promiscuous bits and the per-SI…
- [Medium] pf->msg_lock is now used for two incompatible purposes: (a) as
  the mailbox transaction lock, held across a sleeping hardware poll -…
- [Low] Concern that the patch adds the PF-side mailbox protocol
  (ENETC_MSG_SET_MAC_HASH_TABLE, ENETC_MSG_SET_MAC_PROMISC_MODE) without…

--

net: enetc: add MAC address filtering support for VFs of ENETC v4

This adds two VSI-to-PSI mailbox commands so an ENETC v4 VF can ask the PF
to program its per-SI MAC hash filter tables and to change its unicast and
multicast promiscuous mode bits in PSIPMMR.  The PF-owned PSIPMMR
read-modify-write is now serialized with pf->msg_lock, and losing trusted
status clears part of the VF's filtering state.

A few questions below.

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc4_pf.c b/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
> index 363ec562934e..a4ffe1100bd7 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc4_pf.c

[ ... ]

> @@ -528,8 +523,10 @@ static int enetc4_pf_set_rx_mode(struct net_device *ndev,
>  		type = ENETC_MAC_FILTER_TYPE_ALL;
>  	}
>  
> +	enetc_pf_msg_lock(pf);
>  	enetc_set_si_uc_promisc(si, 0, uc_promisc);
>  	enetc_set_si_mc_promisc(si, 0, mc_promisc);
> +	enetc_pf_msg_unlock(pf);

[Severity: Medium]

Is pf->msg_lock the right lock to reuse for the PSIPMMR read-modify-write
here?  It looks like msg_lock is also the mailbox transaction lock, and it
is held across a sleeping hardware poll:

drivers/net/ethernet/freescale/enetc/enetc_msg.c:
enetc_msg_register_link_status_notifier() {
	mutex_lock(&pf->msg_lock);
	...
	enetc_msg_notify_vf_link_status(pf, PSIMSGSR_MS(vf_id));
msg_unlock:
	mutex_unlock(&pf->msg_lock);
}

which ends in:

enetc_pf_send_msg() {
	...
	return read_poll_timeout(enetc_rd, val, !(val & ms_mask), 1000,
				 200000, false, hw, ENETC_PSIMSGSR);
}

enetc_pf_notify_vf_link_status() takes the same mutex around the same poll.

enetc4_pf_set_rx_mode() is the ndo_set_rx_mode_async callback, which runs
with rtnl held (netif_rx_mode_run() asserts it for a driver that sets none
of request_ops_lock/queue_mgmt_ops/net_shaper_ops, and netdev_work_proc()
takes rtnl_lock() unconditionally).  Can a VF that never drains its mailbox
reply then keep the PF's address-list/promiscuity update, and the global
rtnl mutex, waiting up to 200 ms per poll?  The same helper is now taken by
the mac_filter debugfs reader in enetc_mac_filter_show().

Would a small dedicated lock for the PSIPMMR read-modify-write, separate
from the mailbox transaction mutex, avoid coupling a couple of register
writes to a guest-controlled sleeping poll?

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_msg.c b/drivers/net/ethernet/freescale/enetc/enetc_msg.c
> index c3ae4c024f34..d58fbaeaf46c 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_msg.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_msg.c

[ ... ]

> @@ -108,6 +110,132 @@ static u16 enetc_msg_set_vf_primary_mac_addr(struct enetc_pf *pf, int vf_id,
>  	return pf_msg;
>  }
>  
> +static u16 enetc_msg_set_vf_mac_hash_filter(struct enetc_pf *pf, int vf_id,
> +					    void *vf_msg)
> +{
> +	struct enetc_vf_state *vf_state = &pf->vf_state[vf_id];
> +	struct enetc_msg_mac_hash_filter *msg = vf_msg;
> +	u16 pf_msg = ENETC_PF_MSG_SUCCESS;
> +	struct enetc_si *si = pf->si;
> +	int si_id = vf_id + 1;
> +	u64 uc_hash, mc_hash;
> +	bool trusted;
> +	int type;
> +
> +	/* Currently, hardware only supports 64 bits table size */
> +	if (FIELD_GET(ENETC_MSG_MAC_HASH_SIZE, msg->sz_type) !=
> +	    ENETC_MAC_HASH_TABLE_SIZE_64)
> +		return ENETC_PF_MSG_NOTSUPP;
> +
> +	mutex_lock(&vf_state->lock);
> +
> +	/* For an untrusted VF, unicast MAC hash filtering is not permitted.
> +	 * For multicast, the MAC hash filter is strictly limited to a maximum
> +	 * of 8 bits to satisfy its basic multicast communication requirements
> +	 * while preventing potential network abuse.
> +	 */
> +	trusted = !!(vf_state->flags & ENETC_VF_FLAG_TRUSTED);
> +	type = FIELD_GET(ENETC_MSG_MAC_TYPE, msg->sz_type);
> +	switch (type) {
> +	case ENETC_MAC_FILTER_TYPE_UC:
> +		if (!trusted) {
> +			pf_msg = ENETC_PF_MSG_PERM_DENY;
> +			goto vf_state_unlock;
> +		}
> +
> +		uc_hash = (u64)msg->hash_tbl[1] << 32 | msg->hash_tbl[0];
> +		enetc_set_si_uc_hash_filter(si, si_id, uc_hash);
> +		break;
> +	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);
> +		break;

[Severity: Medium]

Does the changelog match the policy implemented here?  It says:

  "1. ENETC_MSG_SET_MAC_HASH_TABLE (cmd_id 3): allows a trusted VF to
  program its unicast and/or multicast MAC hash filter table. The PF
  validates that the hardware-supported 64-bit table size is requested
  before applying the configuration via the per-SI hash filter registers."

The ENETC_MAC_FILTER_TYPE_MC case does not test ENETC_VF_FLAG_TRUSTED at
all.  An untrusted VF gets its multicast hash filter programmed as long as
no more than ENETC_VF_MC_HASH_BITS_MAX of the 64 buckets are set.

The TYPE_ALL case below does the same: for an untrusted VF it applies the
multicast half of the request and then reports UCF_DENY.

Neither the new ENETC_VF_MC_HASH_BITS_MAX constant nor the "up to 8 buckets
for untrusted VFs" rule appears in the changelog.  Could the commit message
describe this rule, and where the value 8 comes from?

> +	case ENETC_MAC_FILTER_TYPE_ALL:
> +		if (!msg->hdr.len) {
> +			pf_msg = ENETC_PF_MSG_INV_LEN;
> +			goto vf_state_unlock;
> +		}
> +
> +		uc_hash = (u64)msg->hash_tbl[1] << 32 | msg->hash_tbl[0];
> +		mc_hash = (u64)msg->hash_tbl[3] << 32 | msg->hash_tbl[2];
> +
> +		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;
> +		}

[ ... ]

> +static u16 enetc_msg_set_vf_mac_promisc_mode(struct enetc_pf *pf, int vf_id,
> +					     void *vf_msg)
> +{

[ ... ]

> +	mutex_lock(&pf->msg_lock);
> +
> +	if (type & ENETC_MAC_FILTER_TYPE_UC)
> +		enetc_set_si_uc_promisc(si, si_id, promisc);
> +
> +	if (type & ENETC_MAC_FILTER_TYPE_MC)
> +		enetc_set_si_mc_promisc(si, si_id, promisc);
> +
> +	mutex_unlock(&pf->msg_lock);

[Severity: Medium]

This arms PSIPMMR_SI_MAC_UP/MP for the VF's SI, and the hash filter paths
above write PSIUMHFR/PSIMMHFR for that SI.  Those registers live in the PF
port register space, so a VF-level reset does not clear them.

What clears them when a single VF goes away while SR-IOV stays enabled, for
example on VF driver unbind or guest reboot?  The only disarm paths added
here are enetc_pf_set_vf_trust(ndev, vf, false) and
enetc_msg_clear_vf_config(), and the latter only runs from
enetc_msg_psi_free(), i.e. full SR-IOV teardown.

If neither runs, does the SI stay in promiscuous mode with the previously
programmed hash buckets, so that the next driver bound to that VF inherits
filtering scope it never asked for?  An untrusted VF can also leave up to
ENETC_VF_MC_HASH_BITS_MAX multicast buckets armed this way.

The later patch "net: enetc: restore VF MAC promiscuous mode after FLR for
ENETC v4" adds enetc4_pf_vf_flr_handler(), but that re-applies the tracked
ENETC_VF_FLAG_UC_PROMISC/MC_PROMISC state instead of clearing it and does
not touch the hash filter tables, so the gap seems to remain at the end of
the series.

[ ... ]

> @@ -582,6 +720,31 @@ static int enetc_msg_psi_init(struct enetc_pf *pf)
>  	return err;
>  }
>  
> +static void enetc_msg_clear_vf_config(struct enetc_pf *pf, int vf_id)
> +{
> +	struct enetc_vf_state *vf_state = &pf->vf_state[vf_id];
> +	struct enetc_si *si = pf->si;
> +	int si_id = vf_id + 1;
> +
> +	/* For ENETC v1, we only support setting the VF's MAC address via
> +	 * VSI-to-PSI messages, so there is no configuration to clear.
> +	 */
> +	if (is_enetc_rev1(si))
> +		return;
> +
> +	mutex_lock(&vf_state->lock);
> +
> +	mutex_lock(&pf->msg_lock);
> +	enetc_set_si_uc_promisc(si, si_id, false);
> +	enetc_set_si_mc_promisc(si, si_id, false);
> +	mutex_unlock(&pf->msg_lock);
> +
> +	enetc_set_si_uc_hash_filter(si, si_id, 0);
> +	enetc_set_si_mc_hash_filter(si, si_id, 0);
> +
> +	mutex_unlock(&vf_state->lock);
> +}
> +

[ ... ]

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.h b/drivers/net/ethernet/freescale/enetc/enetc_pf.h
> index 06dc47164dc5..12e67f611f77 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_pf.h
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.h
> @@ -6,6 +6,7 @@
>  
>  #define ENETC_PF_NUM_RINGS	8
>  #define ENETC_VLAN_HT_SIZE	64
> +#define ENETC_VF_MC_HASH_BITS_MAX	8 /* For untrusted VFs */

[ ... ]

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c b/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c
> index 7a11370d2b8e..8007dce90195 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c

[ ... ]

> @@ -600,11 +620,39 @@ int enetc_pf_set_vf_trust(struct net_device *ndev, int vf, bool setting)
> +		/* Disable unicast and multicast promiscuous modes */
> +		mutex_lock(&pf->msg_lock);
> +		enetc_set_si_uc_promisc(si, si_id, false);
> +		enetc_set_si_mc_promisc(si, si_id, false);
> +		mutex_unlock(&pf->msg_lock);
> +
> +		/* Clear unicast hash filter */
> +		enetc_set_si_uc_hash_filter(si, si_id, 0);
> +
> +		/* Clear multicast hash filter if its set bits exceed
> +		 * ENETC_VF_MC_HASH_BITS_MAX.
> +		 */
> +		hash = enetc_get_si_mc_hash_filter(si, si_id);
> +		if (hweight64(hash) > ENETC_VF_MC_HASH_BITS_MAX)
> +			enetc_set_si_mc_hash_filter(si, si_id, 0);
> +	}

[ ... ]

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf_common.h b/drivers/net/ethernet/freescale/enetc/enetc_pf_common.h
> index 1d35e906fc3a..91a9c339245a 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_pf_common.h
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf_common.h
> @@ -30,6 +30,18 @@ static inline u16 enetc_get_ip_revision(struct enetc_hw *hw)
>  	return enetc_global_rd(hw, ENETC_G_EIPBRR0) & EIPBRR0_REVISION;
>  }
>  
> +static inline void enetc_pf_msg_lock(struct enetc_pf *pf)
> +{
> +	if (pf->total_vfs)
> +		mutex_lock(&pf->msg_lock);
> +}
> +
> +static inline void enetc_pf_msg_unlock(struct enetc_pf *pf)
> +{
> +	if (pf->total_vfs)
> +		mutex_unlock(&pf->msg_lock);
> +}
> +

[ ... ]

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