Re: [PATCH net-next 8/9] net: dsa: netc: add bridge mode support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] netc_single_vlan_aware_bridge() fails to enforce the single VLAN-aware bridge constraint due to checking stale state
- [High] The driver incorrectly assumes DSA bridge numbers start at 1, causing the first bridge to use an out-of-bounds VLAN ID (4096) and breaking port isolation
- [High] FDB entry deletion by key blindly deletes dynamically learned entries belonging to other ports
- [High] Removing the PVID from a VLAN-aware bridge port assigns it VID 0, leaking untagged frames to the standalone broadcast domain
- [High] Returning -EOPNOTSUPP for unsupported configurations causes the bridge core to silently ignore the errors
--

commit 7827b368f25c80b95ec098de8651a6d572a40bbf
Author: Wei Fang <wei.fang@nxp.com>
Subject: net: dsa: netc: add bridge mode support

Wire up the port_bridge_join, port_bridge_leave and port_vlan_filtering
DSA callbacks to support both VLAN-unaware and VLAN-aware bridge modes.

> diff --git a/drivers/net/dsa/netc/netc_main.c b/drivers/net/dsa/netc/netc_main.c
> index 4db42c888470c..1fe3b43e04598 100644
> --- a/drivers/net/dsa/netc/netc_main.c
> +++ b/drivers/net/dsa/netc/netc_main.c
> @@ -694,13 +694,18 @@ static int netc_port_del_fdb_entry(struct netc_port *np,
>  	mutex_lock(&priv->fdbt_lock);
>  
>  	entry = netc_lookup_fdb_entry(priv, addr, vid);
> -	if (unlikely(!entry))
> -		/* Currently only single port mode is supported, MAC learning
> -		 * is disabled, so there is no dynamically learned FDB entry.
> -		 * We need to support deleting dynamically FDB entry when the
> -		 * bridge mode is supported.
> -		 */
> +	/* If the entry is NULL, the specified FDB entry might be a dynamic
> +	 * entry, so try to delete it through the key element data.
> +	 */
> +	if (!entry) {
> +		struct fdbt_keye_data keye = {};
> +
> +		ether_addr_copy(keye.mac_addr, addr);
> +		keye.fid = cpu_to_le16(vid);
> +		err = ntmp_fdbt_delete_entry_by_keye(ntmp, &keye);
> +

[Severity: High]
Could this inadvertently delete dynamically learned entries belonging to other
ports?

When user-space attempts to delete a non-existent FDB entry on one port,
this fallback uses only the MAC address and VID as the key. Since the target
port number is ignored, it seems this could silently delete a valid dynamic
entry from the hardware that was learned on a completely different port in
the same VLAN.

>  		goto unlock_fdbt;
> +	}
>  
>  	cfge = &entry->cfge;

[ ... ]

> @@ -1494,17 +1526,53 @@ static int netc_port_max_mtu(struct dsa_switch *ds, int port)
>  	return NETC_MAX_FRAME_LEN - VLAN_ETH_HLEN - ETH_FCS_LEN;
>  }
>  
> +static struct net_device *netc_classify_db(struct dsa_db db)
> +{
> +	switch (db.type) {
> +	case DSA_DB_PORT:
> +		return NULL;
> +	case DSA_DB_BRIDGE:
> +		return db.bridge.dev;
> +	default:
> +		return ERR_PTR(-EOPNOTSUPP);
> +	}
> +}
> +
> +static u16 netc_vlan_unaware_pvid(struct dsa_switch *ds,
> +				  const struct net_device *br_ndev)
> +{
> +	struct dsa_port *dp;
> +	int br_num = -1;
> +
> +	if (!br_ndev)
> +		return NETC_STANDALONE_PVID;
> +
> +	dsa_switch_for_each_available_port(dp, ds) {
> +		if (dsa_port_bridge_dev_get(dp) == br_ndev) {
> +			br_num = dp->bridge->num;
> +			break;
> +		}
> +	}
> +
> +	/* The br_num is supposed to be 1 ~ ds->max_num_bridges */
> +	if (WARN_ON(br_num <= 0))
> +		return NETC_STANDALONE_PVID;
> +

[Severity: High]
Are DSA bridge numbers guaranteed to start at 1?

DSA assigns bridge numbers via bridge.num starting at 0. For the first bridge
created, the WARN_ON will continually trigger.

Additionally, NETC_VLAN_UNAWARE_PVID(0) evaluates to 4096, which exceeds the
12-bit VLAN ID limit. When written to the hardware PVID register, it will be
truncated to 0 (NETC_STANDALONE_PVID), causing the first bridge to share the
same broadcast domain as standalone ports.

> +	return NETC_VLAN_UNAWARE_PVID(br_num);
> +}
> +

