Re: [PATCH net-next v1 4/6] net: dsa: mt7628: add VLAN filtering support

[netdev-bot+sashiko@kernel.org]

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

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

- [High] mt7628_port_vlan_add() rejects with -EBUSY any VID that is
  already recorded in a VLAN-AWARE software block by a user port…
- [High] mt7628_port_vlan_add() accepts VLAN ID 0, but `vlan->vid == 0`
  is simultaneously the driver's 'slot is free' sentinel…
- [High] mt7628_vlan_sync() programs the per-VLAN egress-untag bitmap
  (VUB) with the CPU port bit set whenever a VLAN carries…
- [High] Two standalone ports can be incorrectly allowed to share the
  same VLAN ID, which breaks port isolation if they are later enslaved…
- [Low] Retained for policy combination only; my baseline verdict is
  FALSE POSITIVE.
- [Low] Retained for policy combination only; my baseline verdict is
  FALSE POSITIVE.

--

net: dsa: mt7628: add VLAN filtering support

This adds .port_vlan_filtering, .port_vlan_add and .port_vlan_del to the
MT7628 built-in switch driver.  Membership for tag_8021q and
user-requested VLANs is tracked in software, and only one of the two
VLAN types is programmed into hardware per port depending on that port's
VLAN filtering state.  A few questions below about the new
mt7628_vlan_sync() masking and about the input validation in
mt7628_port_vlan_add().

> diff --git a/drivers/net/dsa/mt7628.c b/drivers/net/dsa/mt7628.c
> index 7eabfad3e1144..629dbf968a171 100644
> --- a/drivers/net/dsa/mt7628.c
> +++ b/drivers/net/dsa/mt7628.c

[ ... ]

> @@ -442,10 +443,16 @@ static int mt7628_port_join_vlan_block(struct dsa_switch *ds, int port, u16 vid,
>  		return -ENOSPC;
>  
>  	vlan->members |= BIT(port);
> +
>  	if (flags & BRIDGE_VLAN_INFO_PVID)
> -		esw->tag_8021q_pvid[port] = vid;
> +		esw->pvid[type][port] = vid;
> +	else if (esw->pvid[type][port] == vid)
> +		esw->pvid[type][port] = 0;
> +
>  	if (flags & BRIDGE_VLAN_INFO_UNTAGGED)
>  		vlan->untag |= BIT(port);
> +	else
> +		vlan->untag &= ~BIT(port);
>  	return 0;
>  }

[ ... ]

