Re: [PATCH 19/20] net: dsa: xilinx: offload bridge VLAN filtering to the switch

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Leaking internal default VLAN tags to wire ports when VLAN filtering is disabled.
- [Medium] Mixing goto-based error handling with scope-based cleanup helpers (`guard()`, `scoped_guard()`) in the same function.
- [Medium] Missing rollback of driver software state and hardware registers on error paths in VLAN configuration.
- [Medium] Silent egress tagging policy violation when replacing a port's PVID.
--

--- Patch [19]: [PATCH 19/20] net: dsa: xilinx: offload bridge VLAN filtering to the switch ---
commit ce3068da25a97ee2632521887db3b91540fc81d6
Author: Nagadheeraj Rottela <[email protected]>

net: dsa: xilinx: offload bridge VLAN filtering to the switch

The switch always keys the L2 forwarding lookup on {DMAC, VID}. A
per-VID port-membership table controls which ports each VLAN spans.
The ingress parser assigns untagged frames the native VID of the
receiving port before lookup, so tagged and untagged traffic share
the same {DMAC, VID} key throughout.

Wire bridge VLAN filtering to the port-membership table and
native-VID registers so bridge VLAN commands take effect in
hardware. Program the native-VID register on PVID changes so
untagged ingress lands in the right VLAN.

Filtering is switch-wide, so declare it global. The per-MAC
VLAN-enable bit adjusts the MAC's maximum frame length to
account for the 4-byte tag.

> diff --git a/drivers/net/dsa/xilinx/xilinx_tsn.c b/drivers/net/dsa/xilinx/xilinx_tsn.c
> index 57558b0e2613b..7e5cf77cfa66a 100644
> --- a/drivers/net/dsa/xilinx/xilinx_tsn.c
> +++ b/drivers/net/dsa/xilinx/xilinx_tsn.c

[ ... ]

> +/* Write a port's native VID and egress-untag setting to hardware.
> + * While VLAN filtering is off, every port uses the reset-default
> + * native VID and stays tagged, so committed VLANs have no effect
> + * until the bridge goes VLAN-aware. Caller holds indirect_lock.
> + */
> +static void xlnx_tsn_apply_pvid(struct xlnx_tsn *sw, int port)
> +{
> +	u16 vid = sw->vlan_aware ? sw->pvid[port] : TSN_SW_DEFAULT_VID;
> +	bool untag = sw->vlan_aware && sw->pvid_untagged[port];

[Severity: High]
Does this leak the internal default VLAN tag to the wire when VLAN filtering
is disabled?

When the bridge operates in the default VLAN-unaware mode, the untag boolean
evaluates to false because of the sw->vlan_aware check. This causes egress
untagging to be disabled. Since ingress frames are assigned VID 1, would
they egress with the internal tag and break L2 connectivity for standard
untagged hosts?

> +
> +	xlnx_tsn_set_native_vid(sw, port, vid);
> +	xlnx_tsn_set_native_untag(sw, port, untag);
> +}

[ ... ]

> +static int xlnx_tsn_port_vlan_filtering(struct dsa_switch *ds, int port,
> +					bool vlan_filtering,
> +					struct netlink_ext_ack *extack)
> +{
> +	struct xlnx_tsn *sw = ds->priv;
> +	struct dsa_port *dp;
> +	unsigned long bit;
> +	u32 reg, data;
> +	int ret;
> +
> +	guard(mutex)(&sw->indirect_lock);
> +
> +	/* Flip Port-List-Valid on every configured VID: set it to enforce
> +	 * membership, clear it so VLANs added while filtering was off stay
> +	 * inert.
> +	 */
> +	for_each_set_bit(bit, sw->cfg_vids, VLAN_N_VID) {
> +		ret = xlnx_tsn_vlan_read(sw, bit, &data);
> +		if (ret)
> +			return ret;
> +
> +		if (vlan_filtering)
> +			data |= TSN_VLAN_PORT_LIST_VALID;
> +		else
> +			data &= ~TSN_VLAN_PORT_LIST_VALID;
> +
> +		ret = xlnx_tsn_vlan_write(sw, bit, data);
> +		if (ret)
> +			return ret;

[Severity: Medium]
If xlnx_tsn_vlan_write() fails here, does this return early without
rolling back the VLAN entries that were already modified in earlier
iterations of this loop?

This could leave the switch hardware and driver state permanently
desynchronized from the Linux bridge core's view if hardware communication
fails partway through.

> +	}
> +
> +	sw->vlan_aware = vlan_filtering;

[ ... ]

> +static int xlnx_tsn_port_vlan_add(struct dsa_switch *ds, int port,
> +				  const struct switchdev_obj_port_vlan *vlan,
> +				  struct netlink_ext_ack *extack)
> +{
> +	bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
> +	bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
> +	struct xlnx_tsn *sw = ds->priv;
> +	u32 data;
> +	int ret;
> +
> +	guard(mutex)(&sw->indirect_lock);
> +
> +	/* The hardware strips the tag on egress only for a wire port's native
> +	 * VLAN. Reject an untagged request for any other VID. The CPU port is
> +	 * exempt as it always trunks tagged toward the host.
> +	 */
> +	if (port != XLNX_TSN_CPU_PORT && untagged && !pvid &&
> +	    vlan->vid != sw->pvid[port]) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "untagged egress is only supported for the port PVID");
> +		return -EINVAL;
> +	}

