Re: [PATCH net-next 7/9] net: dsa: netc: add VLAN filter table and egress treatment management

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Clearing the software shadow state (`untagged_port_bitmap`) when a port is removed from a VLAN causes desynchronization with the hardware Egress Treatment Table (ETT), leading to incorrect frame tagging if the port is subsequently re-added.
--

commit dda410d83f4ea2d60e53d4dd219537fabab568c5
Author: Wei Fang <wei.fang@nxp.com>

net: dsa: netc: add VLAN filter table and egress treatment management

This commit implements the DSA port_vlan_add and port_vlan_del operations
to enable VLAN-aware bridge offloading on the NETC switch. It maintains
VLAN membership in a VLAN Filter Table and egress tagging rules in an
Egress Treatment Table.

> diff --git a/drivers/net/dsa/netc/netc_main.c b/drivers/net/dsa/netc/netc_main.c
> index d4475ad7ed6c2..4db42c888470c 100644
> --- a/drivers/net/dsa/netc/netc_main.c
> +++ b/drivers/net/dsa/netc/netc_main.c
[ ... ]
> @@ -1025,6 +1054,369 @@ static void netc_switch_get_ip_revision(struct netc_switch *priv)
[ ... ]
> +static int netc_port_del_vlan_entry(struct netc_port *np, u16 vid)
> +{
> +	struct netc_switch *priv = np->switch_priv;
> +	struct netc_vlan_entry *entry;
> +	struct vft_cfge_data *cfge;
> +	int port = np->dp->index;
> +	u32 vlan_port_bitmap;
> +	int err = 0;
> +
> +	mutex_lock(&priv->vft_lock);
> +
> +	entry = netc_lookup_vlan_entry(priv, vid);
> +	if (!entry)
> +		goto unlock_vft;
> +
> +	cfge = &entry->cfge;
> +	vlan_port_bitmap = FIELD_GET(VFT_PORT_MEMBERSHIP,
> +				     le32_to_cpu(cfge->bitmap_stg));
> +	/* If the VLAN only belongs to the current port */
> +	if (vlan_port_bitmap == BIT(port)) {
> +		err = ntmp_vft_delete_entry(&priv->ntmp, vid);
> +		if (err)
> +			goto unlock_vft;
> +
> +		netc_delete_vlan_egress_rule(priv, entry);
> +		netc_del_vlan_entry(entry);
> +
> +		goto unlock_vft;
> +	}
> +
> +	if (!(vlan_port_bitmap & BIT(port)))
> +		goto unlock_vft;
> +
> +	cfge->bitmap_stg &= cpu_to_le32(~BIT(port));
> +	err = ntmp_vft_update_entry(&priv->ntmp, vid, cfge);
> +	if (err) {
> +		cfge->bitmap_stg |= cpu_to_le32(BIT(port));
> +		goto unlock_vft;
> +	}
> +
> +	entry->untagged_port_bitmap &= ~BIT(port);

[Severity: High]
Could this lead to a desynchronization between the software shadow state
and the hardware Egress Treatment Table (ETT)?

When a port is removed from a VLAN with multiple members, the software
untagged bit is cleared here, but it looks like the hardware ETT entry
for this port is left unmodified.

If the port was previously configured as untagged, and is later re-added to
the same VLAN as a tagged port, netc_port_set_vlan_entry() uses
netc_port_vlan_egress_rule_changed() to check for changes:

static bool netc_port_vlan_egress_rule_changed(struct netc_vlan_entry *entry,
                                               int port, bool untagged)
{
        bool old_untagged = !!(entry->untagged_port_bitmap & BIT(port));
        return old_untagged != untagged;
}

Because the software state was cleared during removal, old_untagged evaluates
to false. If the port is re-added as tagged (untagged is also false), the
function will return false and skip updating the ETT hardware.

Does this mean the ETT hardware remains in its stale untagged mode, which
would incorrectly strip VLAN tags from frames egressing this port?

> +
> +unlock_vft:
> +	mutex_unlock(&priv->vft_lock);
> +
> +	return err;
> +}

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