> @@ -473,18 +480,42 @@ static int mt7628_port_leave_vlan_block(struct dsa_switch *ds, int port,
>  static void mt7628_vlan_sync(struct dsa_switch *ds)
>  {
>  	struct mt7628_esw *esw = ds->priv;
> +	struct dsa_port *dp;
>  	int i;
>  
>  	for (i = 0; i < MT7628_NUM_VLANS; i++) {
>  		struct mt7628_vlan *vlan = &esw->vlans[i];
> +		u8 member_mask;
> +
> +		if (vlan->type == MT7628_VLAN_TYPE_AWARE)
> +			member_mask = esw->vlan_filtering;
> +		else
> +			member_mask = ~esw->vlan_filtering;
> +		member_mask |= MT7628_ESW_PORTS_CPU;

[Severity: High]
Should the CPU bit be forced into the untag mask as well as the member
mask here?

member_mask always has MT7628_ESW_PORTS_CPU OR'ed in, and it is applied
to both values:

	mt7628_esw_set_vmsc(esw, i, vlan->members & member_mask);
	mt7628_esw_set_vub(esw, i, vlan->untag & member_mask);

The CPU port bit can be set in vlan->untag: when a port joins a bridge,
br_switchdev_vlan_replay() replays the bridge master's default_pvid with
BRIDGE_VLAN_INFO_UNTAGGED, and dsa_user_host_vlan_add() only clears
BRIDGE_VLAN_INFO_PVID:

net/dsa/user.c:dsa_user_host_vlan_add() {
	...
	/* Even though drivers often handle CPU membership in special ways,
	 * it doesn't make sense to program a PVID, so clear this flag.
	 */
	vlan.flags &= ~BRIDGE_VLAN_INFO_PVID;
	...
}

so mt7628_port_join_vlan_block() executes vlan->untag |= BIT(cpu), and
MT7628_ESW_VUB_M covers bit 6, so the CPU egress untag really lands in
hardware.

The MT7628 special tag lives in the VLAN tag itself, and the receive path
decodes it unconditionally:

net/dsa/tag_mt7628.c:mt7628_tag_rcv() {
	phdr = dsa_etype_header_pos_rx(skb);
	...
	skb_pull_rcsum(skb, MT7628_TAG_LEN);
	dsa_strip_etype_header(skb, MT7628_TAG_LEN);
}

If frames reach the conduit untagged, does mt7628_tag_rcv() then read the
frame's real EtherType as the source port and eat 4 bytes of payload?
For IPv4 (0x0800) the port field decodes as 0 and 4 bytes of the IP
header are removed; for ARP (0x0806) it decodes as 6 (the CPU port) and
dsa_conduit_find_user() returns NULL.

Before this patch tag_8021q passed flags = 0 for CPU and DSA ports, so
the CPU untag bit could never be set.  Would masking BIT(cpu) out of the
VUB value (keeping the CPU a tagged member of every VLAN) be the right
fix?

> +		/*
> +		 * Put VLAN filtering ports only into VLAN aware VLANs and
> +		 * non VLAN filtering ports into VLAN unaware VLANs.
> +		 *
> +		 * CPU may not be removed from any VLAN, as VLAN filtering
> +		 * applies only to user ports.
> +		 */
>  
> -		mt7628_esw_set_vmsc(esw, i, vlan->members);
>  		mt7628_esw_set_vlan_id(esw, i, vlan->vid);
> -		mt7628_esw_set_vub(esw, i, vlan->untag);
> +		mt7628_esw_set_vmsc(esw, i, vlan->members & member_mask);
> +		mt7628_esw_set_vub(esw, i, vlan->untag & member_mask);

[Severity: Low]
This one looks like it resolves itself later in the series, so it is more
of a note than a bug report.

Masking here removes VLAN-filtering ports from the membership and untag
masks of all UNAWARE (tag_8021q) blocks, while mt7628_port_vlan_add()
refuses dsa_8021q VIDs in AWARE blocks, so at this commit no VLAN entry
permits or untags the VID that mt7628_tag_xmit() inserts for such a port.

The later commit "net: dsa: tag: mt7628: add VLAN awareness support" adds
a VLAN-aware egress path that only rewrites the TPID:

net/dsa/tag_mt7628.c:mt7628_tag_xmit() {
	if (skb->offload_fwd_mark &&
	    br_vlan_enabled(dsa_port_bridge_dev_get(dp))) {
		tag = dsa_etype_header_pos_tx(skb);
		tag[0] = htons(xmit_tpid);
		return skb;
	}
	...
}

so tag_8021q membership is no longer needed for filtering ports.  Frames
injected directly on a bridged user netdev (offload_fwd_mark == 0, e.g.
STP BPDUs) still use the standalone tag_8021q VID though - is that path
expected to work on a filtering port?

> +
>  	}
>  
> -	for (i = 0; i < ds->num_ports; i++)
> -		mt7628_esw_set_pvid(esw, i, esw->tag_8021q_pvid[i]);
> +	dsa_switch_for_each_user_port(dp, ds) {
> +		unsigned int type = BIT(dp->index) & esw->vlan_filtering ?
> +		    MT7628_VLAN_TYPE_AWARE : MT7628_VLAN_TYPE_UNAWARE;
> +		mt7628_esw_set_pvid(esw, dp->index, esw->pvid[type][dp->index]);
> +	}
> +	regmap_update_bits(esw->regmap, MT7628_ESW_REG_SGC2,
> +			   MT7628_ESW_SGC2_DOUBLE_TAG_EN,
> +			   FIELD_PREP(MT7628_ESW_SGC2_DOUBLE_TAG_EN,
> +				      MT7628_ESW_PORTS_NOCPU &
> +				      ~esw->vlan_filtering));

[Severity: Low]
Also not a bug at the end of the series, just a bisectability note.

Clearing MT7628_ESW_SGC2_DOUBLE_TAG_EN for VLAN-filtering ports means the
switch no longer inserts an outer tag, so at this commit mt7628_tag_rcv()
decodes the source port from the wire TPID and strips 4 bytes.  The later
commit "net: dsa: tag: mt7628: add VLAN awareness support" handles that
case:

net/dsa/tag_mt7628.c:mt7628_tag_rcv() {
	...
	if (source_port == 0 && !is_dsa_8021q)
		skb->dev = dsa_find_designated_bridge_port_by_vid(dev, vid);
	else
		skb->dev = dsa_conduit_find_user(dev, 0, source_port);
	...
}

Since the driver change lands before the tagger change, is receive broken
for VLAN-filtering ports at this point in the series?  Would reordering
the two patches help bisection?

>  }
>  
>  static int mt7628_setup(struct dsa_switch *ds)

[ ... ]

> @@ -598,6 +629,78 @@ static int mt7628_dsa_8021q_vlan_del(struct dsa_switch *ds, int port, u16 vid)
>  	return 0;
>  }
>  
> +static int mt7628_port_vlan_filtering(struct dsa_switch *ds, int port,
> +				      bool vlan_filtering,
> +				      struct netlink_ext_ack *extack)
> +{
> +	struct mt7628_esw *esw = ds->priv;
> +
> +	if (vlan_filtering)
> +		esw->vlan_filtering |= BIT(port);
> +	else
> +		esw->vlan_filtering &= ~BIT(port);
> +	mt7628_vlan_sync(ds);
> +	return 0;
> +}
> +
> +static int mt7628_port_vlan_add(struct dsa_switch *ds, int port,
> +				const struct switchdev_obj_port_vlan *vlan,
> +				struct netlink_ext_ack *extack)
> +{
> +	struct mt7628_vlan *vlan_block;
> +	struct dsa_port *other_dp;
> +	struct dsa_port *dp;
> +	int ret;
> +
> +	if (vid_is_dsa_8021q(vlan->vid)) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "Range 3072-4095 reserved for dsa_8021q operation");
> +		return -EBUSY;
> +	}

