Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 1 · 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).
- [Medium] Static entries for VLAN-unaware bridges now share FID 4095
with hardware-learned dynamic entries.
- [Low] The commit fixes a functional bug that has existed since the
driver was added, but it carries no Fixes: tag.
- [Low] yt921x_fdb_dump() declares `u16 vid;` (line 582) with no
initializer.
--
net: dsa: motorcomm: Translate FDB VID for VLAN-unaware bridges
This patch maps VID 0 from a VLAN-unaware bridge to the unaware FID
(YT921X_VID_UNWARE, 4095) in yt921x_fdb_join() and yt921x_fdb_leave().
It also maps FID 4095 back to VID 0 in yt921x_fdb_dump(). The goal is
that static FDB entries installed through switchdev match the hardware
lookups.
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]
This describes a user-visible bug that has been there since the driver
was added. Should this carry a Fixes: tag?
YT921X_VID_UNWARE was added in the same commit as the PVID 4095 setup in
yt921x_vlan_filtering(). So were yt921x_fdb_join() and yt921x_fdb_leave(),
which write the caller's VID straight into FDB_IO1_FID. That suggests:
Fixes: 186623f4aa72 ("net: dsa: yt921x: Add support for Motorcomm YT921x")
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
index 1b05ac29ecd76..e41fe4859bfe4 100644
--- a/drivers/net/dsa/motorcomm/chip.c
+++ b/drivers/net/dsa/motorcomm/chip.c
@@ -590,6 +590,9 @@ yt921x_fdb_dump(struct yt921x_priv *priv, u16 ports_mask,
res = yt921x_fdb_read(priv, addr, &vid, &pmask, &index, &status);
if (res)
return res;
+
+ if (vid == YT921X_VID_UNWARE)
+ vid = 0;
[Severity: Low]
Can this read vid before it has been initialized?
In yt921x_fdb_dump(), vid has no initializer. yt921x_fdb_read() returns 0
without writing *vidp when the slot is NOTFOUND or has INVALID status:
yt921x_fdb_read() {
...
if (val & YT921X_FDB_RESULT_NOTFOUND) {
*ports_maskp = 0;
return 0;
}
...
if ((data1 & YT921X_FDB_IO1_STATUS_M) ==
YT921X_FDB_IO1_STATUS_INVALID) {
*ports_maskp = 0;
return 0;
}
...
}
When FDB index 0 is empty, the new comparison therefore runs on an
uninitialized stack value. The result is thrown away because cb() is
skipped when pmask is 0. KMSAN or -Wmaybe-uninitialized could still flag
it.
Before this patch, vid was only read inside the (pmask & ports_mask)
block.
The copy inside the GET_NEXT loop is fine because the !pmask break runs
first. Could this remap move inside the pmask check, or could vid be
initialized?
if ((pmask & ports_mask) && !is_multicast_ether_addr(addr)) {
res = cb(addr, vid,
status == YT921X_FDB_ENTRY_STATUS_STATIC, data);
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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;
+
/* Check for presence */
res = yt921x_fdb_has(priv, addr, vid, &index);
if (res)
[Severity: Medium]
With this mapping, static entries for a VLAN-unaware bridge now share FID
4095 with entries the hardware learns. Before, they were in FID 0, which
learning never touched. What happens when yt921x_fdb_has() finds an
already learned entry for the same MAC?
Further down in yt921x_fdb_join(), the existing entry is not replaced.
It is forced to static and the new port is ORed into its egress mask:
ctrl1 = val1 & ~YT921X_FDB_IO1_STATUS_M;
ctrl1 |= YT921X_FDB_IO1_STATUS_STATIC;
ctrl2 = val2 | YT921X_FDB_IO2_EGR_PORTS(ports_mask);
A learned entry on port P thus becomes a static entry for both P and the
new port, so unicast to that MAC is also sent to P.
On delete, yt921x_fdb_leave() only clears the departing port's bit:
ctrl2 = val2 & ~YT921X_FDB_IO2_EGR_PORTS(ports_mask);
That leaves a static entry on P that never ages out. It is not cleared by
yt921x_dsa_port_fast_age() either, because that calls
yt921x_fdb_flush_port(priv, port, false).
Assisted learning on the CPU port is one way to get there:
station X is learned in hardware on switch port P
X moves to a foreign bridge port, e.g. Wi-Fi
port_fdb_add(cpu, X, 0) -> static {P, CPU}
bridge entry ages out
port_fdb_del(cpu, X, 0) -> static {P}
If X later shows up on another switch port, hardware learning cannot
override the static entry. DSA also ignores bridge-learned entries on
offloaded ports, so traffic to X keeps going to P.
The same stale state can come from a user "bridge fdb add X dev swpA
master static" for a MAC currently learned on swpB, followed by
"bridge fdb del".
The merge logic is older than this patch and already affects VLAN-aware
bridges. This mapping makes it reachable for VLAN-unaware bridges too.
Can this leave static entries in FID 4095 that the software bridge does
not know about?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261107121631.1040612-1-mmyangfl%40gmail.com