[ ... ]

> @@ -1689,12 +1776,81 @@ static void netc_port_set_host_flood(struct dsa_switch *ds, int port,
>  	netc_port_remove_host_flood(np, old_host_flood);
>  }
>  
> +static int netc_single_vlan_aware_bridge(struct dsa_switch *ds,
> +					 struct netlink_ext_ack *extack)
> +{
> +	struct net_device *br_ndev = NULL;
> +	struct dsa_port *dp;
> +
> +	dsa_switch_for_each_available_port(dp, ds) {
> +		struct net_device *port_br = dsa_port_bridge_dev_get(dp);
> +
> +		if (!port_br || !br_vlan_enabled(port_br))
> +			continue;

[Severity: High]
Is br_vlan_enabled() evaluating the correct state during the
port_vlan_filtering callback?

When the driver checks the state of the bridges, the bridge core has not yet
updated the br->vlan_enabled flag (it gets updated after the switchdev
notifier returns). 

If a second bridge is being made VLAN-aware, this function will see its old
state (false) and falsely conclude there is still only one VLAN-aware bridge,
bypassing the hardware constraint.

> +
> +		if (!br_ndev) {
> +			br_ndev = port_br;
> +			continue;
> +		}
> +
> +		if (br_ndev == port_br)
> +			continue;
> +
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "Only one VLAN-aware bridge is supported");
> +
> +		return -EOPNOTSUPP;

[Severity: High]
Does returning -EOPNOTSUPP cause the software bridge to silently ignore this
hardware constraint?

The switchdev and bridge cores explicitly ignore -EOPNOTSUPP errors because
that return code typically indicates hardware offload is unsupported, rather
than indicating an invalid configuration. 

Should this return -EINVAL or -EBUSY to properly abort the operation and
prevent state desynchronization?

> +	}
> +
> +	return 0;
> +}
> +

[ ... ]

> @@ -1702,20 +1858,123 @@ static int netc_port_vlan_add(struct dsa_switch *ds, int port,
>  	if (vlan->vid == NETC_STANDALONE_PVID)
>  		return 0;
>  
> +	if (vlan->vid >= NETC_VLAN_UNAWARE_PVID(ds->max_num_bridges)) {
> +		NL_SET_ERR_MSG_FMT_MOD(extack,
> +				       "VID %d~4095 reserved for VLAN-unaware bridge",
> +				       NETC_VLAN_UNAWARE_PVID(ds->max_num_bridges));
> +		return -EOPNOTSUPP;

[Severity: High]
Will returning -EOPNOTSUPP here cause the software bridge to silently accept
the reserved VID?

Similar to the single VLAN-aware bridge check above, the bridge core ignores
-EOPNOTSUPP. Does this need to return -EINVAL to ensure the operation is
properly aborted?

> +	}
> +
>  	untagged = !!(vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED);
> +	err = netc_port_set_vlan_entry(np, vlan->vid, untagged);
> +	if (err)
> +		return err;

[ ... ]

> +static int netc_port_vlan_del(struct dsa_switch *ds, int port,
>  			      const struct switchdev_obj_port_vlan *vlan)
>  {
>  	struct netc_port *np = NETC_PORT(ds, port);
> +	int err;
>  
>  	if (vlan->vid == NETC_STANDALONE_PVID)
>  		return 0;
>  
> -	return netc_port_del_vlan_entry(np, vlan->vid);
> +	err = netc_port_del_vlan_entry(np, vlan->vid);
> +	if (err)
> +		return err;
> +
> +	if (np->pvid == vlan->vid) {
> +		np->pvid = 0;
> +
> +		if (dsa_port_is_vlan_filtering(np->dp))
> +			netc_port_set_pvid(np, 0);

[Severity: High]
If the PVID is reset to 0, will untagged frames on the bridge port leak into
the standalone broadcast domain?

VID 0 is used as NETC_STANDALONE_PVID for all standalone ports. Setting a
bridged port's PVID to 0 means any untagged frames arriving on that port
will be classified to VID 0, potentially forwarding them to standalone ports
and breaking isolation. 

Should the hardware be configured to drop untagged frames instead if no PVID
is configured on a VLAN-aware port?

> +	}
> +
> +	return 0;
> +}
> +

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260527100217.794987-1-wei.fang@oss.nxp.com?part=8