[Severity: High]
Does VLAN ID 0 need to be rejected (or ignored) here too?

VID 0 is the driver's "slot is free" sentinel:

drivers/net/dsa/mt7628.c:mt7628_find_vlan_block() {
	for (i = 0; i < MT7628_NUM_VLANS; i++) {
		vlan = &esw->vlans[i];
		if (vlan->vid == vid && vlan->type == type)
			return vlan;
	}
}

drivers/net/dsa/mt7628.c:mt7628_alloc_vlan_block() {
		if (vlan->vid)
			continue;
		if (vlan->type != type)
			continue;
		vlan->vid = vid;
		return vlan;
}

VID 0 arrives on its own for a VLAN-filtering port: dsa_port_vlan_filtering()
sets NETIF_F_HW_VLAN_CTAG_FILTER on the user netdev, and on the next
NETDEV_UP the 8021q core pushes VID 0 down:

net/8021q/vlan.c:vlan_vid0_add() {
	if (!(dev->features & NETIF_F_HW_VLAN_CTAG_FILTER))
		return;

	pr_info("adding VLAN 0 to HW filter on device %s\n", dev->name);

	err = vlan_vid_add(dev, htons(ETH_P_8021Q), 0);
}

which reaches dsa_user_vlan_rx_add_vid() -> dsa_port_vlan_add() ->
ds->ops->port_vlan_add() with vid 0.  So "ip link set swp0 master br0"
(vlan_filtering 1) followed by "ip link set swp0 up" delivers vid 0 here.
mt7530, b53, mv88e6xxx and rtl8365mb all special-case vid 0 in their
.port_vlan_add for this reason.

With vid 0, mt7628_find_vlan_block() matches the first unused AWARE slot,
so mt7628_alloc_vlan_block() never runs and mt7628_port_join_vlan_block()
leaves vlan->vid at 0 while doing:

	vlan->members |= BIT(port);

Can the next real VLAN add then be handed that same still-vid-0 slot by
mt7628_alloc_vlan_block(), inheriting the stale members bitmap?  In that
case the block did not exist before the add, so the cross-bridge check
below is skipped, and mt7628_vlan_sync() would program a port of one
bridge as a hardware member of another bridge's VLAN.

