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

quoted

+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)
+{

[ ... ]

quoted

+	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?

ENETC only used on arm64 little-endian architectures, but this logic can
be improved.

quoted

+
+	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,

quoted

+				  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?

I was not aware of the ndo_set_rx_mode_async() interface had been changed
recently, I will fix this issue in next version.

[ ... ]

quoted

+	if (ndev->flags & IFF_PROMISC) {
+		enetc_vf_set_mac_promisc(priv, ENETC_MAC_FILTER_TYPE_ALL,

true);

quoted

+	} else if (ndev->flags & IFF_ALLMULTI) {
+		enetc_vf_set_mac_promisc(priv, ENETC_MAC_FILTER_TYPE_MC,

true);

quoted

+		enetc_vf_set_mac_promisc(priv, ENETC_MAC_FILTER_TYPE_UC,

false);

quoted

+	} else {
+		enetc_vf_set_mac_promisc(priv, ENETC_MAC_FILTER_TYPE_ALL,

false);

quoted

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

[ ... ]

quoted

@@ -225,6 +341,7 @@ static const struct net_device_ops enetc_ndev_ops =

{

quoted

 	.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,
 };

--
pw-bot: cr