Re: [PATCH v6 net-next 12/15] net: dsa: netc: add FDB, STP, MTU, port setup and host flooding support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Incorrect FDB port bitmap update causes roaming unicast entries to be flooded to multiple ports.
- [High] Local `u8` arrays `bcast` and `dmac_mask` are passed to `ether_addr_copy` without ensuring 16-bit alignment.
- [Medium] The driver silently fails to delete dynamically learned FDB entries.
--

commit 5ef406cd4ab0e5005f26e470e33265be68b4fe54
Author: Wei Fang <wei.fang@nxp.com>

net: dsa: netc: add FDB, STP, MTU, port setup and host flooding support
    
Expand the NETC switch driver with foundational features including FDB
management, STP, MTU configuration, and host flooding support. The driver
operates in standalone port mode.

diff --git a/drivers/net/dsa/netc/netc_main.c b/drivers/net/dsa/netc/netc_main.c
--- a/drivers/net/dsa/netc/netc_main.c
+++ b/drivers/net/dsa/netc/netc_main.c
[ ... ]
> @@ -363,6 +418,206 @@ static void netc_port_default_config(struct netc_port *np)
[ ... ]
> +static int netc_port_set_fdb_entry(struct netc_port *np,
> +				   const unsigned char *addr, u16 vid)
> +{
[ ... ]
> +	cfge = &entry->cfge;
> +	/* If the entry already exists on the port, return 0 directly */
> +	if (unlikely(cfge->port_bitmap & cpu_to_le32(BIT(port))))
> +		goto unlock_fdbt;
> +
> +	/* If the entry already exists, but not on this port, we need to
> +	 * update the port bitmap. In general, it should only be valid
> +	 * for multicast or broadcast address.
> +	 */
> +	cfge->port_bitmap |= cpu_to_le32(BIT(port));

Does this correctly handle roaming unicast MAC addresses?

When a unicast MAC address roams from one port to another, or when a user
replaces a static unicast FDB entry using bridge fdb replace, the bridge core
requests adding the entry on the new port.

Because this unconditionally ORs the port into the bitmap, the unicast address
becomes associated with both the old and new ports simultaneously. This will
cause the switch hardware to duplicate and flood unicast frames to multiple
ports. 

Should this check is_multicast_ether_addr() and replace the port_bitmap
instead of ORing it for unicast addresses?

> +	err = ntmp_fdbt_update_entry(&priv->ntmp, entry->entry_id, cfge);
[ ... ]
> +static int netc_port_del_fdb_entry(struct netc_port *np,
> +				   const unsigned char *addr, u16 vid)
> +{
> +	struct netc_switch *priv = np->switch_priv;
> +	struct ntmp_user *ntmp = &priv->ntmp;
> +	struct netc_fdb_entry *entry;
> +	struct fdbt_cfge_data *cfge;
> +	int port = np->dp->index;
> +	int err = 0;
> +
> +	mutex_lock(&priv->fdbt_lock);
> +
> +	entry = netc_lookup_fdb_entry(priv, addr, vid);
> +	if (unlikely(!entry))
> +		goto unlock_fdbt;

Can this silently fail to delete dynamically learned FDB entries?

The netc_lookup_fdb_entry() function searches priv->fdb_list, which only
tracks static entries explicitly added by software. 

If the bridge core or user attempts to delete a hardware-learned dynamic MAC
address, the lookup will return NULL. The function will then jump to
unlock_fdbt and silently return 0 (success) without actually issuing a delete
command to the hardware FDB.

> +
> +	cfge = &entry->cfge;
[ ... ]
> +static int netc_add_standalone_fdb_bcast_entry(struct netc_switch *priv)
> +{
> +	const u8 bcast[ETH_ALEN] = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff};

Are local byte arrays guaranteed to be 16-bit aligned?

The ether_addr_equal() macro (used inside netc_lookup_fdb_entry which is
subsequently called by netc_port_set_fdb_entry) expects its pointer arguments
to be 16-bit aligned. 

Because bcast is declared as a simple u8 array on the stack, the compiler
only guarantees a 1-byte alignment by default. On architectures with strict
alignment requirements, this could trigger an unaligned access exception.

> +	struct dsa_port *dp, *cpu_dp = NULL;
> +
> +	dsa_switch_for_each_cpu_port(dp, priv->ds) {
> +		cpu_dp = dp;
> +		break;
> +	}
> +
> +	if (!cpu_dp)
> +		return -ENODEV;
> +
> +	/* If the user port acts as a standalone port, then its PVID is 0,
> +	 * MLO is set to "disable MAC learning" and MFO is set to "discard
> +	 * frames if no matching entry found in FDB table". Therefore, we
> +	 * need to add a broadcast FDB entry on the CPU port so that the
> +	 * broadcast frames received on the user port can be forwarded to
> +	 * the CPU port.
> +	 */
> +	return netc_port_set_fdb_entry(NETC_PORT(priv->ds, cpu_dp->index),
> +				       bcast, NETC_STANDALONE_PVID);
> +}
[ ... ]
> +static int netc_port_add_host_flood_rule(struct netc_port *np,
> +					 bool uc, bool mc)
> +{
> +	const u8 dmac_mask[ETH_ALEN] = {0x1, 0, 0, 0, 0, 0};

This has the same alignment concern as the bcast array above.

The ether_addr_copy() macro expects its pointer arguments to be 16-bit
aligned. Passing a stack-allocated u8 array without explicit alignment could
cause unaligned access faults on certain architectures.

> +	struct netc_switch *priv = np->switch_priv;
> +	struct ipft_entry_data *host_flood;
> +	struct ipft_keye_data *keye;
[ ... ]
> +	if (uc && !mc)
> +		ether_addr_copy(keye->dmac_mask, dmac_mask);
> +
> +	if (!uc && mc) {
> +		ether_addr_copy(keye->dmac, dmac_mask);
> +		ether_addr_copy(keye->dmac_mask, dmac_mask);
> +	}

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