Re: [PATCH 18/20] net: dsa: xilinx: offload the bridge FDB to the switch CAM

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing port forwarding matrix configuration breaks standalone port isolation when hardware bridging is enabled.
- [Medium] port_fdb_dump fails to verify port lists, causing missing or incorrectly attributed FDB entries.
- [Medium] Moving a static unicast FDB entry causes traffic duplication and information leak due to bitwise-ORing the port list.
--

--- Patch [18]: [PATCH 18/20] net: dsa: xilinx: offload the bridge FDB to the switch CAM ---
commit f82d05023d1b6a6153d5fd4fda5ef042b99bb5ce
Author: Nagadheeraj Rottela <[email protected]>

net: dsa: xilinx: offload the bridge FDB to the switch CAM

This commit adds support for offloading the bridge forwarding database (FDB)
to the switch's stream-destination lookup CAM. It enables MAC address learning
globally when ports join a bridge, implements hardware-level FDB management
functions, and serializes indirect CAM register accesses.

> diff --git a/drivers/net/dsa/xilinx/xilinx_tsn.c b/drivers/net/dsa/xilinx/xilinx_tsn.c
> index 9a4d4a2fb8925..57558b0e2613b 100644
> --- a/drivers/net/dsa/xilinx/xilinx_tsn.c
> +++ b/drivers/net/dsa/xilinx/xilinx_tsn.c

[ ... ]

