Re: [PATCH net-next v15 8/9] net: dsa: lan9645x: add bridge support

[netdev-bot+sashiko@kernel.org]

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

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

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] The 802.1Q-only restriction can be bypassed by changing the
  bridge's vlan_protocol after the port has joined.

--

net: dsa: lan9645x: add bridge support

This adds hardware offload of a single bridge to the lan9645x DSA
driver. It covers ageing time, bridge port flags, STP state handling and
host flooding. Joining a VLAN-aware bridge whose vlan_protocol is not
802.1Q is refused at join time.

> diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> index d2e8c52e2a178..c6371a7b9a52a 100644
> --- a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c

[ ... ]

> +static int lan9645x_port_bridge_join(struct dsa_switch *ds, int port,
> +				     struct dsa_bridge bridge,
> +				     bool *tx_fwd_offload,
> +				     struct netlink_ext_ack *extack)
> +{
> +	struct lan9645x *lan9645x = ds->priv;
> +	struct lan9645x_port *p;
> +
> +	if (!lan9645x_bridge_vlan_proto_ok(bridge.dev)) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "Only 802.1Q VLAN-aware bridges are supported");
> +		return -EINVAL;
> +	}

[Severity: High]
Can the 802.1Q-only restriction be bypassed by changing vlan_protocol
after the port has joined?

lan9645x_bridge_vlan_proto_ok() is only checked in two places. One is
here, at join time. The other is lan9645x_port_vlan_filtering(). DSA
only calls lan9645x_port_vlan_filtering() when the filtering state
changes, because dsa_port_vlan_filtering() returns early when
dsa_port_is_vlan_filtering(dp) already matches.

A protocol change goes through __br_vlan_set_proto():

net/bridge/br_vlan.c:__br_vlan_set_proto() {
	struct switchdev_attr attr = {
		.orig_dev = br->dev,
		.id = SWITCHDEV_ATTR_ID_BRIDGE_VLAN_PROTOCOL,
		.flags = SWITCHDEV_F_SKIP_EOPNOTSUPP,
    ...
	err = switchdev_port_attr_set(br->dev, &attr, extack);
	if (err && err != -EOPNOTSUPP)
		return err;
    ...
	br->vlan_proto = proto;
    ...
}

Only mlxsw and mlx5 handle SWITCHDEV_ATTR_ID_BRIDGE_VLAN_PROTOCOL. DSA
has no case for it and returns -EOPNOTSUPP. The bridge then commits the
new protocol, and lan9645x never sees the change.

br_changelink() also handles IFLA_BR_VLAN_FILTERING before
IFLA_BR_VLAN_PROTOCOL. So both of these sequences look like they end up
with an offloaded VLAN-aware 802.1ad bridge:

  ip link set br0 type bridge vlan_filtering 1 vlan_protocol 802.1ad

  ip link set br0 type bridge vlan_protocol 802.1ad
  (on a bridge that already has vlan_filtering 1)

In the first case, lan9645x_port_vlan_filtering() runs while
br->vlan_proto is still 802.1Q, so the check passes.

The hardware then keeps its 802.1Q setup in ANA_VLAN_CFG: VLAN aware,
C-tag type, pop count 1. Would it then classify customer 0x8100-tagged
frames by the C-VID the sender chose, and forward them in hardware to
members of that VID?

Under 802.1ad, the software bridge treats that tag as payload and puts
the frame in the port PVID (the S-VLAN). If the hardware classifies by
the C-VID instead, S-VLAN isolation breaks for traffic arriving from
the wire.

The v15 changelog says VLAN-aware bridges whose vlan_protocol is not
802.1Q are refused. Does this need handling for protocol changes, rather
than only the join-time check? Two options would be a veto from a
switchdev or netdev notifier, or a DSA hook for
SWITCHDEV_ATTR_ID_BRIDGE_VLAN_PROTOCOL.

The later patch in the series, "net: dsa: lan9645x: add port
statistics", only adds statistics and does not change this logic.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015-dsa_lan9645x_switch_driver_base-v15-0-562f316f78b9%40microchip.com