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

> 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 <[email protected]>
> 
> 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?

netc_port_set_fdb_entry() is an interface shared by single port mode and bridge
mode. Currently, since only single port mode is supported, it doesn't have any
impact, as this mode only allows setting FDB entries on CPU ports. However, for
future support of bridge mode, this is indeed a potential issue, and I will improve
this shared interface in the next version.

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

Currently, this patch set only supports single port mode, the MAC learning is
disabled, so there is no dynamically learned FDB entry. This interface will
be improved when the bridge mode support is added. So please ignore this
warning as we do not support bridge mode yet.

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

The switch runs on ARM64 SoCs, CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
is always selected, so there is no need to align bcast to 16-bit. This is a false
positive, please ignore it

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

Likewise.

--
pw-bot: cr