Re: [PATCH net-next v5 5/7] net: dsa: motorcomm: Translate FDB VID for VLAN-unaware bridges

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] In drivers/net/dsa/motorcomm/chip.c, yt921x_fdb_join() (line
  798) and yt921x_fdb_leave() (line 749) now rewrite every vid-0 FDB/MDB…
- [Low] The patch fixes a defect that has existed since the driver was
  added in commit 186623f4aa72 ("net: dsa: yt921x: Add support for…
- [Low] The commit message describes only FDB entries ('A VLAN-unaware
  bridge reports VID 0 for its FDB entries').

--

net: dsa: motorcomm: Translate FDB VID for VLAN-unaware bridges

This patch remaps VID 0 to the VLAN-unaware FID (YT921X_VID_UNWARE, 4095)
when adding or removing entries in yt921x_fdb_join() and
yt921x_fdb_leave(). It also maps FID 4095 back to VID 0 when unicast
entries are dumped in yt921x_fdb_dump().

> A VLAN-unaware bridge reports VID 0 for its FDB entries, but lookups run
> with the unaware FID (VID 4095). Entries added through switchdev were
> installed under FID 0 and could never be hit by the hardware lookups.

[Severity: Low]
The commit message only mentions FDB entries. However,
yt921x_fdb_join() and yt921x_fdb_leave() are also the backends for
yt921x_dsa_port_mdb_add() and yt921x_dsa_port_mdb_del():

yt921x_dsa_port_mdb_add()
    res = yt921x_fdb_join(priv, addr, vid, BIT(port));

yt921x_dsa_port_mdb_del()
    res = yt921x_fdb_leave(priv, addr, vid, BIT(port));

That means every vid 0 MDB entry on a VLAN-unaware bridge, from IGMP/MLD
snooping or static mdb entries, also moves from the unused FID 0 into
the live FID 4095.

Before this patch, the hardware treated known-group multicast as unknown
and trapped it to the CPU. The software bridge then forwarded it to the
group members and to the multicast router ports.

Now the hardware forwards it only to the MDB member ports. When the CPU
port is in the mask, the frame arrives with code FORWARD and
offload_fwd_mark set, so software does not forward it again. net/dsa has
no mrouter handling, so router ports that are not group members appear
to stop receiving this traffic.

Could the commit message describe this change in multicast forwarding?
The "back and forth" translation on the dump side also skips multicast
entries.

[ ... ]

>     Signed-off-by: David Yang <mmyangfl@gmail.com>

[Severity: Low]
This isn't a bug, but should this carry a Fixes: tag? Since the driver
was added, yt921x_fdb_join() and yt921x_fdb_leave() have passed vid 0
straight through to FID 0:

Fixes: 186623f4aa72 ("net: dsa: yt921x: Add support for Motorcomm YT921x")

Static entries were never offloaded. Host FDB entries installed through
assisted_learning_on_cpu_port also never overrode the hardware-learned
FID 4095 entry. So a station that moved from a switch port to a CPU-side
interface kept being forwarded to its old port until ageing.

Note that backporting this patch as written would also backport the
cross-domain FID 4095 issue raised below.

> diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
> index fbf3a0cba49b3..46534127d070e 100644
> --- a/drivers/net/dsa/motorcomm/chip.c
> +++ b/drivers/net/dsa/motorcomm/chip.c

[ ... ]

> @@ -740,6 +745,10 @@ yt921x_fdb_leave(struct yt921x_priv *priv, const unsigned char *addr,
>  	u32 val;
>  	int res;
>  
> +	/* See yt921x_fdb_join() */
> +	if (!vid)
> +		vid = YT921X_VID_UNWARE;
> +
>  	/* Check for presence */
>  	res = yt921x_fdb_has(priv, addr, vid, &index);
>  	if (res)

[ ... ]

> @@ -782,6 +791,13 @@ yt921x_fdb_join(struct yt921x_priv *priv, const unsigned char *addr, u16 vid,
>  	u32 val2;
>  	int res;
>  
> +	/* A vlan-unaware bridge reports vid 0, but untagged lookups on its
> +	 * ports run with the unaware FID; map so that static entries match the
> +	 * hardware lookups.
> +	 */
> +	if (!vid)
> +		vid = YT921X_VID_UNWARE;

[Severity: High]
Is this remap safe when more than one forwarding domain uses FID 4095,
or when a vid 0 entry doesn't come from a VLAN-unaware bridge?

Both here and in yt921x_fdb_leave(), every vid 0 add or delete is
remapped. The code ignores the dsa_db argument and never checks the
bridge's vlan_filtering state. Every VLAN-unaware bridge port and every
standalone port seems to share FID 4095:

yt921x_vlan_filtering()
	if (!bdev || !vlan_filtering)
		pvid = YT921X_VID_UNWARE;

yt921x_userport_standalone()
	res = yt921x_port_set_pvid(priv, port, YT921X_VID_UNWARE);

The driver doesn't set ds->fdb_isolation or program a separate FID per
bridge.

Before this patch, vid 0 entries landed in FID 0, which nothing looked
up. Now they are live STATIC entries in the shared FID. The comment in
yt921x_chip_setup_dsa() describes what happens on a cross-domain hit:

	 * isolation takes _after_ FDB lookups, i.e. if an FDB entry (from
	 * another bridge) is matched and the destination port (in another
	 * bridge) is blocked, the packet will be dropped instead of flooding to

Take a VLAN-unaware LAN bridge where a host on swp1 has joined multicast
group G, plus a standalone WAN port:

yt921x_dsa_port_mdb_add(swp1, G, vid 0)
  yt921x_fdb_join()
    vid = YT921X_VID_UNWARE
    STATIC entry {G, FID 4095, egress swp1}

A frame for G that arrives on the standalone WAN port is looked up in
FID 4095 and matches the LAN entry. The standalone isolation mask
(~cpu ports) then removes swp1, so the frame is dropped. Before this
patch, the same frame was unknown multicast and was trapped to the CPU.

Two VLAN-unaware bridges behave the same way. br1 traffic for a MAC or
group known only in br0 is dropped instead of being trapped. If the CPU
port is in the entry, the frame arrives with code FORWARD and
yt921x_tag_rcv() calls dsa_default_offload_fwd_mark(). Then the br1
software path won't forward it to br1 switch ports either.

VLAN-aware bridges send vid 0 entries down too, because br_fdb_add()
adds a vid 0 entry when no VLAN is given. VLAN-aware ports fall back to
pvid 0 rather than 4095:

yt921x_pvid_clear()
	res = yt921x_port_set_pvid(priv, port,
				   vlan_filtering ? 0 : YT921X_VID_UNWARE);

So in FID 4095 those entries would only affect other domains.

The entries are STATIC, so hardware learning can't correct this.

Could the remap be limited to entries that really belong to a
VLAN-unaware bridge, for example based on db.bridge or the port's
VLAN-filtering state, with care for vlan_filtering toggles? The other
option would be for the driver to separate FIDs per bridge first.

Later patches in the series don't touch yt921x_fdb_join() or
yt921x_fdb_leave(). The flood offload patch ("Support BR_FLOOD and
BR_MCAST_FLOOD offloading") mentions that cross-bridge FDB hits are
dropped, but it only restricts flooding of unknown traffic.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261108214450.1662362-1-mmyangfl%40gmail.com