> @@ -539,6 +603,217 @@ static void xlnx_tsn_port_stp_state_set(struct dsa_switch *ds, int port,
> +static int xlnx_tsn_port_fdb_add(struct dsa_switch *ds, int port,
> +				 const unsigned char *addr, u16 vid,
> +				 struct dsa_db db)
> +{
> +	struct xlnx_tsn *sw = ds->priv;
> +	u8 portlist;
> +	int ret;
> +
> +	if (!vid)
> +		vid = TSN_SW_DEFAULT_VID;
> +
> +	guard(mutex)(&sw->indirect_lock);
> +	ret = xlnx_tsn_cam_read_portlist(sw, addr, vid, &portlist);
> +	if (!ret) {
> +		portlist |= TSN_PORT_BIT(port);
> +		ret = xlnx_tsn_cam_write(sw, addr, vid, portlist, true);
> +	}

[Severity: Medium]
Will this bitwise-OR cause traffic duplication when a static unicast FDB entry
is moved?

When an entry moves to a new port (such as during roaming or a bridge fdb
replace), the bridge can send a SWITCHDEV_FDB_ADD_TO_DEVICE event for the new
port before sending a delete for the old port.

By bitwise-ORing the new port into the mask here, the entry will forward
unicast traffic to both the old and new ports simultaneously, instead of
replacing the old port.

> +
> +	return ret;
> +}

[ ... ]

> +static int xlnx_tsn_port_fdb_dump(struct dsa_switch *ds, int port,
> +				  dsa_fdb_dump_cb_t *cb, void *data)
> +{
> +	struct xlnx_tsn *sw = ds->priv;
> +	unsigned char addr[ETH_ALEN];
> +	u32 base, ctrl, key1, key2;
> +	int ret = 0;
> +	u16 vid;
> +	u32 i;
> +
> +	/* Learnt entries live in a per-MAC-port read-key region. The CPU
> +	 * port has no such region.
> +	 */
> +	if (port == XLNX_TSN_CPU_PORT)
> +		return 0;
> +
> +	guard(mutex)(&sw->indirect_lock);
> +
> +	if (port == XLNX_TSN_PORT_MAC2)
> +		base = TSN_CAM_MAC2_READ_KEY_BASE;
> +	else
> +		base = 0;
> +
> +	/* Learnt entries occupy non-consecutive slots, so scan the whole
> +	 * region and report each slot marked found.
> +	 */
> +	for (i = 0; i < TSN_CAM_READ_KEY_COUNT; i++) {
> +		ret = xlnx_tsn_cam_wait_ready(sw);
> +		if (ret)
> +			return ret;
> +
> +		sw_iow(sw, TSN_CAM_CTRL_OFFSET,
> +		       FIELD_PREP(TSN_CAM_READ_KEY_ADDR, base + i) |
> +		       FIELD_PREP(TSN_CAM_OP_MASK, TSN_CAM_OP_READ_KEY) |
> +		       TSN_CAM_OP_ENABLE);
> +
> +		ret = xlnx_tsn_cam_wait_done(sw);
> +		if (ret)
> +			return ret;
> +
> +		ctrl = sw_ior(sw, TSN_CAM_CTRL_OFFSET);
> +		if (!(ctrl & TSN_CAM_FOUND))
> +			continue;
> +
> +		key1 = sw_ior(sw, TSN_CAM_KEY1_OFFSET);
> +		key2 = sw_ior(sw, TSN_CAM_KEY2_OFFSET);

[Severity: Medium]
Does this FDB dump implementation report FDB entries incorrectly if their
port list has been modified?

The dump loop checks if a slot is physically occupied but doesn't read the
TSN_CAM_PORT_ACT_OFFSET register to verify if the queried port is actually
present in the entry's port list.

If an entry's port list was later modified via xlnx_tsn_port_fdb_add() or
xlnx_tsn_port_fdb_del() to point to a different port without changing its
physical slot, xlnx_tsn_port_fdb_dump() will still report the entry as
belonging to the original port.

> +		addr[0] = key1 >> 24;
> +		addr[1] = key1 >> 16;
> +		addr[2] = key1 >> 8;
> +		addr[3] = key1;
> +		addr[4] = key2 >> 8;
> +		addr[5] = key2;

[ ... ]

> @@ -643,7 +918,7 @@ static int xlnx_tsn_setup(struct dsa_switch *ds)
>  	struct dsa_port *cpu_dp = dsa_to_port(ds, XLNX_TSN_CPU_PORT);
>  	struct xlnx_tsn *sw = ds->priv;
>  	struct dsa_port *dp;
> -	u32 mgmt;
> +	u32 mgmt, reg;
>  	int ret;
>  
>  	if (!dsa_is_user_port(ds, XLNX_TSN_PORT_MAC1) ||
> @@ -656,6 +931,22 @@ static int xlnx_tsn_setup(struct dsa_switch *ds)
>  
>  	sw->conduit = cpu_dp->conduit;
>  
> +	/* Pre-arm the learning sub-qualifiers for when a port joins a bridge:
> +	 * learn untagged frames under their ingress native VID, and allow
> +	 * learning on VIDs with no membership entry while the bridge is
> +	 * VLAN-unaware. Both bits are inert while global learning is disabled.
> +	 */
> +	reg = sw_ior(sw, TSN_SW_ADDR_LEARN_OFFSET);
> +	reg |= TSN_SW_ADDR_LEARN_DISABLE | TSN_SW_ADDR_LEARN_UNTAGGED_EN |
> +	       TSN_SW_ADDR_LEARN_NO_VLAN_EN;
> +	sw_iow(sw, TSN_SW_ADDR_LEARN_OFFSET, reg);
> +
> +	/* On a CAM miss flood unknown tagged unicast frames to all ports. */
> +	reg = sw_ior(sw, TSN_SW_CTRL_OFFSET);
> +	reg &= ~TSN_SW_CTRL_UCAST_MISS_MASK;
> +	reg |= FIELD_PREP(TSN_SW_CTRL_UCAST_MISS_MASK, TSN_SW_CTRL_UCAST_MISS_FLOOD);
> +	sw_iow(sw, TSN_SW_CTRL_OFFSET, reg);

[Severity: High]
Does this global flooding configuration break network isolation between
standalone ports?

Since standalone ports share the same default PVID and have VLAN filtering
disabled by default, enabling TSN_SW_CTRL_UCAST_MISS_FLOOD globally here
(and similarly in xlnx_tsn_port_vlan_filtering()) without configuring a port
forwarding matrix means an unknown unicast frame entering one standalone port
could be flooded directly to all other standalone ports, bypassing the CPU
network stack entirely.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=18