[Severity: Medium]
Does this logic allow a silent egress tagging policy violation when an
administrator replaces a port's PVID?

If a port already has a VLAN configured as untagged, and a new untagged
PVID is added, the condition (untagged && !pvid) allows this replacement.
However, because the hardware only untags the active PVID, the old VLAN
will silently lose its untagging capability and begin egressing as tagged
instead of rejecting the configuration.

> +
> +	ret = xlnx_tsn_vlan_read(sw, vlan->vid, &data);

[ ... ]

> @@ -941,12 +1232,26 @@ static int xlnx_tsn_setup(struct dsa_switch *ds)
>  	       TSN_SW_ADDR_LEARN_NO_VLAN_EN;
>  	sw_iow(sw, TSN_SW_ADDR_LEARN_OFFSET, reg);
>  
> -	/* On a CAM miss flood unknown tagged unicast frames to all ports. */
> +	/* On a CAM miss flood unknown tagged unicast frames to all ports.
> +	 * Also drop tagged frames whose ingress port is not in the VLAN
> +	 * member list. The membership check only bites once VLANs exist.
> +	 */
>  	reg = sw_ior(sw, TSN_SW_CTRL_OFFSET);
> -	reg &= ~TSN_SW_CTRL_UCAST_MISS_MASK;
> -	reg |= FIELD_PREP(TSN_SW_CTRL_UCAST_MISS_MASK, TSN_SW_CTRL_UCAST_MISS_FLOOD);
> +	reg &= ~(TSN_SW_CTRL_UCAST_MISS_MASK | TSN_SW_CTRL_MEMBER_VIOL_MASK);
> +	reg |= FIELD_PREP(TSN_SW_CTRL_UCAST_MISS_MASK, TSN_SW_CTRL_UCAST_MISS_FLOOD) |
> +	       FIELD_PREP(TSN_SW_CTRL_MEMBER_VIOL_MASK, TSN_SW_CTRL_MEMBER_VIOL_DISCARD);
>  	sw_iow(sw, TSN_SW_CTRL_OFFSET, reg);
>  
> +	/* Every port uses the default native VID until VLAN filtering is
> +	 * enabled. Seed the PVID shadow and program the native VID
> +	 * registers to match.
> +	 */
> +	scoped_guard(mutex, &sw->indirect_lock)
> +		for (port = XLNX_TSN_CPU_PORT; port < XLNX_TSN_NUM_PORTS; port++) {
> +			sw->pvid[port] = TSN_SW_DEFAULT_VID;
> +			xlnx_tsn_apply_pvid(sw, port);
> +		}

[Severity: Medium]
Since xlnx_tsn_setup() uses goto-based error handling on failure paths
(such as goto err_traps, err_mdio, etc. lower in the function), is it safe
to use scoped_guard() here?

The Cleanup and Guard Subsystem guidelines explicitly forbid mixing
goto-based error handling and scope-based cleanup in the same function to
prevent confusing ownership semantics and possible leaks.

> +
>  	/* Route CPU-originated bridge-group control frames (STP, LLDP) to
>  	 * the single wire port whose MAC-nibble field matches the frame's

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=19