> +
> +	vlan_block =
> +	    mt7628_find_vlan_block(ds, vlan->vid, MT7628_VLAN_TYPE_AWARE);
> +	dp = dsa_to_port(ds, port);
> +	/*
> +	 * CPU port can be added to any VLANs, but user ports need to ensure,
> +	 * that if the VLAN already exists it's not used by a bridge we're not
> +	 * a member of, because VLANs are the only form of forwarding control
> +	 * we have on this switch.
> +	 */
> +	if (vlan_block && !dsa_port_is_cpu(dp)) {
> +		dsa_switch_for_each_user_port(other_dp, ds) {
> +			if (other_dp == dp)
> +				continue;
> +			if (other_dp->bridge == dp->bridge)
> +				continue;
> +			if (!(vlan_block->members & BIT(other_dp->index)))
> +				continue;
> +			NL_SET_ERR_MSG_MOD(extack,
> +					   "VLAN ID used on another bridge");
> +			return -EBUSY;
> +		}
> +	}

[Severity: High]
Should this comparison require that the two ports are actually
bridged, rather than just having equal dp->bridge pointers?

For two standalone user ports both dp->bridge and other_dp->bridge are
NULL, so the test above evaluates NULL == NULL and skips the conflict
check - the two ports are treated as if they were members of the same
forwarding domain.  Standalone ports do reach .port_vlan_add: adding an
8021q upper ("ip link add link swp0 name swp0.10 type vlan id 10")
goes through dsa_user_vlan_rx_add_vid() -> dsa_port_vlan_add() ->
mt7628_port_vlan_add(), so doing that on swp0 and swp1 leaves the AWARE
block for VID 10 with members = BIT(swp0) | BIT(swp1).

Nothing re-validates that state later.  The membership only becomes
visible in hardware once esw->vlan_filtering gains the port bits, and
mt7628_port_vlan_filtering() just sets/clears the bit and calls
mt7628_vlan_sync(), which programs vlan->members & esw->vlan_filtering
into the VMSC entry.  So if the two ports are subsequently enslaved to
different VLAN-aware bridges, is the shared VID 10 block programmed with
both ports as members, giving exactly the cross-bridge forwarding this
check was written to prevent?

Treating a NULL bridge as its own isolation domain (i.e. only skipping
when both ports are bridged into the same dsa_bridge) would catch the
add-then-enslave order too, but it would also refuse the very common
case of the same VLAN ID on two independent standalone ports, which
works fine as long as neither is bridged.  Would it be better to keep
the permissive behaviour at add time and instead re-run the conflict
check from .port_bridge_join / .port_vlan_filtering, where the actual
isolation domains are known?

[Severity: High]
Does this check prevent a second bridge from ever being created on this
switch?

DSA sets ds->configure_vlan_while_not_filtering = true before ops->setup(),
so bridge VLANs are handed to the driver regardless of the bridge's
vlan_filtering state.  br_add_if() then unconditionally installs
br->default_pvid (1, with PVID|UNTAGGED|BRENTRY) and aborts the
enslavement on error:

net/bridge/br_if.c:br_add_if() {
	err = nbp_vlan_init(p, extack);
	if (err) {
		netdev_err(dev, "failed to initialize vlan filtering on this port\n");
		goto err6;
	}
}

__vlan_vid_add() propagates any switchdev error other than -EOPNOTSUPP,
so the -EBUSY above is fatal.  After "ip link set swp0 master br0" the
AWARE block for VID 1 has members = BIT(swp0); "ip link set swp1 master
br1" then finds that block, sees a different dp->bridge pointer, and
returns -EBUSY.  Before this patch .port_vlan_add was absent, so
dsa_switch_vlan_add() returned -EOPNOTSUPP and the join succeeded, which
makes the max_num_bridges = DSA_TAG_8021Q_MAX_NUM_BRIDGES support added
earlier in the series unreachable without first changing
vlan_default_pvid on every bridge.

Would it make sense to skip the rejection when the conflicting membership
is not actually programmed?  mt7628_vlan_sync() masks AWARE members with
esw->vlan_filtering:

	if (vlan->type == MT7628_VLAN_TYPE_AWARE)
		member_mask = esw->vlan_filtering;

so a non-filtering port is never written into an AWARE VMSC entry and no
cross-bridge forwarding could result from it.

> +	ret =
> +	    mt7628_port_join_vlan_block(ds, port, vlan->vid,
> +					MT7628_VLAN_TYPE_AWARE, vlan->flags);
> +	if (ret)
> +		return ret;
> +
> +	mt7628_vlan_sync(ds);
> +	return 0;
> +}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260906171625.533915-1-joey%40tinyisr.com