Re: [PATCH net v2 2/2] net: dsa: realtek: rtl8365mb: keep the neutral PVID in 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 VLAN-unaware bridges, rtl8365mb_port_commit_pvid() now sets
  pvid = 0 (VLANMC index 0, EVID 0).
- [Low] This patch removes the only callers of
  rtl8365mb_vlan_port_get_pvid() (in rtl8365mb_port_vlan_filtering() and…
- [Low] The patch updates the file-header comment in rtl8365mb_vlan.c to
  say that VLANMC index 0 is the neutral PVID "for standalone ports and…

--

net: dsa: realtek: rtl8365mb: keep the neutral PVID in VLAN-unaware bridges

The patch stores the bridge PVID per port in bridge_pvid and only programs
it while the port filters VLANs. Ports of a VLAN-unaware bridge keep the
neutral PVID (VLANMC index 0, VID 0), so untagged frames match the VID 0
FDB/MDB entries that DSA installs. It also replaces
rtl8365mb_vlan_pvid_port_set() and rtl8365mb_vlan_pvid_port_clear() with
rtl8365mb_vlan_pvid_port_commit(), which programs both the PVID and the
accepted frame types.

> diff --git a/drivers/net/dsa/realtek/rtl8365mb_main.c b/drivers/net/dsa/realtek/rtl8365mb_main.c
> index b90e46c165bf8..4f0e0ab7f5023 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb_main.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb_main.c

[ ... ]

> @@ -2004,42 +2007,44 @@ rtl8365mb_port_set_vlan_egress_mode(struct realtek_priv *priv, int port,
>  			RTL8365MB_PORT_MISC_CFG_VLAN_EGRESS_MODE_MASK, val);
>  }
>  
> +/* Program the PVID that the port should use for untagged frames: the PVID of
> + * the bridge while the port filters VLANs, and the neutral PVID (VID 0)
> + * otherwise. A VLAN-unaware bridge, like a standalone port, gets its FDB and
> + * MDB entries with VID 0 from DSA. As the switch looks up its forwarding
> + * database by the VLAN that it classifies a frame to, a bridge PVID would make
> + * those entries miss every lookup.
> + */
> +static int rtl8365mb_port_commit_pvid(struct dsa_switch *ds, int port,
> +				      bool vlan_filtering,
> +				      struct netlink_ext_ack *extack)
> +{
> +	struct realtek_priv *priv = ds->priv;
> +	struct rtl8365mb *mb = priv->chip_data;
> +	u16 pvid;
> +
> +	if (vlan_filtering)
> +		pvid = mb->ports[port].bridge_pvid;
> +	else
> +		pvid = 0;

[Severity: High]
With this change, untagged frames from every VLAN-unaware bridge are
classified to VID 0, which is the VID standalone ports already use. The
previous commit made VID 0 an IVL VLAN. Taken together, do multicast
lookups from all bridges and all standalone ports now land in one shared
{MAC, VID 0} space?

The commit message says:

  "The per-bridge EFID keeps these lookups local to the bridge."

That looks true for unicast only. Hardware multicast entries have no EFID
field, and rtl8365mb_l2_add_mc() says so:

drivers/net/dsa/realtek/rtl8365mb_l2.c:rtl8365mb_l2_add_mc() {
    ...
		 * Entries from different bridges will collide on the same
		 * MAC+VID slot with an OR'ed member mask, but packets will
		 * only exit through ports allowed by the source port's pmask.
    ...
}

Also, rtl83xx_port_mdb_add() accepts both DSA_DB_PORT and DSA_DB_BRIDGE,
and it only uses the EFID in a dev_dbg():

	/* EFID is not used by hardware MDB entries; debugging only */

Before this patch, bridge traffic was classified to the bridge PVID
(VID 1). No VID 0 multicast entry could match it, so unregistered
multicast was flooded within the pmask. Three cases look affected now.

First, DSA installs DSA_DB_PORT host entries at VID 0 for every user
port's mc list, whether the port is bridged or not:

dsa_user_sync_mc()
  -> dsa_user_vlan_for_each()      /* always calls cb(arg, 0) */
  -> dsa_user_standalone_event_work()
    -> dsa_port_standalone_host_mdb_add()

Say an untagged frame arrives on lan1 for 33:33:00:00:00:01, for example
an IPv6 RA or an MLD general query from an upstream router. It now hits a
{CPU}-only entry. Does it still get flooded to the other ports of the
bridge?

rtl8_4_read_tag() calls dsa_default_offload_fwd_mark() for any frame that
was not trapped. That means the software bridge would not re-flood the
frame either. IPv6 33:33:xx groups use DMAC lookup. Whether IPv4 groups
do as well depends on the IPMC lookup mode, which the driver does not
configure.

Second, suppose bridge B snoops a group G and installs {G, VID 0} -> {b1}.
Would that entry now also match traffic for G inside bridge A? The result
would be member & pmask(a1) = {} (dropped) or {CPU}, where bridge A would
otherwise have flooded the unregistered group.

Third, DSA refcounts CPU port MDB entries per database:
dsa_port_do_mdb_del() looks up {addr, vid, db}. However,
rtl8365mb_l2_del_mc() still clears the port bit on the single shared
slot:

	mc.member &= ~BIT(port);

When one bridge or standalone port leaves G, does this remove host
delivery of G for the other databases that still need it?

Does this need per-bridge separation of VID 0 multicast, for example a
distinct classification VID or FID per VLAN-unaware bridge? The other
option would be per-database handling of the shared slot.

> +
> +	return rtl8365mb_vlan_pvid_port_commit(ds, port, pvid, vlan_filtering,
> +					       extack);
> +}
> +
>  static int rtl8365mb_port_vlan_filtering(struct dsa_switch *ds, int port,
>  					 bool vlan_filtering,
>  					 struct netlink_ext_ack *extack)
>  {
> -	enum rtl8365mb_frame_ingress accepted_frame, prev_accepted_frame;
>  	enum rtl8365mb_vlan_egress_mode mode;
>  	struct realtek_priv *priv = ds->priv;
>  	u32 configured_ports = 0;
>  	struct dsa_port *dp;
> -	u16 pvid_vid;
>  	int ret;
>  
>  	dev_dbg(priv->dev, "port %d: %s VLAN filtering\n", port,
>  		vlan_filtering ? "enable" : "disable");
>  
> -	ret = rtl8365mb_vlan_port_get_framefilter(priv, port,
> -						  &prev_accepted_frame);
> -	if (ret) {
> -		NL_SET_ERR_MSG_MOD(extack,
> -				   "Failed to get current framefilter");
> -		return ret;
> -	}
> -
> -	/* While filtering, only accepts untagged frames if PVID is enabled */
> -	if (vlan_filtering) {
> -		ret = rtl8365mb_vlan_port_get_pvid(priv, port, &pvid_vid);

[Severity: Low]
This hunk and the one in rtl8365mb_port_vlan_add() remove the only
callers of rtl8365mb_vlan_port_get_pvid(). The function is still defined
non-static in rtl8365mb_vlan.c and declared in rtl8365mb_vlan.h, so the
compiler will not warn that it is unused. Can it be dropped?

Similarly, the only remaining users of
rtl8365mb_vlan_port_get_framefilter() and
rtl8365mb_vlan_port_set_framefilter() are in
rtl8365mb_vlan_pvid_port_commit() in rtl8365mb_vlan.c. Could they be made
static and removed from the header?

> -		if (ret)
> -			return ret;

[ ... ]

> diff --git a/drivers/net/dsa/realtek/rtl8365mb_vlan.c b/drivers/net/dsa/realtek/rtl8365mb_vlan.c
> index 58948e76f122a..64acd762e912f 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb_vlan.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb_vlan.c
> @@ -42,7 +42,8 @@
>   *    may be useful for ACL rules which specify alternative forwarding
>   *    decisions.
>   *
> - * VLANMC index 0 is reserved as a neutral PVID, used for standalone ports.
> + * VLANMC index 0 is reserved as a neutral PVID, used for standalone ports and
> + * for the ports of a VLAN-unaware bridge.

[Severity: Low]
This isn't a bug, but the matching comment in rtl8365mb_vlan_mc_find()
was not updated. It still says:

	/* By design, VlanMC[0] is reserved as a neutral PVID value for
	 * standalone ports. It always has EVID == 0. That way, we assume that

Should it also mention VLAN-unaware bridge ports?

>   *
>   */

[ ... ]

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