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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Function signature mismatch for ndo_set_rx_mode_async causing CFI panics and silently dropping errors
- [Medium] Endianness logic bug in MAC hash filter conversion
--

commit 7154dd069b56705c2fb84b8fb83bdfde897299bf
Author: Wei Fang <wei.fang@nxp.com>

net: enetc: implement ndo_set_rx_mode_async for ENETC v4 VF

This commit implements ndo_set_rx_mode_async for the ENETC VF driver, allowing
MAC filter changes to be communicated to the PF over a VSI mailbox interface.
It manages promiscuous modes and Bloom filter hash tables for unicast and
multicast traffic.

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_vf.c b/drivers/net/ethernet/freescale/enetc/enetc_vf.c
> index 4bc48a5f6869c..dc9a0f74a2e34 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_vf.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_vf.c
[ ... ]
> +static int enetc_vf_set_mac_hash_filter(struct enetc_ndev_priv *priv,
> +					struct netdev_hw_addr_list *uc,
> +					struct netdev_hw_addr_list *mc)
> +{
[ ... ]
> +	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);
> +
> +		memcpy(&hash_val, mac_filter->mac_hash_table,
> +		       sizeof(hash_val));
> +		msg->hash_tbl[i++] = hash_val & GENMASK(31, 0);
> +		msg->hash_tbl[i++] = hash_val >> 32;
> +	}

[Severity: Medium]
Will this memcpy() correctly handle bitmaps on 32-bit big-endian
architectures?

A DECLARE_BITMAP is an array of 32-bit words, where word 0 (bits 0-31) is
stored at lower addresses and word 1 (bits 32-63) is at higher addresses.

When memcpy() copies these 8 bytes into a u64 variable on a big-endian
system, word 0 becomes the most significant 32 bits and word 1 becomes the
least significant 32 bits. The subsequent bitwise operations
(hash_val & GENMASK(31, 0) and hash_val >> 32) then extract the halves in
reverse order compared to 64-bit or little-endian systems, which silently
swaps the hardware filter configuration.

Should this use bitmap_to_arr32() instead to safely convert the bitmap to
a u32 array across all architectures?

> +
> +	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);
> +
> +		memcpy(&hash_val, mac_filter->mac_hash_table,
> +		       sizeof(hash_val));
> +		msg->hash_tbl[i++] = hash_val & GENMASK(31, 0);
> +		msg->hash_tbl[i++] = hash_val >> 32;
> +	}
> +
> +	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 void enetc_vf_set_rx_mode(struct net_device *ndev,
> +				 struct netdev_hw_addr_list *uc,
> +				 struct netdev_hw_addr_list *mc)
> +{

[Severity: High]
Does this function signature cause a Control Flow Integrity (CFI) panic?

The ndo_set_rx_mode_async callback expects a function returning an int,
but enetc_vf_set_rx_mode() is defined with a void return type. This
mismatch will trigger a CFI kernel panic on architectures with CFI enabled.

Additionally, because it returns void, the caller __dev_set_rx_mode_async()
will read uninitialized garbage from the return register. If that garbage is
non-zero, could it cause continuous polling retries in the network stack?

[ ... ]
> +	if (ndev->flags & IFF_PROMISC) {
> +		enetc_vf_set_mac_promisc(priv, ENETC_MAC_FILTER_TYPE_ALL, true);
> +	} else if (ndev->flags & IFF_ALLMULTI) {
> +		enetc_vf_set_mac_promisc(priv, ENETC_MAC_FILTER_TYPE_MC, true);
> +		enetc_vf_set_mac_promisc(priv, ENETC_MAC_FILTER_TYPE_UC, false);
> +	} else {
> +		enetc_vf_set_mac_promisc(priv, ENETC_MAC_FILTER_TYPE_ALL, false);
> +	}
> +
> +	enetc_vf_set_mac_hash_filter(priv, uc, mc);
> +}

[Severity: High]
Are the error codes from these mailbox commands inadvertently being dropped?

Because this function is defined as void and doesn't return the results of
enetc_vf_set_mac_promisc() or enetc_vf_set_mac_hash_filter(), any hardware
configuration errors are discarded instead of propagated to the network
stack, which breaks the framework's retry mechanism.

[ ... ]
> @@ -225,6 +341,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,
>  };

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260610091844.3423693-1-wei.fang@oss.nxp.com?part=13