netdev - Re: [PATCH] dsa: vsc73xx: add support for vlan filtering

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210121224505.nwfipzncw2h5d3rw@skbuf>
Date:   Fri, 22 Jan 2021 00:45:05 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Pawel Dembicki <paweldembicki@...il.com>
Cc:     netdev@...r.kernel.org, Linus Wallej <linus.walleij@...aro.org>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] dsa: vsc73xx: add support for vlan filtering

Hi Pawel,

On Wed, Jan 20, 2021 at 07:30:18AM +0100, Pawel Dembicki wrote:
> This patch adds support for vlan filtering in vsc73xx driver.
> 
> After vlan filtering enable, CPU_PORT is configured as trunk, without
> non-tagged frames. This allows to avoid problems with transmit untagged
> frames because vsc73xx is DSA_TAG_PROTO_NONE.
> 
> Signed-off-by: Pawel Dembicki <paweldembicki@...il.com>

What are the issues that are preventing you from getting rid of
DSA_TAG_PROTO_NONE? Not saying that making the driver VLAN aware is a
bad idea, but maybe also adding a tagging driver should really be the
path going forward. If there are hardware issues surrounding the native
tagging support, then DSA can make use of your VLAN features by
transforming them into a software-defined tagger, see
net/dsa/tag_8021q.c. But using a trunk CPU port with 8021q uppers on top
of the DSA master is a poor job of achieving that.

> ---
> +static int
> +vsc73xx_port_read_vlan_table_entry(struct dsa_switch *ds, u16 vid, u8 *portmap)
> +{
> +	struct vsc73xx *vsc = ds->priv;
> +	u32 val;
> +	int ret;
> +
> +	if (vid > 4095)
> +		return -EPERM;

This is a paranoid check and should be removed (not only here but
everywhere).

> +static int vsc73xx_port_vlan_prepare(struct dsa_switch *ds, int port,
> +				     const struct switchdev_obj_port_vlan *vlan)
> +{
> +	/* nothing needed */
> +	return 0;
> +}

Can you please rebase your work on top of the net-next/master branch?
You will see that the API has changed.
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git

> +
> +static void vsc73xx_port_vlan_add(struct dsa_switch *ds, int port,
> +				  const struct switchdev_obj_port_vlan *vlan)
> +{
> +	bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
> +	bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
> +	struct vsc73xx *vsc = ds->priv;
> +	int ret;
> +	u32 tmp;
> +
> +	if (!dsa_port_is_vlan_filtering(dsa_to_port(ds, port)))
> +		return;

Sorry, but no. You need to support the case where the bridge (or 8021q
module) adds a VLAN even when the port is not enforcing VLAN filtering.
See commit:
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=0ee2af4ebbe3c4364429859acd571018ebfb3424

> +
> +	ret = vsc73xx_port_update_vlan_table(ds, port, vlan->vid_begin,
> +					     vlan->vid_end, 1);
> +	if (ret)
> +		return;
> +
> +	if (untagged && port != CPU_PORT) {
> +		/* VSC73xx can have only one untagged vid per port. */
> +		vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port,
> +			     VSC73XX_TXUPDCFG, &tmp);
> +
> +		if (tmp & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA)
> +			dev_warn(vsc->dev,
> +				 "Chip support only one untagged VID per port. Overwriting...\n");

Just return an error, don't overwrite, this leaves the bridge VLAN
information out of sync with the hardware otherwise, which is not a
great idea.

FWIW the drivers/net/dsa/ocelot/felix.c and drivers/net/mscc/ocelot.c
files support switching chips from the same vendor. The VSC73XX family
is much older, but some of the limitations apply to both architectures
nonetheless (like this one), you can surely borrow some ideas from
ocelot - in this case search for ocelot_vlan_prepare.

> +
> +		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> +				    VSC73XX_TXUPDCFG,
> +				    VSC73XX_TXUPDCFG_TX_UNTAGGED_VID,
> +				    (vlan->vid_end <<
> +				    VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT) &
> +				    VSC73XX_TXUPDCFG_TX_UNTAGGED_VID);
> +		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> +				    VSC73XX_TXUPDCFG,
> +				    VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA,
> +				    VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA);
> +	}
> +	if (pvid && port != CPU_PORT) {
> +		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> +				    VSC73XX_CAT_DROP,
> +				    VSC73XX_CAT_DROP_UNTAGGED_ENA,
> +				    ~VSC73XX_CAT_DROP_UNTAGGED_ENA);
> +		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> +				    VSC73XX_CAT_PORT_VLAN,
> +				    VSC73XX_CAT_PORT_VLAN_VLAN_VID,
> +				    vlan->vid_end &
> +				    VSC73XX_CAT_PORT_VLAN_VLAN_VID);
> +	}
> +}